Skip to content

Conversation

@pgavlin
Copy link
Member

@pgavlin pgavlin commented Oct 23, 2025

We must tolerate syntax errors when loading an environment. Returning a
nil EnvironmentDecl prevents us from being able to analyze the
portions of the environment that were successfully parsed. This
interferes with analysis of environments that are only partially
erroneous. One important scenario this covers is autocompletion in the
face of syntax errors, which is common while editing an environment.

@pgavlin pgavlin requested a review from nyobe October 23, 2025 22:13
We must tolerate syntax errors when loading an environment. Returning a
nil `EnvironmentDecl` prevents us from being able to analyze the
portions of the environment that were successfully parsed. This
interferes with analysis of environments that are only partially
erroneous. One important scenario this covers is autocompletion in the
face of syntax errors, which is common while editing an environment.
@pgavlin pgavlin force-pushed the pgavlin/load-syntax branch from 2be78f3 to ae1b32c Compare October 23, 2025 22:23
Comment on lines -73 to -76
if tdiags.HasErrors() {
return nil, diags, nil
}

Copy link
Member Author

Choose a reason for hiding this comment

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

this is the core of the fix. the other edits in this PR are changes to accommodate nil object keys. I don't think we have other places that need nil checks, as everything else goes through declare, and declare will produce missingExpr values for nil AST nodes.

@pgavlin pgavlin requested a review from a team October 23, 2025 22:29
@pgavlin
Copy link
Member Author

pgavlin commented Oct 24, 2025

Validated on a dev stack that this does fix autocomplete in the ESC console.

Copy link
Member

@fnune fnune left a comment

Choose a reason for hiding this comment

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

Looks simple enough and the expected.json results help. But as an ESC rookie I would like to know more broadly what features this affects. Is there anything beyond autocomplete?

ACK that you tested autocomplete on the review stack, I'd ask for (1) a link so that I can verify, (2) maybe a photo to show exactly what you're referring to.

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.

3 participants