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

html/template: add break and continue #23683

Open
dgryski opened this issue Feb 3, 2018 · 14 comments

Comments

@dgryski
Copy link
Contributor

commented Feb 3, 2018

#20531 added break and continue to text/template, but these keywords are not available in html/template.

Running a simple template (https://github.com/campoy/gotalks/blob/master/go1.10/template/main.go) with html/template fails at runtime with

panic: escaping {{continue}} is unimplemented

@bontibon

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2018

I'll send a CL.

https://golang.org/cl/91815

@ALTree

This comment has been minimized.

Copy link
Member

commented Feb 3, 2018

Why do the 1.10 release notes say that html/template supports {{break}} and {{continue}}? Either we add them to html/template before the release, or we need to delete that paragraph from the 1.10 release notes.

I've sent a CL in case we decide to go with the latter.

@dgryski

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2018

html/template is a wrapper around text/template that adds context-senstive escaping. The oversight in the original patch as that the html escaping logic needs to be updated to deal with the new template commands.

@ALTree ALTree added this to the Go1.10 milestone Feb 3, 2018

@ALTree ALTree added the NeedsDecision label Feb 3, 2018

@ALTree

This comment has been minimized.

Copy link
Member

commented Feb 3, 2018

The fix looks small but it's not up to me so labelling this as NeedsDecision for 1.10.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 3, 2018

Change https://golang.org/cl/91815 mentions this issue: html/template: add support for {{break}}, {{continue}}

@andybons

This comment has been minimized.

Copy link
Member

commented Feb 4, 2018

@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

It sure is unfortunate that we never tested that break and continue work in html templates.

I don't believe CL 91815 is sufficient to add them. Break and continue stop the control flow midway through a range loop iteration. The flow-sensitive analysis of HTML context assumes that the loop body runs in full. It would need to be updated to take the early stop into account. I don't see an easy way to do that in the code. Go 1.10 was supposed to have shipped already. It seems to me far too late to add a change this subtle.

It seems like the safest course of action is to remove break and continue from both text/template and html/template for Go 1.10, and then add them back in Go 1.11, with appropriate attention spent on html/template.

Thoughts?

/cc @robpike @mikesamuel @stjj89

@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

I agree, the change to text/template should be rolled back.

@stjj89

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2018

CL 91815 actually looks fine to me. I don't think that html/template assumes that the loop body will run in full; it escapes the range loop so that it will always produce safe output, no matter how it is executed at run-time. Specifically, it unconditionally escapes the nodes in the if and else branches of a range loop, and checks that executing the body of the range loop multiple times does not change its escaping context.

I can't come up with an edge case involving continue or break that the current escaping logic will not handle. I might be missing something, though. It probably makes sense to delay this change to Go 1.11 so we have more time to consider and test this change.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 5, 2018

Change https://golang.org/cl/92155 mentions this issue: text/template: revert CL 66410 "add break, continue actions in ranges"

@mikesamuel

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2018

gopherbot pushed a commit that referenced this issue Feb 6, 2018
text/template: revert CL 66410 "add break, continue actions in ranges"
The new break and continue actions do not work in html/template, and
fixing them requires thinking about security issues that seem too
tricky at this stage of the release. We will try again for 1.11.

Original CL description:

    text/template: add break, continue actions in ranges

    Adds the two range control actions "break" and "continue". They act the
    same as the Go keywords break and continue, but are simplified in that
    only the innermost range statement can be broken out of or continued.

    Fixes #20531

Updates #20531
Updates #23683

Change-Id: Ia7fd3c409163e3bcb5dc42947ae90b15bdf89853
Reviewed-on: https://go-review.googlesource.com/92155
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@rsc

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2018

Exactly. This is why we're rolling it back.

@ALTree ALTree modified the milestones: Go1.10, Go1.11 Feb 7, 2018

@stjj89

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2018

<script>{{if .C}}{{..}}{{else}}{{break}}{{end}}</script>

Ah, that makes sense. We can probably fix this by storing the context whenever we see break or continue, and comparing that context with the end-of-loop context when we get there.

@ALTree ALTree added NeedsFix and removed NeedsDecision labels Feb 7, 2018

@mikesamuel

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.