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/parse: add a SkipFuncCheck Mode flag to disable function checking during parse #38627

Closed
a8m opened this issue Apr 23, 2020 · 22 comments

Comments

@a8m
Copy link
Contributor

a8m commented Apr 23, 2020

Following the discussion on #34652 (accepted and added to 1.16) and the proposal of #36911, I'm proposing to add an option to skip the function existence check on parsing, in order to make it possible to parse an arbitrary template text and get its AST.

Currently, if we parse the following template text {{ "foo" | title }} - without passing the functions map we'll get a "function "title" not defined" error. This is not ideal if we plan to add tooling support around the template package (like gopls).

I'm suggesting adding another mode to the parse.Modes, named SkipFuncCheck to address this issue (CL301493).

@gopherbot gopherbot added this to the Proposal milestone Apr 23, 2020
@ianlancetaylor

This comment has been minimized.

@a8m

This comment has been minimized.

@ianlancetaylor

This comment has been minimized.

@a8m

This comment has been minimized.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Jan 6, 2021
@gopherbot
Copy link

Change https://golang.org/cl/301493 mentions this issue: text/template/parse: add mode to skip func-check on parsing

@a8m
Copy link
Contributor Author

a8m commented Mar 20, 2021

Hey @ianlancetaylor, is there any update regarding this issue?

Tagging @stamblerre, because as far as I know, #36911 is blocked on this change.

@ianlancetaylor
Copy link
Contributor

This is waiting for review by the proposal review committee. Sorry for the delay.

@rsc rsc changed the title proposal: text/template/parse: add an option to skip functions existence check on parsing proposal: text/template/parse: add a Mod eflag to skip functions existence check on parsing Mar 24, 2021
@rsc rsc changed the title proposal: text/template/parse: add a Mod eflag to skip functions existence check on parsing proposal: text/template/parse: add a Mode flag to skip functions existence check on parsing Mar 24, 2021
@rsc
Copy link
Contributor

rsc commented Mar 24, 2021

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 rsc moved this from Incoming to Active in Proposals (old) Mar 24, 2021
@rsc rsc changed the title proposal: text/template/parse: add a Mode flag to skip functions existence check on parsing proposal: text/template/parse: add a SkipFuncCheck Mode flag to disable function checking during parse Mar 31, 2021
@rsc
Copy link
Contributor

rsc commented Mar 31, 2021

/cc @robpike

@robpike
Copy link
Contributor

robpike commented Mar 31, 2021

It's OK with me. Hyrum's law.

@rogpeppe
Copy link
Contributor

rogpeppe commented Apr 1, 2021

I support this. It should be possible to do static analysis of templates without the need to provide executable functions.

@robpike
Copy link
Contributor

robpike commented Apr 1, 2021

Another option would be to avoid the check by doing it lazily always, deferring until the first execution. This would be a behavior change, but only for incorrect templates, and might be OK. It would avoid any API change or new feature.

@a8m
Copy link
Contributor Author

a8m commented Apr 4, 2021

Another option would be to avoid the check by doing it lazily always, deferring until the first execution. This would be a behavior change, but only for incorrect templates, and might be OK. It would avoid any API change or new feature.

This change is too big and will affect many programs.

Currently, when users declare a template.Template in a package-level variable, they expect the program to fail on startup if the template is invalid. This is useful in the development flow and important in some deployment system that can detect if a program is unable to start properly and will keep the previous version (e.g. CrashLoopBackOff in Kubernetes). Of course, this can be solved by executing the template on sample data on init, but that's weird.

Another example is when some tools (like ones used by ent) check if a template is valid, and don't have sample data to execute with the template - something like third-party templates.

@rsc
Copy link
Contributor

rsc commented Apr 7, 2021

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) Apr 7, 2021
@mvdan
Copy link
Member

mvdan commented Apr 8, 2021

they expect the program to fail on startup if the template is invalid

I get what you mean, but there's many other ways in which a template could fail during execution. The only proper way to check if a template works is by executing it, e.g. via tests.

Static analysis of templates is a nice idea, as @rogpeppe mentioned, and it should do more than just check that function names can be resolved. For example:

  • With field expressions, if you can tell that the type is a struct, you can check that the field name is valid.
  • You could check that function calls don't receive known-to-be-invalid argument types.
  • Similar to go vet, you could check for invalid printf calls given their arguments.
  • You could flag nonsensical pipelines, such as conditionals which are always true/false.

Personally, I'm leaning towards Rob's idea. This API is for parsing, and I'm not convinced that we should give the impression that it also checks a template's validity.

@a8m
Copy link
Contributor Author

a8m commented Apr 8, 2021

I get what you mean, but there's many other ways in which a template could fail during execution. The only proper way to check if a template works is by executing it, e.g. via tests.

Right, but that's not how it works today. Users rely on some behavior, and I think it's better to leave it as it is today and not introduce this breaking change.

Personally, I'm leaning towards Rob's idea. This API is for parsing, and I'm not convinced that we should give the impression that it also checks a template's validity.

The parser today does more than "parsing", and I agree with most of your proposals, but static-analysis tools can't be written without a parser that can return an AST to check, format, etc (this is how the Go compiler works, right?).

@a8m
Copy link
Contributor Author

a8m commented Apr 8, 2021

@mvdan, I'll ask it this way - I want to format my template files using a standalone tool (e.g. like gofmt for templates).
I'm doing it currently with my template/parse fork, because it's not possible to parse arbitrary template files and get their ASTs. Do you have other suggestions on how to do such a thing?

I know I'm not the only one that is interested in this because this was requested a long time ago (see microsoft/vscode-go#228 and #36911), and this why I'm proposing it.

@mvdan
Copy link
Member

mvdan commented Apr 8, 2021

Maybe I wasn't clear. I support this proposal, but I argue that a new option isn't necessary, as explained here.

@rsc
Copy link
Contributor

rsc commented Apr 14, 2021

Regarding what Rob said and mvdan reemphasized above:

Another option would be to avoid the check by doing it lazily always, deferring until the first execution. This would be a behavior change, but only for incorrect templates, and might be OK. It would avoid any API change or new feature.

For better or worse the current check really is useful for catching typos and the like. The fact that there are some things that don't get caught doesn't seem like a strong argument for not catching the things we can.

We don't catch all set-and-not-used in the Go compiler, but that isn't an argument for removing all the set-and-not-used checks that we do have.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Apr 14, 2021
@rsc
Copy link
Contributor

rsc commented Apr 14, 2021

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/parse: add a SkipFuncCheck Mode flag to disable function checking during parse text/template/parse: add a SkipFuncCheck Mode flag to disable function checking during parse Apr 14, 2021
@rsc rsc modified the milestones: Proposal, Backlog Apr 14, 2021
@a8m
Copy link
Contributor Author

a8m commented Apr 17, 2021

CL301493 is ready for review, and it's going to be useful for CL297871 as well.

@gopherbot
Copy link

Change https://golang.org/cl/311569 mentions this issue: doc/go1.17: document text/template/parse.SkipFuncCheck

gopherbot pushed a commit that referenced this issue May 29, 2021
Documents the newly added mode that skips type checking
functions as per CL 301493.

Fixes #46025
For #34652
For #44513
For #38627

Change-Id: I56c4f65924702a931944796e39f43cfeb66abc8a
Reviewed-on: https://go-review.googlesource.com/c/go/+/311569
Reviewed-by: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
Trust: Michael Knyszek <mknyszek@google.com>
@rsc rsc mentioned this issue Jun 10, 2021
83 tasks
@golang golang locked and limited conversation to collaborators May 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

7 participants