-
Notifications
You must be signed in to change notification settings - Fork 596
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
fix: be more graceful when trying to access the value of an invalid expression template #665
Conversation
…xpression template
…xpression template for templates with a prefix
if part == nil { | ||
// we silently ignore nil parts as they only occur for configuration that is already known to be invalid | ||
continue | ||
} |
There was a problem hiding this comment.
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:
-
Add a new type
hclsyntax.ExprSyntaxError
which implementshclsyntax.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)
. -
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 ofnil
, and have the parser copy its syntax-related diagnostics intoParseDiags
and generate some suitable placeholder value, which could becty.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. 🤔
There was a problem hiding this comment.
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
closing this in favor of #668 |
The editor extension needs to work with the available AST even if there were diagnostics / errors.
While typing out namespaced functions (e.g. during a partial
provider::
) this will produce expressions that arenil
in various places.This PR makes template expressions behave more graceful in such cases.