-
Notifications
You must be signed in to change notification settings - Fork 17.4k
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
Comments
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 |
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. |
This would be a backwards incompatible change. |
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.
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).
I think you answered your own question here;
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. |
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. |
@mvdan I wouldn't call it overkill when there are more than two conditions. Just compare |
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. |
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 See this for an example: 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 |
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.
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. |
Well, I don't know about the prevalence, I tried to search the web, but searching for |
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? |
There is already an options API in place. tmpl, _ := New("t1").Parse("{{ if (or .InitA .InitB) }}{{ end }}")
tmpl.Option("logicaloperators=legacy/sensible") // or something |
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. |
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. |
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. |
Go templating evaluates all arguments to a function. As a result, and/or operators will fail under certain conditions when Level is a string rather than a Level struct. Having `Level` potentially be a non Level type is probably bad Go practice anyway. Keeping this type consistent somewhat simplifies the logic of the nav. This issue with operator evaluation is described in depth here, and appears to be something that will change: golang/go#31103
Change https://golang.org/cl/321490 mentions this issue: |
Change https://golang.org/cl/353130 mentions this issue: |
There was a bug in the short-circuit code for and/or added in CL 321490: it ignored the value passed in by an earlier pipeline. For #31103 Change-Id: Ic31f4d7cedfe563ef968cbb712ecfb2413c42eb5 Reviewed-on: https://go-review.googlesource.com/c/go/+/353130 Trust: Ian Lance Taylor <[email protected]> Run-TryBot: Ian Lance Taylor <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]> TryBot-Result: Go Bot <[email protected]>
Change https://golang.org/cl/353610 mentions this issue: |
For #31103 Change-Id: I9c0aa64f95f564de31a4c178e3930584d41316bb Reviewed-on: https://go-review.googlesource.com/c/go/+/353610 Trust: Ian Lance Taylor <[email protected]> Run-TryBot: Ian Lance Taylor <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]>
Change https://golang.org/cl/354091 mentions this issue: |
In the last CL I missed the fact that except for the final value the code already unwraps the argument. For #31103 Change-Id: Ic9099aeb50c6b3322fc14a90ac8026c1d8cb1698 Reviewed-on: https://go-review.googlesource.com/c/go/+/354091 Trust: Ian Lance Taylor <[email protected]> Run-TryBot: Ian Lance Taylor <[email protected]> Reviewed-by: Bryan C. Mills <[email protected]>
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:
nil-pointer
situations.AND
andOR
operators in Go. The current situation is very surprising to most people.https://play.golang.org/p/gGdMuJ3-3er
Prints:
The text was updated successfully, but these errors were encountered: