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

[naga wgsl-in] Fill out error message for deep nesting. #5486

Closed

Conversation

jimblandy
Copy link
Member

In the WGSL front end, clarify error message for deeply nested statements.

@ErichDonGubler I was going to push this as a suggestion to the PR but forgot to. Only take it if you like it.

In the WGSL front end, clarify error message for deeply nested
statements.
@jimblandy jimblandy requested a review from a team as a code owner April 3, 2024 19:58
Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

I'm currently working on a few of our Firefox timeline items right now, so I won't be prioritizing this for the next couple of days, but I definitely intend to get back to this!

I'm actually surprised that you wanted to tweak this. Could you help me understand what your goals for revising were?

@@ -775,10 +775,10 @@ impl<'a> Error<'a> {
]
},
Error::ExceededLimitForNestedBraces { span, limit } => ParseError {
message: "brace nesting limit reached".into(),
labels: vec![(span, "limit reached at this brace".into())],
message: "Statements nested too deeply".into(),
Copy link
Member

Choose a reason for hiding this comment

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

issue: The capitalized casing, for better or worse, is not consistent with other diagnostics.

Suggested change
message: "Statements nested too deeply".into(),
message: "statements nested too deeply".into(),

Copy link
Member

Choose a reason for hiding this comment

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

This feedback also applies to the notes field.

notes: vec![
format!("nesting limit is currently set to {limit}"),
format!("The maximum nesting depth of brace-enclosed statements in a function is {limit}."),
Copy link
Member

@ErichDonGubler ErichDonGubler Apr 4, 2024

Choose a reason for hiding this comment

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

issue: This new diagnostic takes pains to note that it is related to statements, but that's not true, AFAIK. The limit current works based on opening braces in any parsing rule (i.e., incl. loop, as you pointed out in the initial PR). Therefore, I think attributing this to statements is misleading.

Copy link
Member

@ErichDonGubler ErichDonGubler left a comment

Choose a reason for hiding this comment

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

oderp, wrong button 😖

@cwfitzgerald
Copy link
Member

Boop from triage

@jimblandy
Copy link
Member Author

I don't want to change this any more.

@jimblandy jimblandy closed this Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants