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

proposal: text/template, html/template: add LookupFunc, LookupOption methods #43062

Closed
jba opened this issue Dec 8, 2020 · 11 comments
Closed

Comments

@jba
Copy link
Contributor

jba commented Dec 8, 2020

It is possible using only exported symbols to write a type-checker for templates. There is just one missing piece: to check a function call like {{len .}}, the checker needs the function signature, but there is no way to retrieve that from a Template.

To work around this, I copied the information for built-ins from text/template, and I must ask the user to pass to my checker whatever FuncMaps they used when parsing the template. The first solution is brittle and the second unpleasant.

All that is missing is the method Template.LookupFunc(name string) interface{}. It would behave much like the unexported findFunction function in text/template/funcs.go, but it would return the unreflected function value or nil if the name was undefined.

I propose it be added to both text/template.Template and html/template.Template.

@jba jba added the Proposal label Dec 8, 2020
@gopherbot gopherbot added this to the Proposal milestone Dec 8, 2020
@jba jba changed the title proposal: {text/html}/template: add LookupFunc method proposal: {text/html}/template: add LookupFunc, LookupOption methods Dec 8, 2020
@jba
Copy link
Contributor Author

jba commented Dec 8, 2020

Actually, there is another bit of info that needs to be exposed: the options set with Template.Option. A LookupOption(name string) string on Template would also be useful.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Dec 8, 2020
@pjweinb
Copy link

pjweinb commented Dec 9, 2020

LookupFunc would be convenient (although for user functions it can be done with reflection). However, the types of the built-in functions are unavailable.

@rsc rsc changed the title proposal: {text/html}/template: add LookupFunc, LookupOption methods proposal: text/template, html/template: add LookupFunc, LookupOption methods Dec 9, 2020
@rsc
Copy link
Contributor

rsc commented Dec 9, 2020

I'm not entirely sure these are general enough to warrant the complexity they would require us to support.
I'm also not entirely sure that the functions can't be changed after type-checking and before execution.

It seems like any kind of type-checking will stop being able to do anything as soon as there is an interface{}, right?

@rsc rsc moved this from Incoming to Active in Proposals (old) Dec 9, 2020
@pjweinb
Copy link

pjweinb commented Dec 9, 2020

I'd settle for making the built-in template functions visible. I don't understand the comment about interface{}. If the call is t.Execute(x), then t.Check(xx) ought to work fine as long as reflect.TypeOf(xx) == reflect.Type(x). Type checking just needs to check that the fields and methods used in the template exist on the argument.

My use case is in the debug server started by gopls. It stopped working when some underlying data structures were refactored. There's no way to detect that in unit tests presently, but t.Check() could go in debug_test.go, and would discover that the templates could no longer execute because of type errors.

@jba
Copy link
Contributor Author

jba commented Dec 9, 2020

It seems like any kind of type-checking will stop being able to do anything as soon as there is an interface{}, right?

I think this means that if you pass a value of

type homePage struct {
    Name string
    Details interface{}
}

to the type-checker, then You can check {{.Name}} but not {{.Details.Foo}}.
That's correct, and in fact that happens in the pkgsite templates. But in that case we usually call an associated template:

{{if ....}}
    {{template "versions" .Details}}
{{end}}

and "versions" does assume a specific type. So I just call Check on t.Lookup("versions") with that type.

@jba
Copy link
Contributor Author

jba commented Dec 9, 2020

It is possible to get user-defined func maps via reflection, and I've made that change. But it makes the code depend on implementation details. For instance, for html/template I need reflect.ValueOf(*t).FieldByName("text").Elem().FieldByName("parseFuncs").

I'm also not entirely sure that the functions can't be changed after type-checking and before execution.

They can. You can also pass a different dot type to Execute than the one you pass to Check. I'm not sure I understand your point. I think you're arguing that this sort of checker isn't very useful because it can be fooled, but in practice one would write tests to avoid that as much as possible—for instance, by defining the FuncMap in a global and referencing it in the tests.

@rsc
Copy link
Contributor

rsc commented Dec 16, 2020

The difficult part about this issue is that committing to public API is forever, and we don't really know whether we want to commit to this. But I also don't want to stand in the way of you building a useful tool and exploring the space of possible changes. For example it might be that the right long-term API is not LookupFunc and LookupOption but instead adding your Check to the template package, where it will have access to internals. I don't know.

It sounds like if you've got a way to not be blocked, by using reflection, that might be the right answer for now, and then we don't have to decide whether to add LookupFunc and LookupOption more generally.

@jba
Copy link
Contributor Author

jba commented Dec 16, 2020

I'm not blocked by this issue.

@jba
Copy link
Contributor Author

jba commented Dec 22, 2020

adding your Check to the template package

Should I file a separate proposal for that, or co-opt this one?

@rsc
Copy link
Contributor

rsc commented Jan 6, 2021

Please file a different proposal once you have some experience using it and think all the wrinkles are ironed out. Thanks!

Will treat this one as retracted.

@rsc rsc closed this as completed Jan 6, 2021
@rsc rsc moved this from Active to Declined in Proposals (old) Jan 6, 2021
@rsc
Copy link
Contributor

rsc commented Jan 6, 2021

No change in consensus, so declined.
— rsc for the proposal review group

@golang golang locked and limited conversation to collaborators Jan 6, 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

4 participants