It isn't clear what you are proposing. Does the argument function map replace the complete set available, does it replace only those in the template, or is it just a place to look first?

You don't say what the function signatures are, either. Please edit the proposal to make it clearer.

I hope that I've made this clearer for you. PR #54432 hopefully shows the intended API in practice.

A further optimization in #54432 would be to only check and convert the overlay functions in to a valueFunc when the function is called instead of beforehand.

I hope that I've made this clearer for you. PR #54432 hopefully shows the intended API in practice.

Thanks, that is clear now.

The new names are cumbersome (but could be changed), while thought needs to be given to whether this is the right approach.

I agree that they're slightly cumbersomely named but they are consistent with naming practices elsewhere in std. Another option could be to make them ExecuteWithFuncMap.

There is genuinely a need to provide funcmaps at time of execution differently from those used at parsing, and whilst such funcmaps are related - certainly the parsing funcmap constrains what types of funcs and the names of funcs - they're not the same.

This is recognised in the current API by having template.Funcs do two different things before and after parsing. This results in Funcs having to have an explicit documentation warning about this different behaviour(!). Internally they're also stored separately. The problem is this method necessarily changes the template - and there is no way to express a desire only change a single execution of a template.

In a lot of ways the naturality of execution funcmaps can be seen in how simple it is to implement. Apart from a small difficulty regarding changing the provided funcmap in to a valuemap there is almost nothing to do to implement it.

Now, how else could this be implemented?

The suggestion of Clone Funcs Execute hasn't worked in the past and isn't going to work. It's too hard to speed up and in a lot of ways it's not the right thing. We don't want to change the template - just this execution of the template.

That leaves the question of is there a way to expose execution state outside of the current functions? Well... The current API means that a template.Template is a lot of somewhat disparate things. It is not simply a single parsed template, but a collection of parsed templates, a parser and an executor all of itself. That's fine and makes for a simple API but at some point it might be sensible to actually separate those things out and expose them individually. Is this the reason to do that? I don't necessarily believe it is. (Certainly any attempt to implement this functionality without the proposed functions would at least result in partial separation out of the executor from the template.) Does this API prevent that or make that migration harder? Again I don't think so. Any separation out is going to have to expose this kind of functionality and it would map nicely on to that.

I should add that the slight distaste in the naming of the go API here should be balanced against the substantial cumbersomeness the current API is imposing on the template API, (and the impracticality of the suggested workaround).

A codebase using multiple templates is likely to have only one or two places where ExecuteFuncMap is written - compare that to every template having to account for the lack of execution funcmaps, pretending that contextual functions are somehow data and then having to fiddle about trying to remember if it is .Tr, $.Tr, $.root.Tr or even having to go back and change every call of a template to add root in. Whilst one can think of a locale as data to be rendered it is somewhat orthogonal to it and is genuinely a context of a render. (This extends to other things but locale is the simplest case.)

The currently suggested workaround of using Clone() + Funcs() + Execute() is slow and would require cloning all of the templates needed on a per execution basis. The current structure of templates means that can never be quick because the execution funcmaps are stored within the templates themselves and these have to have a concurrency lock because templates can parse themselves. This fundamentally cannot be sped up without significant restructuring of the whole of the text/template package whereas the proposed change is easy to do.

When balancing these factors, although these method names are slightly cumbersome, it is likely that only one of these 4 methods will be used only once or twice in a codebase. Not having them is making templates incredibly cumbersome and error prone. This suggests really that they would be a net reduction in cumbersomeness.

@bep, would this simplify the implementation of Hugo? Being able to call a page function in templates would be nice.

Dear golang team have there been any further thoughts about this?

@carlmjohnson I don't understand this issue very well (I have only skimmed it). In Hugo we have a fork of the execution part of Go's template packages with some small adjustments (we have a script that keep the 2 in sync); one of them is a ExecuteWithContext method, which I think make more sense re. passing contextual data. And yes, it will eventually allow you to do page.Foo once I find time to wire everything ...

Forking the execution code is part is something I am trying to avoid...

Is this proposal still under discussion? I agree with zeripath that the customized FuncMap is really helpful for large projects.

Without this approach, I guess the developers should do:

1. Parse all templates
2. Each time, Clone a template to new one, and inject new functions (eg: with Context)
3. The cloned templates could be managed by an object pool, because the Clone seems heavy.

According my test, this approach is not feasible, for a site with 400 templates:

1. It wastes too much memory , 55M (no clone) vs 3.7G (clone)
2. It's quite slow, each clone takes ~ 10-20ms, the program startup time increases from nearly zero to 10s.

@robpike I also think this is the right approach, is there any more concern?

I still believe that #38114 is the path forward here. The fact that no one has had time to do it does not necessarily imply it is difficult or impossible, just low priority.

The clone idea could be more complex than the ExecFuncMap idea. I have succeeded to get the per-calll func map work by:

• ScopedTemplate: https://github.com/go-gitea/gitea/blob/e6589ed257af1cee35713a97d5db9de6dcc48c70/modules/templates/scopedtmpl/scopedtmpl.go
• And the test: https://github.com/go-gitea/gitea/blob/e6589ed257af1cee35713a97d5db9de6dcc48c70/modules/templates/scopedtmpl/scopedtmpl_test.go

Without official support, there are a lot of work to do, because the sub-templates share the internal state:

1. Make a new html/template/Template for all templates
2. Add template code to the all template
3. Freeze the all template, reset its exec func map, it shouldn't execute any template.
4. When a caller wants to render a template by its name
   1. Find the name in all
   2. Find all its related sub templates
   3. Escape all related templates (just like what the html template package does)
   4. Add the escaped parse-trees of related templates into a new (scoped) text/template/Template
   5. Add context-related func map into the new (scoped) text template
   6. Execute the new (scoped) text template
   7. To improve performance, the escaped templates are cached to template sets

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

I have talked about this before, without seeing much enthusiasm, but I'll repeat the bullet points.

Hugo used to do a clone of the full template set for each language to be able to have language specific template functions. This was ... not great, as many have mentioned above.

Now we have a "soft fork" of Go's template packages where we have made some slight adjustments to the execution flow. The relevant part in this discussion could be illustrated with the 2 interfaces below:

// Preparer prepares the template before execution.
type Preparer interface {
	Prepare() (*Template, error)
}

// Executer executes a given template.
type Executer interface {
	ExecuteWithContext(ctx context.Context, p Preparer, wr io.Writer, data any) error
}

With the above, Executer holds a map of functions and the templates can be reused. The Prepare method above is just for the (little bit odd) t.escape() construct in the html package.

@bep would the proposed API help hugo get rid of the "soft fork" of Go's template? (or at least remove some customization)

I would welcome such a change to get rid of the Clone call for the csrf field.

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

Hello ~~ kindly ping .... @rsc is there any progress (weekly review)?

Update: if the name is somewhat "cumbersome", it could only keep the "ExecuteFuncMap" since "ExecuteTemplateFuncMap" is only a wrapper. "ExecuteFuncMap" is good enough and in most cases developers only need it.

@bep would the proposed API help hugo get rid of the "soft fork" of Go's template?

@oliverpool It would be a step forward ... I can understand why the Go std library team would be a little worried about changing the API, but it should be easy to keep to old while getting a, in my eye, much more performant and clean design where the concerns of parsing and execution are clearly separated.

Change https://go.dev/cl/510738 mentions this issue: text/template, html/template: add ExecuteFuncs

I spent a while looking into optimizing Clone, and while I've convinced myself it can be done with significant changes, it seems that not making those changes is the better part of valor.

I also think there's no need for ExecuteTemplateFuncMap, since ExecuteTemplate is just a helper wrapper around Lookup+Execute and can be left alone.

Finally I think we can shorten ExecuteFuncMap to ExecuteFuncs, especially since the Template method is Funcs.

So this proposal shortens to just adding ExecuteFuncs in the two packages, which I've sent CL 510738. I've retitled it as such.

Please try out that CL and see if it does everything you want it to do. Thanks!

The only question in my mind is:

Is it necessary to call createValueFuncs in func (t *Template) execute() ?

The code is a hot path, checking the funcs or not, the result could be the same.

Suppose a developer passes "funcs" into ExecuteFuncs:

• With createValueFuncs in func (t *Template) execute():
  • It uses loops to check many things, which is slow
  • ExecuteFuncs returns an error or panics if any func is not valid
• Without createValueFuncs in func (t *Template) execute():
  • Just add the funcs into the state.funcs
  • ExecuteFuncs returns an error or panics if any func is not valid

Eventually there seems no different, while "without createValueFuncs" is more performant

Please comment on the CL for that implementation detail. I don't think it affects the proposal. Thanks!

@bep, does this look good to you? Can you try it in Hugo and see if it lets you remove one of the divergences?

@rsc so, the current set of execution related methods duplicated in both text/template.Template and html/template.Template are:

• Execute
• ExecuteTemplate

With the addition of ExecuteFuncs we get

• Execute
• ExecuteTemplate
• ExecuteFuncs

When you eventually decide you want to add context.Context to these, you get:

• Execute
• ExecuteTemplate
• ExecuteFuncs
• ExecuteWithContext
• ExecuteTemplateWithContext
• ExecuteFuncsWithContext

Or something.

I have read your recent blog articles about storing state in control flow etc., but reading the above, I would say that it would make perfect sense to introduce a new exported struct type that would be responsible to execute a given template. This new struct would hold the funcs (which could be initialized once at construction time) and you could get a API ala:

• TemplateExec.Execute(TemplateInterface, Writer) error
• TemplateExec.ExecuteWithContext(Context, TemplateInterface, Writer) error

Or something. This would also support most future requirements that may pop up in this area.

@bep It seems questionable to me, that you assume there will be context.Context variants thus doubling the numbers of relevant methods. I see no reason why template-execution would take a context. It's not a network operation. And in the rare corner cases where someone does believe they need it, they can create closures over them and use those in a FuncMap passed to ExecFunc.

And even if we decided we need a native way to call context functions, I don't see why it couldn't take the form of a single func (Template) WithContext(context.Context) Template method - the FuncMap functions are called via reflect anyways. So, I see it as very unlikely that we'd get this doubling of methods you just assume. But then, lastly: Even if we would get it, so what? What's the harm of having those methods? And why would we need a TemplateExec type (which feels strongly like a Java-ism), instead of making it a package-scoped function?

So, no, I honestly don't see that.

@Merovius My context.Context example is a relevant one (there's an existing proposal floating around) and I don't see how any of your proposed workarounds working/be any good (I could start a discussion about that, but that would derail this proposal).

The thing is, state doesn't vanish just because you add it as a function argument (e.g. ExecuteFuncs(funcs)):

1. Someone has to maintain that state, creating that func map is not a per request operation.
2. And as seen by the CL in question, doing costly conversion of (I presume) the funcs from interface{} to reflect.Value may end up killing the performance you tried to fix in the first place.

I feel like every proposal about improving something in any of these template packages end up with making the least amount of change to get something working, often ending up with yet another unforeseen data race etc.

And why would we need a TemplateExec type (which feels strongly like a Java-ism), instead of making it a package-scoped function?

Se my "the state doesn't vanish" argument above. I'm not sure how global state would somehow be better than a struct for this (again, see my comment about introducing unforeseen data races above).

there's an existing proposal floating around

Do you mean #31107? Because that is closed, ultimately in favor of this one, AIUI.

I could start a discussion about that, but that would derail this proposal.

I agree, for what it's worth. That's why I said that we shouldn't complicate this proposal by assuming we'd also add context.Context at some point. Like, even if you think we should, taking it as a given that we will seems very questionable.

I agree, for what it's worth. That's why I said that we shouldn't complicate this proposal by assuming we'd also add context.Context at some point

Sure, but we should take into account that for every similar API in Go's stdlib (that does "per request operations", e.g. http.Client) will eventually need to be expanded/adjusted to fit new requirements/fix bugs, so going in a direction where this gets harder and harder to do, is not ideal. Which I still think my contex.Context serves good as an example (also, a closed issue is not the same as "it will. never happen").

Background: One of the things Hugo can do is to make an HTTP requests in a template. It wouldn’t make sense in a normal HTTP server app, but it makes sense for an application where the user controls templates but not the handlers. If a user makes a file change and then another one, the first request needs to be cancelled.

If I were going to do ExecuteContext (big if!) I would make that one new function take both the context and the funcs.

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

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