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: Detect missing errors for } braces before else in let...else statements #18908

Merged
merged 5 commits into from
Jan 17, 2025

Conversation

jnyfah
Copy link
Contributor

@jnyfah jnyfah commented Jan 10, 2025

fixes #18345 - rust-analyzer fails to emit errors for let...else statements when the initializer expression ends in a block-like construct

  • Updated CompletedMarker to store the position where the node was completed.
  • Introduced a new last_token function that walks back through the parsing events from the completion position to find the last emitted token.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 10, 2025
@jnyfah jnyfah marked this pull request as ready for review January 10, 2025 13:43
@@ -134,10 +134,12 @@ pub(super) fn let_stmt(p: &mut Parser<'_>, with_semi: Semicolon) {
// test_err let_else_right_curly_brace
// fn func() { let Some(_) = {Some(1)} else { panic!("h") };}
if let Some(expr) = expr_after_eq {
if BlockLike::is_blocklike(expr.kind()) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should actually change how BlockLike works, it should be checking the syntax kind and the last token (for the kinds where the node is conditionally a block). That way we likely fix a bunch of bugs in other places where BlockLike is used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checking the last token in BlockLike breaks some closure parsing because closures with block bodies will being considered block-like
eg: fn main() { || -> i32 { 92 }(); } the closure is considered blocklike even though the { 92 } part is the body of the closure, not a standalone block expression.

one way to handle this tho would be explicitly excluding CLOSURE_EXPR when checking for trailing braces

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, let's keep it as is for now then actually. There might be other problems like that then.

crates/parser/src/parser.rs Outdated Show resolved Hide resolved
crates/parser/src/parser.rs Show resolved Hide resolved
@jnyfah jnyfah requested a review from Veykril January 16, 2025 11:29
Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

Ah, forgot to mention something, adding more tests would be nice :) (the fixture posted in the issue ideally). Just throw that in https://github.com/rust-lang/rust-analyzer/tree/master/crates/parser/test_data/parser/ok as a new .rs file, then running the tests with env var UPDATE_EXPECT=1 set should generate the corresponding syntax tree file

@jnyfah
Copy link
Contributor Author

jnyfah commented Jan 16, 2025

Ah, forgot to mention something, adding more tests would be nice :) (the fixture posted in the issue ideally). Just throw that in https://github.com/rust-lang/rust-analyzer/tree/master/crates/parser/test_data/parser/ok as a new .rs file, then running the tests with env var UPDATE_EXPECT=1 set should generate the corresponding syntax tree file

@Veykril All test passed apart from macros probably because of how macros are being parsed or expanded? not so sure

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

Thanks!

@Veykril Veykril added this pull request to the merge queue Jan 17, 2025
Merged via the queue into rust-lang:master with commit cd199ea Jan 17, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing errors for } braces before else in let...else statement
3 participants