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.

Sign up for GitHub

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

fix: be more graceful when trying to access the value of an invalid expression template #665

Closed
wants to merge 2 commits into from
Closed

fix: be more graceful when trying to access the value of an invalid expression template #665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2adeb39
fix: be more graceful when trying to access the value of an invalid e…
ansgarm Mar 1, 2024
4763cd0
fix: be more graceful when trying to access the value of an invalid e…
ansgarm Mar 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions hclsyntax/expression_template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ func (e *TemplateExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics)
marks := make(cty.ValueMarks)

for _, part := range e.Parts {
if part == nil {
// we silently ignore nil parts as they only occur for configuration that is already known to be invalid
continue
}
Comment on lines +36 to +39
Copy link
Contributor

@apparentlymart apparentlymart Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me a little nervous because it creates a situation where this function produces an incomplete result without indicating any errors, which means that an incorrect result could "leak out" into another system and make it fail in a confusing way.

As far as I remember we haven't yet had a situation where the evaluator needs to directly respond to something that was already found to be invalid during the parsing phase, but I had a similar problem in another project and solved it in a slightly different way that perhaps we could emulate here:

  1. Add a new type hclsyntax.ExprSyntaxError which implements hclsyntax.Expression and is defined something like this:

    type ExprSyntaxError struct {
    Placeholder cty.Value
    ParseDiags  hcl.Diagnostics
}

    The Value implementation of this would just immediately return (Placeholder, ParseDiags).

  2. Change the parser so that in situations like this where expression parsing fails so badly that we can't generate any expression node at all we insert an *ExprSyntaxError node instead of nil, and have the parser copy its syntax-related diagnostics into ParseDiags and generate some suitable placeholder value, which could be cty.DynamicVal as a last resort when the parser doesn't have enough information to insert anything more specific.

    (In some cases we might have enough information to return a better-typed unknown value, but that isn't true here since an invalid function call could've been to any function and therefore could return a value of any type.)

The effect then would be that if a syntax error during parsing causes us to be unable to produce an evaluable expression then we'll arrange so that any attempt to evaluate that incomplete result will just return the same parse error again, including all of the same details the parser would've reported earlier.

This should then allow us to produce incomplete results that don't require so much care when handling, and avoid having to add explicit nil checks all over the rest of this package to deal with that special situation.

Do you think that seems plausible as an answer here? I wonder if we can find other situations in the parser where we're currently returning nil for an expression parse error that could be treated in the same way we add this new expression node type. 🤔

Copy link
Member Author

@ansgarm ansgarm Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me a little nervous because it creates a situation where this function produces an incomplete result without indicating any errors

I agree! And I really like your suggestion of introducing a new type hclsyntax.ExprSyntaxError – especially since it feels like there are a ton of places that now have nil in expressions where they weren't formerly expected (not just in this package, also in terraform-schema, hcl-lang).

Do you think that seems plausible as an answer here?

Yes! I'll give implementing this ExprSyntaxError a try and will loop you into the new PR 👍

edit: PR


partVal, partDiags := part.Value(ctx)
diags = append(diags, partDiags...)

Expand Down Expand Up @@ -250,6 +255,10 @@ func (e *TemplateWrapExpr) walkChildNodes(w internalWalkFunc) {
}

func (e *TemplateWrapExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnostics) {
if e.Wrapped == nil {
return cty.NilVal, nil
}

return e.Wrapped.Value(ctx)
}

Expand Down
22 changes: 22 additions & 0 deletions hclsyntax/expression_template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,28 @@ trim`,

}

func TestTemplateExprGracefulValue(t *testing.T) {
// we don't care about diags since we know it's invalid config
expr, _ := ParseTemplate([]byte(`prefix${provider::}`), "", hcl.Pos{Line: 1, Column: 1, Byte: 0})

got, _ := expr.Value(nil) // this should not panic

if !got.RawEquals(cty.StringVal("prefix")) {
t.Errorf("wrong result\ngot: %#v\nwant: %#v", got, cty.NilVal)
}
}

func TestTemplateExprWrappedGracefulValue(t *testing.T) {
// we don't care about diags since we know it's invalid config
expr, _ := ParseTemplate([]byte(`${provider::}`), "", hcl.Pos{Line: 1, Column: 1, Byte: 0})

got, _ := expr.Value(nil) // this should not panic

if !got.RawEquals(cty.NilVal) {
t.Errorf("wrong result\ngot: %#v\nwant: %#v", got, cty.NilVal)
}
}

func TestTemplateExprIsStringLiteral(t *testing.T) {
tests := map[string]bool{
// A simple string value is a string literal
Expand Down
Loading