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

add ExprSyntaxError #668

Merged
merged 4 commits into from
Mar 13, 2024
Merged

add ExprSyntaxError #668

merged 4 commits into from
Mar 13, 2024

Conversation

ansgarm
Copy link
Member

@ansgarm ansgarm commented Mar 12, 2024

As suggested by @apparentlymart in this review comment this PR introduces a ExprSyntaxError which is returned instead of nil when an invalid namespaced function is encountered during parsing. This makes it easier to walk / inspect the parse result even when it was invalid (as e.g. required in the Terraform Language server) without the need to check for nil expressions all over the place.

This PR supersedes #665

@ansgarm ansgarm changed the title add expr syntax error add ExprSyntaxError Mar 12, 2024
@ansgarm ansgarm requested a review from apparentlymart March 12, 2024 08:38
}

func (e *ExprSyntaxError) Range() hcl.Range {
return hcl.Range{}
Copy link
Member Author

@ansgarm ansgarm Mar 12, 2024

Choose a reason for hiding this comment

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

Is there a valid Range we could return here? Maybe from the diagnostic?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the simplest approach would be to add SourceRange as a field in ExprSyntaxError and then have the parser decide what range to "blame" for the error.

As you noted, the parser already ought to be picking source ranges to go into the diagnostics anyway, and so I think it shouldn't be a big hardship to choose one of those ranges to also write into that field, and then we can keep it explicit and thus hopefully the ranges are more likely to be useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good! Added SrcRange to that struct and set the same range as in the diagnostic 👍

@ansgarm ansgarm marked this pull request as ready for review March 13, 2024 08:19
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This looks good to me! 🎉

@ansgarm ansgarm merged commit 5160967 into main Mar 13, 2024
13 checks passed
@ansgarm ansgarm deleted the add-expr-syntax-error branch March 13, 2024 15:29
@ansgarm
Copy link
Member Author

ansgarm commented Mar 13, 2024

Thanks for the review! Went ahead and merged it.

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.

2 participants