Skip to content

proposal: text/template: safe navigation with ?. #43608

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

Open
thernstig opened this issue Jan 9, 2021 · 19 comments
Open

proposal: text/template: safe navigation with ?. #43608

thernstig opened this issue Jan 9, 2021 · 19 comments
Labels
Milestone

Comments

@thernstig
Copy link

It would be beautiful if the package text/template (used by e.g. Helm) could support optional chaining. Similar to JavaScript in latest standard ECMAScript 2020. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining

It would basically look like this:

spec:
  type: {{ .Values.service?.type | default "ClusterIP" }}

If service is not defined, it would short-circuit to undefined/false and thus default to "ClusterIP".

This could be used in other cases, such as:

spec:
  {{- if .Values.global?.internalIPFamily }}
  ipFamily: {{ .Values.global.internalIPFamily }}
  {{- end }}

It solves a few things in e.g. Helm that uses text/template for templating:

  1. default not working when parent values does not exist in a chain, giving errors, see nil pointer evaluating interface when upper level doesn't exist prevents usage of default function helm/helm#8026.
  2. It would be able to get rid of the, in my opinion, not great best practice for nested or flat values for e.g. Helm (which I assume is applicable to other text/template usages as well), see https://helm.sh/docs/chart_best_practices/values/#flat-or-nested-values. Optional chaining would instead still make the code more readable (one-liner) not caring about value depth. It would also in my opinion make it easier to read YAML files with the nested structure, clearly separating levels, than to use camelCase.

As I know very little about Go and the packge text/template, feel free to add more precise info on how this translates to what needs to be done in text/template.

@cagedmantis cagedmantis changed the title Feature: Support for optional chaining in package text/template test/template: add support for optional chaining Jan 11, 2021
@cagedmantis cagedmantis changed the title test/template: add support for optional chaining text/template: add support for optional chaining Jan 11, 2021
@cagedmantis cagedmantis added this to the Backlog milestone Jan 13, 2021
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 13, 2021
@cagedmantis
Copy link
Contributor

This seems more like a proposal. I'm adding the text/template owners in order to begin the discussion.

/cc @robpike @mvdan

@bilak
Copy link

bilak commented Mar 18, 2021

@cagedmantis @robphoenix @mvdan please any update on this task? It would be super helpful if we can use this style of property resolver.

@thernstig
Copy link
Author

@bilak @cagedmantis @robphoenix @mvdan do you have some opinions on this?

@mvdan
Copy link
Member

mvdan commented Apr 27, 2021

There are dozens of open text/template issues. Please don't ping proposals every few weeks; we have limited time.

Personally, I think we should implement the already-accepted proposals (#20531 and #31103) before we consider new ones. Any help with those is welcome.

@thernstig
Copy link
Author

It was 5 months ago this was written, and I find it common courtesy to give a short comment that it has been seen. But each repo to his own.

@emmaLP

This comment was marked as duplicate.

@ianlancetaylor
Copy link
Member

The way to make progress on this is to turn it into a proposal with a precise definition of the suggested change and the new semantics.

For example, if I pass

var s struct {
    p *struct {
        f int
    }
}

to a template and write s.p.f then assuming that s.p is not nil I will get an integer. What should happen for s.p.?f if p is nil? Should I get 0? If I get nil that may cause a different error.

The initial comment on the issue says "feel free to add more precise info on how this translates to what needs to be done in text/template" but that is hard for us to do. There are many many many more people suggesting changes than there are people implementing those changes. This can only scale if the people interested in the change can describe exactly what how it should work.

Thanks.

@thernstig
Copy link
Author

@ianlancetaylor I do not have the knowledge in Go to give specifics, but regarding semantics one can look at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining which can serve as the spec for how it should work.

But regarding s.p?.f it should return nil (since undefined does not exist in Go?).

Anyone more comfortable in Go; feel free to write up a proper proposal. I can link it in the original issue.

@ungerik
Copy link

ungerik commented Sep 26, 2022

There is no value in nil pointer panics and maybe not much code that depends on it to work properly. Because it just breaks things and doesn't fix anything, the nicest solution would be to make optional chaining the default.

Now I know we can't have nice things because of the remote chance that some code depends on the panics, backward compatibility and all, so the next best thing would be to make it a configuration option on template level.

@seankhliao seankhliao added the FeatureRequest Issues asking for a new feature that does not need a proposal. label Jul 13, 2024
@seankhliao seankhliao changed the title text/template: add support for optional chaining proposal: text/template: safe navigation with ?. Dec 4, 2024
@ianlancetaylor ianlancetaylor removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest Issues asking for a new feature that does not need a proposal. labels Dec 18, 2024
@ianlancetaylor ianlancetaylor modified the milestones: Backlog, Proposal Dec 18, 2024
@ianlancetaylor ianlancetaylor moved this to Incoming in Proposals Dec 18, 2024
@apparentlymart
Copy link

It seems like this proposal has got a little stuck because those who are proposing it don't have enough familiarity with the template language implementation to state concretely what they would like it to do, but those who do have that knowledge are busy with other things and so unable to try to "meet the proposers in the middle" with some context to work on.

I'm admittedly not incredibly invested in this proposal since I don't really use Go's template language in any software I work on, but I do know a little about how it works and so I'm sharing the following in the hope that it gives others some hooks to pull on to make this proposal more concrete.


An important starting piece of context for the template language is that all values that the template interacts with are represented as reflect.Value, which is more or less the reflection system's equivalent of any1.

This proposal seems to be focused on the specific detail of what the template language documentation refers to as a "field invocation":

  • The name of a field of the data, which must be a struct, preceded by a period, such as .Field The result is the value of the field. Field invocations may be chained: .Field1.Field2 Fields can also be evaluated on variables, including chaining: $x.Field1.Field2

I would recommend that those iterating on this proposal also consider how the new proposed behavior might change or take inspiration from the similar syntax for accessing "a key of the data" (from a map), since that also uses the . syntax but with a slightly different meaning:

  • The name of a key of the data, which must be a map, preceded by a period, such as .Key The result is the map element value indexed by the key. Key invocations may be chained and combined with fields to any depth: .Field1.Key1.Field2.Key2 Although the key must be an alphanumeric identifier, unlike with field names they do not need to start with an upper case letter. Keys can also be evaluated on variables, including chaining: $x.key1.key2

The code that handles this kind of traversal today -- including the rule of returning an error if you try to traverse through a nil pointer -- is here:

// evalField evaluates an expression like (.Field) or (.Field arg1 arg2).
// The 'final' argument represents the return value from the preceding
// value of the pipeline, if any.
func (s *state) evalField(dot reflect.Value, fieldName string, node parse.Node, args []parse.Node, final, receiver reflect.Value) reflect.Value {
if !receiver.IsValid() {
if s.tmpl.option.missingKey == mapError { // Treat invalid value as missing map key.
s.errorf("nil data; no entry for key %q", fieldName)
}
return zero
}
typ := receiver.Type()
receiver, isNil := indirect(receiver)
if receiver.Kind() == reflect.Interface && isNil {
// Calling a method on a nil interface can't work. The
// MethodByName method call below would panic.
s.errorf("nil pointer evaluating %s.%s", typ, fieldName)
return zero
}
// Unless it's an interface, need to get to a value of type *T to guarantee
// we see all methods of T and *T.
ptr := receiver
if ptr.Kind() != reflect.Interface && ptr.Kind() != reflect.Pointer && ptr.CanAddr() {
ptr = ptr.Addr()
}
if method := ptr.MethodByName(fieldName); method.IsValid() {
return s.evalCall(dot, method, false, node, fieldName, args, final)
}
hasArgs := len(args) > 1 || !isMissing(final)
// It's not a method; must be a field of a struct or an element of a map.
switch receiver.Kind() {
case reflect.Struct:
tField, ok := receiver.Type().FieldByName(fieldName)
if ok {
field, err := receiver.FieldByIndexErr(tField.Index)
if !tField.IsExported() {
s.errorf("%s is an unexported field of struct type %s", fieldName, typ)
}
if err != nil {
s.errorf("%v", err)
}
// If it's a function, we must call it.
if hasArgs {
s.errorf("%s has arguments but cannot be invoked as function", fieldName)
}
return field
}
case reflect.Map:
// If it's a map, attempt to use the field name as a key.
nameVal := reflect.ValueOf(fieldName)
if nameVal.Type().AssignableTo(receiver.Type().Key()) {
if hasArgs {
s.errorf("%s is not a method but has arguments", fieldName)
}
result := receiver.MapIndex(nameVal)
if !result.IsValid() {
switch s.tmpl.option.missingKey {
case mapInvalid:
// Just use the invalid value.
case mapZeroValue:
result = reflect.Zero(receiver.Type().Elem())
case mapError:
s.errorf("map has no entry for key %q", fieldName)
}
}
return result
}
case reflect.Pointer:
etyp := receiver.Type().Elem()
if etyp.Kind() == reflect.Struct {
if _, ok := etyp.FieldByName(fieldName); !ok {
// If there's no such field, say "can't evaluate"
// instead of "nil pointer evaluating".
break
}
}
if isNil {
s.errorf("nil pointer evaluating %s.%s", typ, fieldName)
}
}
s.errorf("can't evaluate field %s in type %s", fieldName, typ)
panic("not reached")
}

In particular I think y'all are interested in this particular case:

case reflect.Pointer:
etyp := receiver.Type().Elem()
if etyp.Kind() == reflect.Struct {
if _, ok := etyp.FieldByName(fieldName); !ok {
// If there's no such field, say "can't evaluate"
// instead of "nil pointer evaluating".
break
}
}
if isNil {
s.errorf("nil pointer evaluating %s.%s", typ, fieldName)
}
}

The key question that this proposal needs to answer, I think, is what replaces the s.errorf call in that if isNil { ... } branch. s.errorf causes a panic, meaning that currently that function doesn't return in the regular way at all when that situation occurs. This proposal suggests that it should instead return normally, and so the important question is exactly which reflect.Value it ought to return.

When considering the answer to that, it's important to recognize that the identifier nil in the main Go language does not actually represent a single value of a particular type. Instead, it's a special typeless thing that can convert into a value of a subset of Go's types, and can be compared to a subset of Go's types. So there is no single reflect.Value that represents what nil would represent in a Go program.

What we do have is the concept of the "zero value" of each type. For a number of different types, including pointers, the zero value is considered to be equal to nil, and so one possible answer is to say that the return value would be the zero value of whatever type the named field was declared with, assuming that there is a field with that name. This would be similar to the treatment of accessing a non-existent key in a map when the missingkey=zero option is set, although it would presumably still need to fail if the given field name doesn't exist in the struct type at all, or if the pointer is not of a struct type, because then there would be no field to use to decide the result type.

However, the motivation for this issue linked to a downstream Helm issue involving its default function. As far as I can tell, that default function is actually from github.com/Masterminds/sprig, and has its own special idea of "empty" which (despite what the documentation says) doesn't seem to exactly match "the zero value of the value's type". However, the only difference that stuck out to me is that it never considers a struct-typed value to be empty, regardless of its value, and so it seems like it would work fine for a primitive-typed field like the .Values.service?.type in the first motivating example.

That's all the digging and writing I have time for right now. I hope the above is a useful starting point for someone who wants to develop this from a feature request into a proposal.

Footnotes

  1. Things are a little more subtle than that since the reflection system can represent some details that any cannot, but I think this statement is true enough for the sake of the rest of this comment.

@balmeida-nokia
Copy link

balmeida-nokia commented Dec 19, 2024

Thank you apparentlymart. That is a great summary. You do tell:

the identifier nil in the main Go language does not actually represent a single value of a particular type

Do understand that from my perspective (which hopefully overlaps with the other ones here) is that it's never a problem to have a nil coming out of a ?. sequence. However, if a nil ends up being the result of a {{...}}, then it makes sense to fail.

Ideally, we then either end the sequence with a default value we want to use if none is set or we want it to fail if the value isn't set. That's how people are bringing up the default function from sprig (which is a HUGE misnomer, from 2nd hand experience)

As a side-thought: I read in this sequence of messages that maybe the behavior described of ?. when using . makes most sense to be the behavior implemented. However, to do that, you need to keep extra status or stack information, for logging purposes, which mapping returned nil. For me, that's would be a requirement.

I don't see the need to have a zero for this purpose. If the value doesn't exist, it doesn't exist, it's nil which is a thing in go language itself.

Examples of how the behavior is with this proposal:
Doing a ?. in go template providing a nil always returns nil and never fails. E.g.
nil?.abc <- is nil
nil?.abc?.def <- is nil
exists.notexist <- is nil (and had always been nil)
exists.notexist?.abc <- is nil
exists.notexist.abc <- fails because cannot . operate on nil

I hope I complemented apparentlymart's comment with my own perspective.

@ianlancetaylor
Copy link
Member

Having ?. always produce nil is one way to go but it's not obviously correct. See my comment #43608 (comment). If the template is expecting an int and getting nil, then we have converted a nil-pointer crash into a type-mismatch crash. Is that what people want?

@balmeida-nokia
Copy link

In my perspective, you always get nil. Even if you are expecting an int because you already know the field is of type int. If there's nothing there, then it's nil. If there's a value which is the integer 0 there, then it's the integer 0.

Am I making sense?

@ianlancetaylor
Copy link
Member

You are making sense, I'm just saying that you are trading one kind of crash for a different kind of crash. If the code is going to crash anyhow, what's the point of adding the ?. syntax?

@apparentlymart
Copy link

apparentlymart commented Dec 19, 2024

I want to reinforce my point that nil is not actually a value in Go.

func main() {
	v := nil // ERROR: use of untyped nil in assignment
	fmt.Println("It's ", v)
}

The above fails because nil alone doesn't give enough information to decide what type v will have.

nil assigned to *Foo is a distinct value from nil assigned to *Bar, or nil assigned to []Foo, or nil assigned to map[string]Foo. There is also no value of type string, or any of the numeric primitive types, or of a struct type, that is equal to nil or assignable from nil.

Therefore it isn't really meaningful to say that the template expression result "is nil". The result must be defined as some specific reflect.Value, which means the result must have a specified type and must be a value of that type. Since there is no value of int that is equal to nil, using a nil-like value as the result would imply the result being of some type other than int.


I didn't mention in my first comment that the template system does also include the idea of "no value", which is what access to a non-existing map key returns by default. Internally within the template engine, that's represented as the zero value of reflect.Value, which represents the absence of a value rather than a value1.

I see in the evalField function I shared earlier that accessing a field on "no value" returns another "no value":

if !receiver.IsValid() {
if s.tmpl.option.missingKey == mapError { // Treat invalid value as missing map key.
s.errorf("nil data; no entry for key %q", fieldName)
}
return zero
}

...and so I suppose another possible answer is to say that foo?.Bar returns "no value" (the zero value of reflect.Value) if foo is nil, and then any subsequent chained field accesses would presumably be handled by the branch I linked to above and would again return "no value".

Between the current behavior and the two possibilities I've discussed so far I've effectively covered all of the existing available options for how to handle accessing a map key that doesn't exist in the map:

  • missingkey=default/missingkey=invalid: returns "no value", like what I've just described above.
  • missingkey=zero: returns the zero value of the map's element type, similar to what I suggested in my original comment.
  • missingkey=error: fails with an error, matching the template engine's current treatment of field access through a nil pointer

If it's possible to do so, I expect that matching one of these already-available behaviors for accessing non-existent map keys as closely as possible is probably best so that the overall template language stays somewhat consistent.

(@ungerik also noted that there could be a new option for specifying how the template engine ought to treat field access through a nil pointer, similar to the existing option for missing map keys. That is true, but seems like it would be a materially different proposal since it would make this a global decision for the Go code that renders the template rather than a local decision made by the template author for just one field access.)

Footnotes

  1. I suppose, framed that way, the template engine treats the zero value of reflect.Value in a somewhat-similar way to how JavaScript treats undefined, so perhaps this is the more compelling answer after all.

@balmeida-nokia
Copy link

@ianlancetaylor
Because it becomes useful when you combine with other actions. For example:
E.g.1:

val: {{ a.b?.maybeexist | default 0 }}

E.g.2:

{{ if a.b?.maybeexist }}
set: accept
val: "yes"
{{- end -}}

Does this help?

@balmeida-nokia
Copy link

@apparentlymart I was thinking about nil as (interface{})(nil) when no other can be reasonably deducted. AFAIK, a nil like that does exist and is assignable. However, if there's already that no value which is the zero of reflect.Value, then that works just as well for my use-cases. That can then be used to handle as not being set and replace with a default, make it error or even use in a conditional to execute a custom conditional process based on whether it actually exists or not.

@apparentlymart
Copy link

Since I've only been working in theory so far, and that's dangerous 😀, I think probably a good next step would be to confirm whether this is viable by making a temporary edit to the function I've been linking to above so that it uses return zero instead of raising an error when the pointer is nil.

Although that behavior would not exactly match the proposal -- it would redefine the existing . operator rather than introducing a new ?. operator -- it would be enough to experiment with combining that behavior with other features of the language to make sure that it fits well with the existing features. In particular, it would be interesting to see how that downstream default function interacts with an expression that returns "no value".

If you want to experiment with it without temporarily modifying text/template then another option would be to try using an access to a non-existent map key in place of access through a nil pointer, since that would already return "no value" in the existing language. That could then let us see whether that approach is likely to be successful in actually meeting the use-cases that we've been discussing in this proposal.

@balmeida-nokia
Copy link

Yes. Good idea. I don't have a setup on my system to build go but if no one steps forward, I can shift some things around to try finding the time to do that (hopefully, I won't forget about it drowned in my work).

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

No branches or pull requests