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: add "return" action #53015

Open
josharian opened this issue May 20, 2022 · 16 comments
Open

text/template: add "return" action #53015

josharian opened this issue May 20, 2022 · 16 comments

Comments

@josharian
Copy link
Contributor

josharian commented May 20, 2022

There is currently no good way to cleanly stop template execution early.

I propose that we add a return action that stops execution of the current template, without error. When using nested templates, the calling template should keep executing. That is, return should be scoped to the current template.

This can be partially implemented with a simple FuncMap entry and a (custom) sentinel error:

"return": func() (string, error) {
    return "", ErrStop
}

The Go code executing the template can then recognize ErrStop and ignore it. However, this implementation stops the entire template execution, including caller (parent) templates. One can write an alternative FuncMap implementation of the template action that handles ErrStop. But that's a lot of machinery, and you must buffer the intermediate template result.

The other alternative is to wrap large chunks of templates with if blocks, which is bad for readability.

Given that Go style encourages early exits, the template package ought to support it.

cc @robpike @mvdan

@gopherbot gopherbot added this to the Proposal milestone May 20, 2022
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) May 20, 2022
@rsc rsc moved this from Incoming to Active in Proposals (old) May 25, 2022
@rsc
Copy link
Contributor

rsc commented May 25, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jun 8, 2022

@josharian Do you know whether this is a trivial change?

@josharian
Copy link
Contributor Author

No idea. I suspect that it is (return a sentinel error, handle the sentinel error), but I haven't tried it. Should I?

josharian added a commit to josharian/go that referenced this issue Jun 8, 2022
DO NOT SUBMIT

This is a quick and dirty experiment to see how
hard it is to implement a return keyword for golang#53015.
@josharian
Copy link
Contributor Author

It appears to be trivial, although I have no history with text/template, so my approach might be unwelcome. I cobbled together josharian@2a1e6f1 with a minimum of effort. (It obviously needs docs and probably more tests.)

@rsc
Copy link
Contributor

rsc commented Jun 15, 2022

That looks entirely reasonable. I put the panics in for break and continue; using them for return too seems OK.

/cc @robpike for any thoughts about whether we want to add 'return'.

@robpike
Copy link
Contributor

robpike commented Jun 15, 2022

Should it return an error? Should it optionally return an error? Never having wanted this facility, I have no experience to lean on regarding how it should behave.

@josharian
Copy link
Contributor Author

It's fairly trivial to write a FuncMap entry that returns an error (see OP).

And I'm not sure where the error would come from: In the typical case, errors come from FuncMaps that you call, and they already interrupt control flow. So the error would have to be passed in via Execute (weird), or we'd have to accept any type of thing as an error and then passed to fmt.Sprint and then errors.New.

I am not opposed to an optional error, but I definitely wouldn't want to make it required.

@josharian
Copy link
Contributor Author

On further thought, I don’t think it should accept an error, as that would create an expectation that the calling template would also exit. But the property that is difficult to achieve with a FuncMap is precisely that nested templates returning early do not impact calling templates.

@rsc
Copy link
Contributor

rsc commented Jun 22, 2022

In general templates don't return values, so plain return alone seems like it makes the most sense: it's like break/continue just for the overall current template "call" and not the enclosing loop.

@hherman1
Copy link

Given that templates don’t return values, would “exit” be clearer?

@josharian
Copy link
Contributor Author

Given that templates don’t return values, would “exit” be clearer?

Neither do some functions. :) I chose "return" because it matches the language, just like if, else, break, and so on. The one exception to that list is range, and it still trips me up every time I use it.

@rsc
Copy link
Contributor

rsc commented Jul 1, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Jul 1, 2022
@robpike
Copy link
Contributor

robpike commented Jul 2, 2022

I'm not sure what is actually being proposed. The conversation has drifted a bit. Can someone please clarify? Is it really just exit early from the current template invocation, and continue execution? If so, is there also need for an "exit", which was also suggested here?

What is the actual problem being solved? Why is it important to stop execution early? I've literally never wanted that.

@josharian
Copy link
Contributor Author

Is it really just exit early from the current template invocation, and continue execution?

Yes.

If so, is there also need for an "exit", which was also suggested here?

I don't think so. "exit" can be easily implemented using a FuncMap; "return" cannot. This is laid out in detail in the original proposal.

What is the actual problem being solved?

I have a lot of templates of the form:

{{ if .X }}
lots of stuff
{{ end }}

I would like to be able to remove the layer of (virtual) indentation and write (as I would in code):

{{ if not .X }}
{{ return }}
{{ end }}
lots of stuff

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Jul 13, 2022
@rsc
Copy link
Contributor

rsc commented Jul 13, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: text/template: add "return" action text/template: add "return" action Jul 13, 2022
@rsc rsc modified the milestones: Proposal, Backlog Jul 13, 2022
@gopherbot
Copy link

Change https://go.dev/cl/425875 mentions this issue: text/template: add "return" action

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Accepted
Development

No branches or pull requests

5 participants