text/template: fix truth handling of typed interface nils in if and with #30534

bep · 2019-03-02T15:39:27Z

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 #30501

gopherbot · 2019-03-02T15:41:35Z

This PR (HEAD: d8d8c03) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/164958 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-03-02T15:42:59Z

Message from Gobot Gobot:

Patch Set 1:

Congratulations on opening your first change. Thank you for your contribution!

Next steps:
Within the next week or so, a maintainer will review your change and provide
feedback. See https://golang.org/doc/contribute.html#review for more info and
tips to get your patch through code review.

Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.

During May-July and Nov-Jan the Go project is in a code freeze, during which
little code gets reviewed or merged. If a reviewer responds with a comment like
R=go1.11, it means that this CL will be reviewed as part of the next development
cycle. See https://golang.org/s/release for more details.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-02T17:00:17Z

Message from Daniel Martí:

Patch Set 2:

We don't use markdown in commit messages; see https://github.com/golang/go/wiki/CommitMessage

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-02T17:02:49Z

Message from Bjørn Erik Pedersen:

Patch Set 3: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-02T17:04:55Z

Message from Bjørn Erik Pedersen:

Patch Set 3:

Patch Set 2:

We don't use markdown in commit messages; see https://github.com/golang/go/wiki/CommitMessage

I assume that was the code fence wrappers (now removed); I assume backticks around identifiers (e.g. isTru) isn't consiedered to be Markdown.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-02T17:06:52Z

Message from Bjørn Erik Pedersen:

Patch Set 5: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

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 · 2019-03-02T17:11:54Z

This PR (HEAD: 95fc2c8) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/164958 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

gopherbot · 2019-03-02T17:14:52Z

Message from Bjørn Erik Pedersen:

Patch Set 7:

Patch Set 3:

Patch Set 2:

We don't use markdown in commit messages; see https://github.com/golang/go/wiki/CommitMessage

I assume that was the code fence wrappers (now removed, I hope ...); I assume backticks around identifiers (e.g. isTru) isn't consiedered to be Markdown.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-03T15:49:20Z

Message from Daniel Martí:

Patch Set 7:

The commit message is still like it was initially, unless I've missed something?

Remember that if you're using the PR bot, you must make all changes through the PR. It ignores commit messages; you must modify the PR title and description.

I know that's cumbersome, which is why I'd just send a CL directly.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-03T18:19:04Z

Message from Bjørn Erik Pedersen:

Patch Set 8: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-03T18:23:43Z

Message from Bjørn Erik Pedersen:

Patch Set 10: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-03T18:32:06Z

Message from Bjørn Erik Pedersen:

Patch Set 11:

Patch Set 7:

The commit message is still like it was initially, unless I've missed something?

Remember that if you're using the PR bot, you must make all changes through the PR. It ignores commit messages; you must modify the PR title and description.

I know that's cumbersome, which is why I'd just send a CL directly.

I tried both, but it seems to be reverted ... I will have to read up on the Gerrit documentation tomorrow, the section about how to edit the commit message ...

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-03T18:36:25Z

Message from Daniel Martí:

Patch Set 11:

Please note what I mentioned earlier; you must modify the PR body, i.e. the top comment's content.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-03T22:37:01Z

Message from Bjørn Erik Pedersen:

Patch Set 11:

Patch Set 11:

Please note what I mentioned earlier; you must modify the PR body, i.e. the top comment's content.

Yes, I understand that. I have done that several times, but the change gets reverted.

I suppose it is this issue:

#24887

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-03T22:52:59Z

Message from Daniel Martí:

Patch Set 12:

Seems to work fine for me :) See my edit on the PR content.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-03T22:55:05Z

Message from Dmitri Shuralyov:

Patch Set 11:

Please note what I mentioned earlier; you must modify the PR body, i.e. the top comment's content.

Yes, I understand that. I have done that several times, but the change gets reverted.

I see a total of two edits on the PR body:

https://user-images.githubusercontent.com/1924134/53703193-0d32a380-3ddd-11e9-8aea-6b0c86cc3dc2.png

Neither of those were reverted as far as I can see.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-03T23:03:09Z

Message from Bjørn Erik Pedersen:

Patch Set 13:

Patch Set 11:

Please note what I mentioned earlier; you must modify the PR body, i.e. the top comment's content.

Yes, I understand that. I have done that several times, but the change gets reverted.

I see a total of two edits on the PR body:

https://user-images.githubusercontent.com/1924134/53703193-0d32a380-3ddd-11e9-8aea-6b0c86cc3dc2.png

Neither of those were reverted as far as I can see.

OK, I got it now. The "PR body" referred to the original GitHub PR. I edited the commit message here in the Gerrit UI, which got reverted.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-04T03:28:45Z

Message from Dmitri Shuralyov:

Patch Set 14:

OK, I got it now. The "PR body" referred to the original GitHub PR. I edited the commit message here in the Gerrit UI, which got reverted.

Do you think we can improve the wording in the documentation (i.e.,

go/.github/PULL_REQUEST_TEMPLATE

Lines 1 to 3 in 4fb900e

    
           This PR will be imported into Gerrit with the title and first 
        
           comment (this text) used to generate the subject and body of 
        
           the Gerrit change.

and https://golang.org/wiki/GerritBot#how-does-gerritbot-determine-the-final-commit-message) to make it more clear or visible?

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-03-10T21:18:10Z

Message from Daniel Martí:

Patch Set 14:

(3 comments)

This makes sense to me, in theory. There are no comments about explicitly avoiding indirectInterface in if/with, and there are no tests locking the old behavior, so perhaps this was just due to human error.

I also tend to agree that this change makes text/template more consistent. Inside a template, all values are evaluated behind interface{}, so it's easy for nil pointers to suddenly gain a type and become non-nil when automatically translated to an interface.

On the other hand, it seems like text/template has behaved this way for a long time; at least since 2011, from what I can tell. There's a good chance that some template out there will suddenly change behavior, so this would need to be submitted early and contain a clear changelog entry for 1.13.

Adding Rob and Roger for a second opinion, but I think we can give this a go for 1.13. Worst case, if it turns out that this behavior was on purpose, or that it broke too many programs, we can revert and consider other options.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-05-01T23:45:33Z

Message from Bjørn Erik Pedersen:

Patch Set 15:

(3 comments)

Friendly bump. To me this is an obvious fix, but what do I know.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-05-14T00:29:38Z

Message from Russ Cox:

Patch Set 15: Code-Review+2

Looked at with Rob. +2

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-05-14T00:30:31Z

Message from Russ Cox:

Patch Set 15: Run-TryBot+1

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-05-14T00:30:32Z

Message from Gobot Gobot:

Patch Set 15:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=abc6b648

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-05-14T00:34:00Z

Message from Gobot Gobot:

Patch Set 15:

Build is still in progress...
This change failed on misc-vet-vetall:
See https://storage.googleapis.com/go-build-log/abc6b648/misc-vet-vetall_50ae1850.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

gopherbot · 2019-05-14T00:39:53Z

Message from Gobot Gobot:

Patch Set 15: TryBot-Result-1

1 of 19 TryBots failed:
Failed on misc-vet-vetall: https://storage.googleapis.com/go-build-log/abc6b648/misc-vet-vetall_50ae1850.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.

Please don’t reply on this GitHub thread. Visit golang.org/cl/164958.
After addressing review feedback, remember to publish your drafts!

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 #30501 Change-Id: I9c49eed41e737d8f162e39bef1c3b82fd5518fed GitHub-Last-Rev: 95fc2c8 GitHub-Pull-Request: #30534 Reviewed-on: https://go-review.googlesource.com/c/go/+/164958 Reviewed-by: Russ Cox <[email protected]> Run-TryBot: Russ Cox <[email protected]>

gopherbot · 2019-05-14T00:52:14Z

This PR is being closed because golang.org/cl/164958 has been merged.

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 #30501 Change-Id: I9c49eed41e737d8f162e39bef1c3b82fd5518fed GitHub-Last-Rev: 95fc2c82f26d24a457de4deaa7e5756718fbf07c GitHub-Pull-Request: golang/go#30534 Reviewed-on: https://go-review.googlesource.com/c/go/+/164958 Reviewed-by: Russ Cox <[email protected]> Run-TryBot: Russ Cox <[email protected]>

googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Mar 2, 2019

bep force-pushed the with-if-interface branch from 7a2f08b to d8d8c03 Compare March 2, 2019 15:40

bep force-pushed the with-if-interface branch from d8d8c03 to 95fc2c8 Compare March 2, 2019 17:11

bep mentioned this pull request Mar 2, 2019

Update to Go 1.12 gohugoio/hugo#5716

Closed

gopherbot closed this May 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

text/template: fix truth handling of typed interface nils in if and with #30534

text/template: fix truth handling of typed interface nils in if and with #30534

bep commented Mar 2, 2019 •

edited

Loading

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 4, 2019

gopherbot commented Mar 10, 2019

gopherbot commented May 1, 2019

gopherbot commented May 14, 2019

gopherbot commented May 14, 2019

gopherbot commented May 14, 2019

gopherbot commented May 14, 2019

gopherbot commented May 14, 2019

gopherbot commented May 14, 2019

text/template: fix truth handling of typed interface nils in if and with #30534

text/template: fix truth handling of typed interface nils in if and with #30534

Conversation

bep commented Mar 2, 2019 • edited Loading

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 2, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 3, 2019

gopherbot commented Mar 4, 2019

gopherbot commented Mar 10, 2019

gopherbot commented May 1, 2019

gopherbot commented May 14, 2019

gopherbot commented May 14, 2019

gopherbot commented May 14, 2019

gopherbot commented May 14, 2019

gopherbot commented May 14, 2019

gopherbot commented May 14, 2019

bep commented Mar 2, 2019 •

edited

Loading