text/template/parse: add a SkipFuncCheck Mode flag to disable function checking during parse

[a8m]

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]

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

[a8m]

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

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

[ianlancetaylor]

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

[rsc]

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]

/cc @robpike

[robpike]

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

[rogpeppe]

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

[robpike]

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]

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]

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

[mvdan]

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]

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]

@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]

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

[rsc]

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]

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

[a8m]

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

[gopherbot]

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