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 diagnostic coverage, remove possibly-unreachable unary op diagnostic #4519

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Nov 12, 2024

I'm working to make sure remaining diagnostics have coverage, at least the ones I'd previously added a TODO for. Note in particular that I couldn't figure out a repro for UnaryOperatorRequiresWhitespace; if you have one, I can add a test, but otherwise maybe it's actually unreachable due to being diagnosed through infix logic (or, maybe this'll let fuzzing tell me an example).

Comment on lines 306 to 308
} else {
CARBON_CHECK(!IsLexicallyValidInfixOperator(),
"whitespace should have been required for an infix parse");
Copy link
Contributor

Choose a reason for hiding this comment

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

I spent some time digging into this. It's almost not possible to reach this diagnostic, because almost every single one of our symbolic prefix or postfix operators happens to also be valid as an infix operator -- and so if a use were lexically valid as an infix operator, it would have been parsed as one. The exception is that the -- and ++ operators have no infix form. And even though they can only appear at the start of a statement, it is possible to reach this diagnostic for them.

Here's a testcase that produces the "whitespace is required before this unary operator" diagnostic:

fn F() { var n: i32 = 0; if (false) {}--n; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I've added a parse test, with some minimization.

Note, I also added a similar test that doesn't repro, but produces a very similar parse tree so probably should, but leaving it as a todo.

@zygoloid zygoloid added this pull request to the merge queue Nov 13, 2024
Merged via the queue into carbon-language:trunk with commit fa95892 Nov 13, 2024
8 checks passed
@jonmeow jonmeow deleted the diag-cov branch November 13, 2024 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants