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: make and/or operators short-circuit evaluation #31103

Open
bep opened this issue Mar 28, 2019 · 18 comments

Comments

Projects
None yet
8 participants
@bep
Copy link
Contributor

commented Mar 28, 2019

Note that the documentation is correct regarding this, so I'm not claiming this is a bug/regression etc. But I suspect the original restriction was motivated by "simpler to implement" -- and since this has become increasingly painful for me lately, I'm testing the water with this issue.

There are at least 3 good arguments in favor of my case:

  1. To avoid nil-pointer situations.
  2. To avoid expensive data initialization when not needed.
  3. The behavior would match the most common boolean AND and OR operators in Go. The current situation is very surprising to most people.
package main

import (
	"io/ioutil"
	"log"
	"text/template"
)

type I interface {
	B() bool
}

type T struct {
}

func (t *T) B() bool {
	return true
}

func main() {
	var t I
	templ, _ := template.New("").Parse("{{ if and .Foo .Foo.B }}TRUE{{ end }}")
	if err := templ.Execute(ioutil.Discard, map[string]interface{}{"Foo": t}); err != nil {
		log.Fatal(err)
	}

}

https://play.golang.org/p/gGdMuJ3-3er

Prints:

2019/03/28 08:45:28 template: :1:19: executing "" at <.Foo.B>: nil pointer evaluating interface {}.B
@robpike

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

The execution engine really isn't set up to do this. It's not the answer you want, but use a nested if to avoid unwanted evaluation.

@bep

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

The execution engine really isn't set up to do this. It's not the answer you want, but use a nested if to avoid unwanted evaluation.

I have read and understood the code in text/template, and even if it's "not set up to do this", it should not be too hard to "set it up" to do exactly that. And note that I didn't create this issue only to make the "template life" easier for myself, but I'm on the receiving end of issues/questions from tens of thousands of Go template users, which I think you need to consider before brushing this off as a non-important issue. Because it is.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

Please, I am not "brushing this off as a non-important issue". I am saying that the language evaluates all its arguments before calling a function and, even if it's easy to change (I don't think it is), it is unwise to make such a significant change to the language. The documentation does not offer the feature of early out for binary boolean operators, so it's not a bug. And as I wrote, although it's more inconvenient it's not difficult to solve the underlying issue by rewriting the template.

I believe the regularity of the current model is worth preserving.

For the actual example you posted, it's possible there's an unrelated bug. People have been fiddling with nil evaluation in the template engine and it may be that evaluation of methods with nil receivers has been broken, or never worked.

@beoran

This comment has been minimized.

Copy link

commented Mar 29, 2019

This would be a backwards incompatible change.
Rather, why not introduce some syntactic sugar where {{ if .Foo && if .Foo.B }}TRUE{{ end }} gets expanded to {{ if .Foo }}{{ if .Foo.B }}TRUE{{ end }}{{ end }} ? That would make nil easier to handle and require no changes to the execution model.

@andybons andybons changed the title text/template: make and/or work like &&/|| proposal: text/template: make and/or work like &&/|| Apr 1, 2019

@gopherbot gopherbot added this to the Proposal milestone Apr 1, 2019

@gopherbot gopherbot added the Proposal label Apr 1, 2019

@andybons

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

@mvdan also since improvements have been made to error messages relating to this: #29137

@mvdan

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

To avoid nil-pointer situations.

Do users misunderstand the nil pointer error and thus file issues on Hugo? I'm asking because perhaps making the errors a bit clearer could avoid confusion amongst users, without the need to make backwards incompatible changes.

To avoid expensive data initialization when not needed.

A template always evaluates all arguments at once, so I don't think this should surprise any user with some experience with the template package. Perhaps it's surprising at first, but we can't change this without breaking backwards compatibility or adding new syntax (see below).

The behavior would match the most common boolean AND and OR operators in Go.

I think you answered your own question here; and and or are functions, not operators with their own syntax that change how the arguments are treated. We have a few scenarios:

  1. Changing all calls to evaluate arguments lazily. I think this would break far too many templates.
  2. Changing the and/or functions to evaluate arguments lazily. This would make these calls inconsistent with others, which would add confusion and break some templates, I think.
  3. Start treating and/or as template language "keywords", as suggested by @bep above. Similar to option 2, but it would still be a breaking change.
  4. Introduce new keywords or syntax, like @beoran's idea above. This is always an option, but it seems overkill given that we can already nest ifs.

I don't think any of these four scenarios is good even in the long run, so I lean towards smaller changes like improving the quality of errors.

@mvdan

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

For the actual example you posted, it's possible there's an unrelated bug. People have been fiddling with nil evaluation in the template engine and it may be that evaluation of methods with nil receivers has been broken, or never worked.

By the way, I tried the original code on Go 1.11, and it still failed. The nil pointer error did indeed change a bit, but it's largely still the same. If anyone finds a regression in a template that now errors when it didn't on older Go versions, please provide details.

@beoran

This comment has been minimized.

Copy link

commented Apr 4, 2019

@mvdan I wouldn't call it overkill when there are more than two conditions. Just compare {{ if .Foo && if .Foo.B && if .Foo.B.C }}TRUE{{ end }} with {{ if .Foo }}{{ if .Foo.B }}{{ if .Foo.C }}TRUE{{ end }}{{ end }}{{ end }}. I'd say the former is just as clear as the latter, but more concise.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

I would like to understand better (1) how much work it would be to change them, and (2) whether this would be a breaking change for real templates. Re (2) @beoran asserted that it would, but I can't see why.

@beoran

This comment has been minimized.

Copy link

commented Apr 11, 2019

Allow me to give evidence for my 'assertion'.

Now, it is possible to call functions and methods in templates, also as as arguments to the and() function. If these functions or methods have side effects, then as it stands, the side effects of both functions will be performed. If we were to make and() shortcut evaluation, then in case the first function or method returns true, the second function's side effect will not be perfomed anymore. This breaks backwards compatibility as nothing in the documentation of text/template states that methods or functions used with it may not have side effects.

See this for an example:
https://play.golang.org/p/Zwkc1hIhBj5

package main

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

type Data struct {
}

// Method with side effects
func (foo Data) Foo() bool {
	fmt.Println("Foo!")
	return false
}

// Another method with side effects
func (foo Data) Bar() bool {
	fmt.Println("Bar!")
	return false
}

// Simulate a shortcut and function
func Sand(object interface{}) bool {
  	data := object.(Data)
	return data.Foo() && data.Bar()
}

const TEMPLATE = `{{if and (.Foo) (.Bar)}}{{end}}{{if sand .}}{{end}}`

func main() {
	var data Data
	
	funcMap := template.FuncMap{
		// shortcut and
		"sand": Sand,
	}


	tmpl, err := template.New("andTest").Funcs(funcMap).Parse(TEMPLATE)
	if err != nil {
		log.Fatalf("parsing: %s", err)
	}

	err = tmpl.Execute(os.Stdout, data)
	if err != nil {
		log.Fatalf("execution: %s", err)
	}
}

Note the difference between the side effects of the template with and and the one with sand to see why they are not the same.

@bep

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

So, @beoran is right. Kind of. With the changes proposed in this issue, this very unusual template construct might be considered to be broken by some people, fixed by many others.

{{ if (or .InitA .InitB) }}
{{ end }}

I think of this as a proposal with a net positive effect in this area: You may introduce 3 issues in templates, but you will fix 997 already broken template constructs.

@beoran

This comment has been minimized.

Copy link

commented Apr 11, 2019

Well, I don't know about the prevalence, I tried to search the web, but searching for {{if (or and {{if (and didn't yield much usable results. I am simply looking at it from a strict go1 compatibility promise point of view. Currently, the behavior as is specified. If the specs need to change, in such a backwards incompatible way, then I consider that this becomes a Go2 issue.

@bep

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

If the specs need to change, in such a backwards incompatible way, then I consider that this becomes a Go2 issue.

Or make this new (sensible) behaviour optional, which would make everyone happy.

@andybons

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Or make this new (sensible) behaviour optional, which would make everyone happy.

What would you envision the API looking like for this to be opt-in behavior?

@bep

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

What would you envision the API looking like for this to be opt-in behavior?

There is already an options API in place.

tmpl, _ := New("t1").Parse("{{ if (or .InitA .InitB) }}{{ end }}")
tmpl.Option("logicaloperators=legacy/sensible") // or something
@mvdan

This comment has been minimized.

Copy link
Member

commented Apr 13, 2019

Perhaps I'm missing something, but how would that work? You're passing the option after the parsing has completed. I don't think we want to add any form of lazy or delayed parsing to the package.

Edit: I re-read the thread; I assume this option would be to change the evaluation of arguments, not the syntax parsing.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

One way to provide options to be used by the parser is to put directives in the template itself. Not sure that we want that, just adding it as a possibility.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2019

I spoke to Rob about this, and we are inclined to try changing the default to short-circuit or/and. It should fix far more than it breaks. Let's try this at the start of the Go 1.14 cycle, and if we need to add an option to get the old non-short-circuit operators back, we can do that.

@rsc rsc modified the milestones: Proposal, Go1.14 Apr 24, 2019

@rsc rsc added the early-in-cycle label Apr 24, 2019

@rsc rsc changed the title proposal: text/template: make and/or work like &&/|| text/template: make and/or operators short-circuit evaluation Apr 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.