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: Invalid handling off typed nils in if and with #30501

Closed
bep opened this issue Mar 1, 2019 · 12 comments
Closed

text/template: Invalid handling off typed nils in if and with #30501

bep opened this issue Mar 1, 2019 · 12 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@bep
Copy link
Contributor

bep commented Mar 1, 2019

This issue is a rework of #30481, which I felt was closed a little prematurely. This is, on one or more levels, a real issue that needs to be addressed. I will try to make this shorter and to the point.

The program below prints:

<nil>:	         IsTrue: false: Nil == nil: true 
*main.Nill:	 IsTrue: false: Nil == nil: false Failed, got <nil>

Also see https://play.golang.org/p/HzwnX062jjk

The documentation for template.IsTrue states that

IsTrue reports whether the value is 'true', in the sense of not the zero of its type, and whether the value has a meaningful truth value. This is the definition of truth used by if and other such actions.

So, while I understand that Nil != nil in the second example above, which, I suspect, is part of the reason why the template package uses reflect. I would expect that all values that is truthful according to template.IsTrue is also truthful in if and with.

package main

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

type Niller interface {
	Nil() Niller
	Foo() string
}

type Nill struct {
	nilv Niller
}

func (n Nill) Nil() Niller {
	return n.nilv
}

func (n Nill) Foo() string {
	return "asdf"
}

func main() {

	tmpl, err := template.New("").Parse(`
{{- with .Niller.Nil }}{{ if not $.IsTrue }}Failed, got {{ . }}{{ end }}
{{- else }}{{ if $.IsTrue }}Failed else{{ end -}}
{{ end -}}
`)
	if err != nil {
		log.Fatal(err)
	}

	var (
		nil1 Niller
		nil2 *Nill
	)

	execute(tmpl, nil1)
	execute(tmpl, nil2)

}

func execute(tmpl *template.Template, niller Niller) {
	var buff bytes.Buffer
	n := &Nill{nilv: niller}
	isTrue, ok := template.IsTrue(n.Nil())
	if !ok {
		log.Fatal("IsTrue failed")
	}

	data := map[string]interface{}{
		"IsTrue": isTrue,
		"Niller": n,
	}

	err := tmpl.Execute(&buff, data)
	if err != nil {
		log.Fatal(err)
	}

	fmt.Printf("%T:\t IsTrue: %t: Nil == nil: %t %s\n", niller, isTrue, (n.Nil() == nil), buff.String())
}
@bcmills
Copy link
Contributor

bcmills commented Mar 1, 2019

The template language sees .Niller.Nil as a typed interface value (a Niller), and evaluates whether the value is the zero Niller.

However, template.IsTrue accepts an interface{}, so the Niller interface is converted to plain interface{}, and it reports the truthiness of the underlying concrete value: that is, it evaluates whether the value is the zero *Nill.

The difference you're seeing here is due to the fact that Go implicitly converts a Niller to an interface{}, while the template language does not.

@bcmills
Copy link
Contributor

bcmills commented Mar 1, 2019

CC @mvdan, but I'm pretty sure this is working as documented.

(nil interfaces vs. nil pointers can be confusing.)

@bcmills bcmills added arch-ppc64x NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed arch-ppc64x labels Mar 1, 2019
@bcmills bcmills added this to the Unplanned milestone Mar 1, 2019
@mvdan
Copy link
Member

mvdan commented Mar 1, 2019

I will have a proper look soon, just not today.

bep added a commit to bep/go that referenced this issue Mar 2, 2019
Before this commit, the two logically equivalent conditionals below would produce different output:

```
{{ if not .NonEmptyInterfaceTypedNil }}OK{{ else }}{{ end }}
{{ if .NonEmptyInterfaceTypedNil }}{{ else }}OK{{ end }}
```

The functions `not`, `or`, and `and` all use the same `truth` function, which unwraps any concrete interface value before passing it to ´isTrue`.

`if` and `with` also use `isTrue` to establish truth, but was missing the interface indirect call.

Fixes golang#30501
bep added a commit to bep/go that referenced this issue Mar 2, 2019
Before this commit, the two logically equivalent conditionals below would produce different output:

```
{{ if not .NonEmptyInterfaceTypedNil }}OK{{ else }}{{ end }}
{{ if .NonEmptyInterfaceTypedNil }}{{ else }}OK{{ end }}
```

The functions `not`, `or`, and `and` all use the same `truth` function, which unwraps any concrete interface value before passing it to `isTrue`.

`if` and `with` also use `isTrue` to establish truth, but was missing the interface indirect call.

Fixes golang#30501
@gopherbot
Copy link

Change https://golang.org/cl/164958 mentions this issue: text/template: fix truth handling of typed interface nils in if and with

bep added a commit to bep/go that referenced this issue Mar 2, 2019
Before this commit, the two logically equivalent conditionals below would produce different output:

{{ if not .NonEmptyInterfaceTypedNil }}OK{{ else }}{{ end }}
{{ if .NonEmptyInterfaceTypedNil }}{{ else }}OK{{ end }}

The functions `not`, `or`, and `and` all use the same `truth` function, which unwraps any concrete interface value before passing it to `isTrue`.

`if` and `with` also use `isTrue` to establish truth, but was missing the interface indirect call.

Fixes golang#30501
@bep
Copy link
Contributor Author

bep commented Mar 3, 2019

I would appreciate if this was considered for Go 1.12.1. I understand that my definition of what is and how severe a bug is may differ from others', but please consider the construct below:

{{ if .AuthenticatedUser }}User is authenticated!{{ else }}{{ end }}
{{ if not .AuthenticatedUser }}{{ else }}}User is authenticated!{{ end }}

In Go <= Go 1.12 the two logically equivalent statements above could produce different results (I have tested it with versions I had on my PC, which was down to Go 1.10).

I maintain a Go project that I'm pretty sure has the most widespread use of end user defined Go templates, and I don't think I can wait for 3-4 months for a Go main release before I address this issue for that user base, which I fear would mean a temporary fork of some sort ...

@mvdan
Copy link
Member

mvdan commented Mar 3, 2019

If you think there was a regression in 1.12, could you share a program that does something sensible on 1.11.5 and something different on 1.12?

@bep
Copy link
Contributor Author

bep commented Mar 3, 2019

If you think there was a regression in 1.12

As I said, this is not a regression in 1.12. It behaves like this at least >= Go 1.10 <= Go 1.12.

@bcmills
Copy link
Contributor

bcmills commented Mar 3, 2019

This definitely should not be backported to 1.12.

It is quite likely that some existing programs depend on the current behavior, even if it is buggy. Programs that depend on relatively benign bugs should only break at major releases.

(see also http://www.hyrumslaw.com/)

@mvdan
Copy link
Member

mvdan commented Mar 3, 2019

I was actually going to say that this would be a somewhat risky change to do in 1.13, as there's a notable chance that some templates out there will break with the change. That's still something we can consider for 1.13 with a proper changelog entry, but not for 1.12.x. If a template worked in 1.11.x and 1.12, it shouldn't break in 1.12.1; even if one could argue that the template was incorrect.

@bep
Copy link
Contributor Author

bep commented Mar 6, 2019

OK, then I need to take on the work myself and fix this for all the users of my applications.

But you really, really need to add a big warning sign somewhere visible in the documentation. If you don't think that's a good idea, think what you would do if this somehow failed in certain common situations:

if true != !false {
  log.Fatal("black hole?")
}

benign bugs should only break at major releases.

@bcmills you and I have somewhat different views on bugs and their severity, but I had to look in the dictionary for the word benign (I'm from Norway), and I would not use that word.

@mvdan
Copy link
Member

mvdan commented Mar 6, 2019

But you really, really need to add a big warning sign somewhere visible in the documentation.

I personally think having this bug filed and fixed for 1.13, whichever the fix is, would be enough. Sure there's a known bug in 1.12.x, but we don't add documentation warnings for those.

and I would not use that word.

I think Bryan said "benign" in the context that this is a bug which simply makes a template's behavior slightly different and/or inconsistent. In contrast, a "bad" bug would make a program crash, error, or would be a regression from 1.11 to 1.12.

@bep
Copy link
Contributor Author

bep commented Mar 6, 2019

@mvdan OK, I don't agree.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants