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

[bep]

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]

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]

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

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

[mvdan]

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

[gopherbot]

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

[bep]

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]

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]

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]

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]

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]

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]

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]

@mvdan OK, I don't agree.