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: pointer to empty slice/map considered truthy #51816

Closed
jo3-l opened this issue Mar 19, 2022 · 5 comments
Closed

text/template: pointer to empty slice/map considered truthy #51816

jo3-l opened this issue Mar 19, 2022 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@jo3-l
Copy link

jo3-l commented Mar 19, 2022

What version of Go are you using (go version)?

$ go version
go version go1.18 windows/amd64

Does this issue reproduce with the latest release?

Yes; checked with Go 1.18 and tip on the playground.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Joe\AppData\Local\go-build
set GOENV=C:\Users\Joe\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.18
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\Joe\AppData\Local\Temp\go-build1916548998=/tmp/go-build -gno-record-gcc-switches

What did you do?

Executed the following template using text/template:

{{ if .PtrEmptySlice }} true {{ else }} false {{ end }}
{{ if .EmptySlice }} true {{ else }} false {{ end }}

where PtrEmptySlice is &[]int{} and EmptySlice is []int{}.

Go playground link.

What did you expect to see?

false for both the pointer to the empty slice and the empty slice; the text/template documentation gives the following description for the if action:

If the value of the pipeline is empty, no output is generated;
otherwise, T1 is executed. The empty values are false, 0, any
nil pointer or interface value, and any array, slice, map, or
string of length zero.
Dot is unaffected.

While pointers to arrays, slices, maps or strings are not explicitly mentioned, the rest of the documentation sets a precedent that when it refers to an array, slice, map or string, pointers are also accepted and indirected prior to being handled. For example, while the description for the range action does not explicitly state that it handles pointers to arrays, slices or maps, in practice it also accepts items of these types.

Similarly, most built-in template functions indirect their inputs before working with them. For example, the len built-in reports 0 for both the pointer to the empty slice and the empty slice, meaning that in certain cases the output for the two if actions below differs, which is unintuitive:

{{ if eq (len $x) 0 }} empty {{ else }} not empty {{ end }}
{{ if not $x }} empty {{ else }} not empty {{ end }}

Given all this, it seems reasonable to me that pointers to empty values should also be treated as empty values, for consistency purposes. While this is inconsistent with the Go language, it is notable that the existing definition is already so; slices of length zero are not zero values for their type in Go, but are treated as such in package text/template.

What did you see instead?

true for the pointer to the empty slice and false for the empty slice.

@seankhliao seankhliao added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 19, 2022
@seankhliao
Copy link
Member

cc @robpike

@ianlancetaylor
Copy link
Contributor

You quote the documentation, which says that a nil pointer is considered to be empty. It follows that a non-nil pointer is not considered to be empty. And &int{} is not a nil pointer. So the current behavior seems to be exactly as documented.

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 20, 2022
@jo3-l
Copy link
Author

jo3-l commented Mar 20, 2022

Thanks for the response.

Fair point about the documentation; I missed that when reading. With that said, I would point out that the documentation is also somewhat contradictory in the other direction: calling the len built-in on a pointer to an empty slice/map yields 0, suggesting that a pointer to an empty slice could be considered empty under the condition "any array, slice, map or string of length zero". Though "slice, map or string" does not strictly include pointers to such, I noted in my original post that the documentation is not precise in this regard. For example, while the range action accepts pointers to slices, its description suggests otherwise, stating that the input pipeline "must be an array, slice, map, or channel".

Additionally, the logic that a nil pointer is considered to be empty implies that a non-nil pointer is not considered to be empty does not feel completely sound to me. For example, if we apply similar logic to the statement nil interface values are considered empty, which is also in the documentation, we get that non-nil interface values are not considered to be empty, which is incorrect: package text/template indirects interfaces before considering truthiness, so interface{}(0) would be considered an empty value.

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Mar 20, 2022

We would be happy to take specific suggestions for improving the documentation.

It's true that operations like range and len handle pointers by indirecting through them, but those operations have no meaning on pointers. The only alternative other than indirecting would be to give an error on a pointer value (and maybe that would have been a better choice, but what's done is done). An operation like if does have a meaning on pointers: a non-nil pointer is true, a nil pointer is false. It would be odd for if to give a meaning to pointers, and then to extend that meaning by in some (but not all) cases indirecting through the pointer.

In any case the behavior is documented and is working as documented. If we change that behavior now, we will certainly break some existing code. We would need a very strong reason to make such a change, a reason much stronger than "it would make the various operations more consistent" (and I'm not even sure that it would make the operations more consistent).

@jo3-l
Copy link
Author

jo3-l commented Mar 20, 2022

Thanks for the explanation. While I do not fully agree with it, I understand the standpoint and agree with the importance to maintain backward compatibility. I do think the documentation regarding when values are indirected could be improved, but I do not have a specific suggestion at the moment. (If I do, I will fill another issue.)

For what it's worth, a user of an application I contribute to came across this behaviour and found it inconsistent as the distinction between pointer and non-pointer values is otherwise mostly opaque for template users. It's not a major issue for us as we maintain a fork of package text/template already.

Given that I don't have a more convincing reason than consistency, closing. Thanks again for your time.

@jo3-l jo3-l closed this as completed Mar 20, 2022
@golang golang locked and limited conversation to collaborators Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

4 participants