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

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

Closed
wants to merge 2 commits into from

Conversation

ansgarm
Copy link
Member

@ansgarm ansgarm commented Mar 1, 2024

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 are nil in various places.
This PR makes template expressions behave more graceful in such cases.

@ansgarm ansgarm marked this pull request as ready for review March 4, 2024 14:26
Comment on lines +36 to +39
if part == nil {
// we silently ignore nil parts as they only occur for configuration that is already known to be invalid
continue
}
Copy link
Member

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

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

@ansgarm ansgarm marked this pull request as draft March 5, 2024 09:09
ansgarm added a commit that referenced this pull request Mar 12, 2024
@ansgarm ansgarm mentioned this pull request Mar 12, 2024
@ansgarm
Copy link
Member Author

ansgarm commented Mar 12, 2024

closing this in favor of #668

@ansgarm ansgarm closed this Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants