Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

text/template: arbitrary template picked on empty blocks #40186

Closed
bep opened this issue Jul 13, 2020 · 14 comments
Closed

text/template: arbitrary template picked on empty blocks #40186

bep opened this issue Jul 13, 2020 · 14 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@bep
Copy link
Contributor

bep commented Jul 13, 2020

Ref. the documentation:

A block is shorthand for defining a template
{{define "name"}} T1 {{end}}
and then executing it in place
{{template "name" pipeline}}

The program below prints:

Not Empty: 
notempty1-overlay|
notempty2-block|
Empty: empty-foo|
Empty2:     |

The below is a common construct to create master/overlay constructs, and it's also common to leave the blocks empty and optional. I would expect Empty: empty-foo| to be Empty:| -- and not just pick an arbitrary template with the same name. I have added the Empty2: | to show that empty template defintions (e.g. whitespace only) works fine on its own.

package main

import (
	"fmt"
	"log"
	"os"
	"text/template"
)

func main() {
	var (
		master = `
Not Empty: 
{{ block "notempty1" . }}notempty-block{{ end }}|
{{ block "notempty2" . }}notempty2-block{{ end }}|
Empty: {{ block "empty" . }}   {{ end }}|
Empty2: {{ template "empty2" . }}|`
		overlay = `{{ define "notempty1" }}notempty1-overlay{{ end }}`
	)

	templs := template.New("")
	templs.New("foo").Parse(`
{{ define "notempty1" }}notempty1-foo{{ end }}
{{ define "notempty2" }}notempty2-foo{{ end }}
{{ define "empty" }}empty-foo{{ end }}
{{ define "empty2" }}    {{ end }}

`)

	masterTmpl, err := templs.New("master").Parse(master)
	if err != nil {
		log.Fatal(err)
	}
	overlayTmpl, err := template.Must(masterTmpl.Clone()).Parse(overlay)
	if err != nil {
		log.Fatal(err)
	}

	if err := overlayTmpl.Execute(os.Stdout, ""); err != nil {
		log.Fatal(err)
	}

	fmt.Println()

}
@andybons
Copy link
Member

@robpike @mvdan

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 13, 2020
@andybons andybons added this to the Unplanned milestone Jul 13, 2020
@mvdan
Copy link
Member

mvdan commented Aug 30, 2020

There aren't any tests that cover this use case; it's always a block being re-defined in an overlay, like in this example: https://golang.org/pkg/text/template/#example_Template_block

So it's hard for me to tell if the current behavior is explicitly by design, or if it's an unintended behavior that we should fix. The feature was added in https://go-review.googlesource.com/c/go/+/14005/ in 2015, so I hope @adg @robpike @rogpeppe can chime in.

@mvdan
Copy link
Member

mvdan commented Aug 30, 2020

Also, here's a playground link with source code that I found easier to understand: https://play.golang.org/p/ILdl-3v1edT

@adg
Copy link
Contributor

adg commented Aug 31, 2020

I'm not sure what the confusion is here.

If you define a block then it redefines the earlier template of the same name with the contents of the block.

Maybe this is a documentation issue?

@mvdan
Copy link
Member

mvdan commented Aug 31, 2020

The confusion on my part is whether this is working as intended, thus the documentation should be tweaked, or if it's an implementation bug like @bep says, and then we should fix the implementation to make block ignore previous templates with the same name.

@bep
Copy link
Contributor Author

bep commented Aug 31, 2020

If my issue above was a little long, I think it boils down to this:

{{ block "empty" . }}   {{ end }}

Is currently not considered to be a valid template definition. However, this is:

{{ define "empty" . }}   {{ end }}

@adg
Copy link
Contributor

adg commented Aug 31, 2020

What do you mean by "valid template definition"? It doesn't get defined?

Btw I don't think this works with the .:

{{ define "empty" . }}   {{ end }}

It should be

{{ define "empty"}}   {{ end }}

Can you boil it down to a tiny play.golang.org example to demonstrate what you mean?

@bep
Copy link
Contributor Author

bep commented Sep 1, 2020

It doesn't get defined?

According to the documentation, it does:

A block is shorthand for defining a template
{{define "name"}} T1 {{end}}
and then executing it in place
{{template "name" pipeline}}

(and yes, my example above had a cut and paste-error).

So this:

{{ block "empty" . }}   {{ end }}

Should be the same as:

{{define "empty"}}   {{end}}
{template "empty" . }}

But it isn't.

@adg
Copy link
Contributor

adg commented Sep 1, 2020

These are the same: https://play.golang.org/p/-Wf47qTqKu1

What's the distinction you're drawing here?

In your examples in the first comment I see template redefinitions - is that the source of confusion?

The docs for Parse say:

Templates can be redefined in successive calls to Parse. A template definition with a body containing only white space and comments is considered empty and will not replace an existing template's body. This allows using Parse to add new named template definitions without overwriting the main template body.

In this example we can see that by parsing a template definition of the same name effectively shadows the original one. I show it using both block, and the equivalent using both define and template.

If I'm still not understanding you, can you please alter the example and share it here so that it demonstrates the problem you're seeing? Thanks.

@bep
Copy link
Contributor Author

bep commented Sep 1, 2020

What's the distinction you're drawing here?

The distinction between an empty template defined separately or as part of a block definition. Your example does not resemble my example.

@adg
Copy link
Contributor

adg commented Sep 1, 2020

Defining an empty template will not redefine an existing template, as per the docs on Parse (that I quoted earlier):

A template definition with a body containing only white space and comments is considered empty and will not replace an existing template's body.

Again, I observe that block and define/template behave the same way: https://play.golang.org/p/RkgJ3eDhKCp

If I'm still not understanding you, can you please alter my example and share it here so that it demonstrates the problem you're seeing? Thanks.

@adg adg added WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 1, 2020
@gopherbot
Copy link

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@bep
Copy link
Contributor Author

bep commented Oct 2, 2020

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

This is a mistake.

@mvdan
Copy link
Member

mvdan commented Oct 2, 2020

As far as I can tell, the label and bot worked as intended here. @adg asked a question in #40186 (comment). If you can answer it, we can reopen the thread.

@golang golang locked and limited conversation to collaborators Oct 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

5 participants