html/template: untyped nil now prints "<nil>"

[bep]

The following test passes on Go 1.10:

import (
	"bytes"
	"html/template"
	"strings"
	"testing"
)

func TestTemplateNil(t *testing.T) {
	data := map[string]interface{}{}
	tpl := `Nil: {{ .noMatch }}`

	var buf bytes.Buffer
	tmpl, err := template.New("").Parse(tpl)
	if err != nil {
		t.Fatal(err)
	}

	if err := tmpl.Execute(&buf, data); err != nil {
		t.Fatal(err)
	}

	result := strings.TrimSpace(buf.String())

	if result != "Nil:" {
		t.Fatal(result)
	}

}

It fails with the current master (Go .11):

--- FAIL: TestTemplateNil (0.00s)
    template_nil_test.go:27: Nil: <nil>

[andybons]

@mvdan @ianlancetaylor

[mvdan]

Thank you for reporting this issue. I did some changes in this cycle to text/template and html/template when it comes to nil values, so I think one of them would be the cause. I'll look into it this week.

[mvdan]

Slightly smaller repro, where 1.10 outputs nothing and tip outputs nil: https://play.golang.org/p/MhLDoPaEDAg

$ go run f.go
<nil>
$ go1 run f.go

[mvdan]

@bep please see CL https://go-review.googlesource.com/c/go/+/109215. In particular:

Before this change and on 1.10, printing a typed nil and an untyped nil resulted in:

null ""

After this change, one will get:

null null

I agree that this is a breaking change to a certain degree, but I'm not sure if going back to ignoring the value (and hence printing nothing) is a great outcome either. Why should untyped nil be different from typed nil? Why should it output nothing?

For example, untyped nils have always printed <no value> in text/template. I'm not sure that it's expected that untyped nil should output nothing in html/template. Then again, I'm not very familiar with the HTML variant, so perhaps I am missing something.

[bep]

I agree that this is a breaking change to a certain degree

Breaking changes are binary. It is either a breaking change or it is not. This case came from a failing test in Hugo, discovered while working on getting Hugo to compile with Go 1.11 (seems that the template API also has changed).

And while the breaking test is of the synthetic/made up type, we have lots of untyped nils, and I'm very close to 100% certain that this particular issue will break real sites/applications in the wild. And this type of breakage tends to be really subtle and hard to discover.

[mvdan]

seems that the template API also has changed

If you mean text/template/parse, that's because it's not a package people should be using directly. If internal packages were a thing when it was added, it would be an internal package.

As for breakage - I generally agree, but in practice it's more subtle. Each new Go release changes the behavior of lots of pieces of the standard library, and it's always possible that some program would break because of those changes.

Given that the documentation of text/template and html/template don't seem to indicate either way, and that tip is more consistent than 1.10, I'm wondering if this kind of change is acceptable. For example, try this typed nil version on 1.10: https://play.golang.org/p/OyF7NAeiktT

Having said that, I'll try to come up with a CL to revert this change in behavior, without reintroducing the crash that I was originally fixing in the package. Even if we decide to keep this long-running inconsistency in place for the sake of Go1, we can always leave a TODO to clean it up for Go2.

[mvdan]

I think I was mistaken, too - my fix above in html/template was for the JS value escaper. It plays no part here. I believe the issue is that html/template inserts the HTML escaper to run in all pipelines, so it ends up running on our untyped nil value.

In 1.10, untyped nil arguments are ignored, so the pipeline doesn't pass anything along and nothing is escaped. In tip, the pipeline passes on the untyped nil value correctly, so the nil value is escaped.

I'm not sure where a fix here could be. I'd imagine that changing the behavior of HTMLEscaper wouldn't be a good solution. Reverting https://go-review.googlesource.com/c/go/+/95215 would be a pity, as that fixed a previous issue.

[gopherbot]

Change https://golang.org/cl/118835 mentions this issue: html/template: ignore single untyped nil values

[ianlancetaylor]

@mvdan I'm not persuaded that your CL is the right approach. In the name of keeping backward compatibility with the test case in this issue, it breaks backward compatibility for other test cases. For example, as far as I can tell, it changes the output of this variant of the test case:

package main

import (
	"bytes"
	"fmt"
	"html/template"
	"log"
	"strings"
	ttemplate "text/template"
)

func main() {
	data := map[string]interface{}{"match": (*byte)(nil)}
	const tpl = `Nil: {{ .noMatch }} {{ .match }} {{ html .noMatch }} {{ html .match }}`
	tmpl, err := template.New("").Parse(tpl)
	if err != nil {
		log.Fatal(err)
	}
	var sb bytes.Buffer
	if err := tmpl.Execute(&sb, data); err != nil {
		log.Fatal(err)
	}
	fmt.Println(strings.TrimSpace(sb.String()))

	ttmpl, err := ttemplate.New("").Parse(tpl)
	if err != nil {
		log.Fatal(err)
	}
	sb.Reset()
	if err := ttmpl.Execute(&sb, data); err != nil {
		log.Fatal(err)
	}
	fmt.Println(strings.TrimSpace(sb.String()))
}

For Go 1.10 this prints

Nil:  <nil> <nil> <nil>
Nil: <no value> <nil> &lt;no value&gt; &lt;nil&gt;

With current tip this prints

Nil: <nil> <nil> <nil> <nil>
Nil: <no value> <nil> &lt;no value&gt; &lt;nil&gt;

With your CL 118835 this prints

Nil:  <nil> <nil> <nil>
Nil: <no value> <nil>  &lt;nil&gt;

So relative to 1.10 the CL fixes html/template to be compatible but text/template is no longer compatible.

I'm not sure what to do at the moment but I don't think it makes sense to add special cases that wind up still breaking backward compatibility.

[gopherbot]

Change https://golang.org/cl/121815 mentions this issue: html/template: ignore untyped nil arguments to default escapers

[ianlancetaylor]

@mvdan I sent https://golang.org/cl/121815. Let me know what you think.

[mvdan]

@ianlancetaylor thanks for the thorough review. You are right - your fix seems to do a much better job at keeping previous behavior, while also being a considerably simpler fix. I'll abandon my CL and review yours.