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: Boolean operation on boolean pointer in a Template does not yield the correct value #12995

Closed
jmcauley opened this issue Oct 20, 2015 · 15 comments
Milestone

Comments

@jmcauley
Copy link

If you have a boolean variable and a pointer to a boolean variable in a template, the behavior of the dereferenced values in the template vary incorrectly.

Playground example: https://play.golang.org/p/q7BZFIui5M

Tested with Go versions 1.3.3, 1.4, and 1.5
Tested on Fedora and Mac OS X

Setup:
type Testvalues struct {
Mybool bool
Ptrbool *bool
}
flag := false
testitem = Testvalues{flag, &flag}

Based on this, I would expect {{.Mybool}} {{not .Mybool}} {{.Ptrbool}} {{not .Ptrbool}} to yield: false true false true
Actual results: false true false false

The pointer to the boolean seems to be dereferenced correctly but the logical not operator applied to it does not yield the correct result.

@ianlancetaylor ianlancetaylor changed the title Boolean operation on boolean pointer in a Template does not yield the correct value text/template: Boolean operation on boolean pointer in a Template does not yield the correct value Oct 20, 2015
@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Oct 20, 2015
@nodirt
Copy link
Contributor

nodirt commented Oct 20, 2015

a pointer value is considered true if it is not nil.

@adg
Copy link
Contributor

adg commented Oct 20, 2015

The question is whether the truthiness test should indirect through the pointer.

@jmcauley
Copy link
Author

Of course not. Pointers are automatically dereferenced in templates. If you look at the playground example I provided, you can see other dereferencing working correctly. A String pointer, for example, returns a string in the template and when you do: {{ len .PtrToAString}} it correctly calculates the length of the string at the other end of the pointer.

And the pointer value be considered true is definitely not what is going on here. As provided in the example, if a bool is set to false and there is a pointer to that bool, then {{ .PtrToABool }} correctly shows the value 'false'. If it was referencing the pointer itself, it would show either True (for existence) or a hex value for the pointer. The problem really only comes up when applying a logical operator to the dereferenced value via the pointer to a boolean.

@nodirt
Copy link
Contributor

nodirt commented Oct 20, 2015

I think we could indirect a value in isTrue. This will affect if and with as well.
Currently {{if .P}}wrong{{end}} where P is a valid pointer to false, prints wrong. https://play.golang.org/p/JiXyfBTUZi

If someone needs to check if a pointer is nil, they could use {{eq .P nil}} (which doesn't work and probably it is a bug).

@adg
Copy link
Contributor

adg commented Oct 20, 2015

I understand the argument to change this, but this is behavior that existing users might be relying on. To make the incompatible change this would need to be clearly a bug, but I'm not sure that it is.

@jmcauley
Copy link
Author

As an addendum, the behavior of logical NOT depends on the initial state. If the pointer has a dereferenced value of False and you apply a NOT, it will stay False. If the pointer has a dereferenced value of True and you apply a NOT, it will become False.

@jmcauley
Copy link
Author

@adg which behavior might people be relying on? The change to have pointers deferenced in templates was made in 2011. If LEN works on a String pointer, then NOT needs to work on a Bool pointer.

@nodirt
Copy link
Contributor

nodirt commented Oct 20, 2015

@jmcauley. The specs are

{{if pipeline}} T1 {{end}}
    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.

Source: https://godoc.org/text/template

They would have to be changed, which violates Go 1 compatibility guidelines:

Specification errors. If it becomes necessary to address an inconsistency or incompleteness in the specification, resolving the issue could affect the meaning or legality of existing programs. We reserve the right to address such issues, including updating the implementations. Except for security issues, no incompatible changes to the specification would be made.

Source: https://golang.org/doc/go1compat

@jmcauley
Copy link
Author

I'm still missing the incompatibility piece. If pipeline in the above spec is a pointer to a bool that is false, then false and it is an empty value and T1 should not be executed and is not executed under the current behavior. But should it not be the case that {{if not pipeline}} T1 {{end}} in the same scenario result in T1 being executed? Execution should happen inside out, first pipeline which dereferences the pointer, then logical not, then if, the truthiness check. What am I missing?

@adg
Copy link
Contributor

adg commented Oct 20, 2015

@nodirt The docs also say

Arguments may evaluate to any type; if they are pointers the implementation automatically indirects to the base type when required.

So "when required" is contentious and open for interpretation.

@adg
Copy link
Contributor

adg commented Oct 20, 2015

@jmcauley people may be relying on the current behavior, which is the truthiness of pointers being whether they are nil or not (and nothing to do with their underlying value).

http://play.golang.org/p/JjXCYt8fFb

Thinking about it in a slightly different context: in terms of template truthiness, the integers 1 and 0 are true and false. But what if someone relies on a pointer to an int being non-nil in their template logic?

{{with .UserCount}}Users: {{.UserCount}}{{end}}

If you pass in a struct { UserCount *int } it is possible to display UserCount: 0. With your proposed change, you could not longer to that.

@nodirt
Copy link
Contributor

nodirt commented Oct 20, 2015

@jmcauley I don't know how this will be resolved, and I hope it will be consistent, but in the meantime you can actually solve your immediate problem by redefining not https://play.golang.org/p/rjCBQmtY-s

@jmcauley
Copy link
Author

@adg I get what you are saying and I think it is a good point. It seems like a non-nil test needs to be separate from working with their underlying value, though. When you do: {{ len .PtrToString }} you aren't getting the length of True or False, you are getting the length of the underlying string after the nil check has been done. It seems to me like you should be doing something like {{ .PtrToSomething eq nil }} rather than {{ not .PtrToSomething }}.

@jmcauley
Copy link
Author

Thanks @nodirt, I'll take a look at that.

@adg
Copy link
Contributor

adg commented Oct 21, 2015

@jmcauley I agree that there may be a different, more sensible set of semantics to use, but these are the semantics we are required to support to remain compatible with existing Go programs. I'm going to close this issue now since there is a work-around for your case, and I don't think there's anything we can do here.

@adg adg closed this as completed Oct 21, 2015
@golang golang locked and limited conversation to collaborators Oct 24, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants