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.

Sign up for GitHub

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
Merged

Fix: Detect missing errors for } braces before else in let...else statements #18908

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
11717ca
Missing errors
jnyfah Jan 10, 2025
e3deeec
update blocklike
jnyfah Jan 16, 2025
2ac6cb1
revert blocklike changes
jnyfah Jan 16, 2025
b96819c
added tests
jnyfah Jan 16, 2025
31c07e4
CRLF to LF
jnyfah Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions crates/parser/src/grammar/expressions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

p.error(
"right curly brace `}` before `else` in a `let...else` statement not allowed",
)
if let Some(token) = expr.last_token(p) {
if token == T!['}'] {
p.error(
"right curly brace `}` before `else` in a `let...else` statement not allowed"
)
}
}
}

Expand Down
27 changes: 20 additions & 7 deletions crates/parser/src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,8 @@ impl Marker {
_ => unreachable!(),
}
p.push_event(Event::Finish);
CompletedMarker::new(self.pos, kind)
let end_pos = p.events.len() as u32;
CompletedMarker::new(self.pos, end_pos, kind)
}

/// Abandons the syntax tree node. All its children
Expand All @@ -336,13 +337,14 @@ impl Marker {
}

pub(crate) struct CompletedMarker {
pos: u32,
start_pos: u32,
end_pos: u32,
kind: SyntaxKind,
}

impl CompletedMarker {
fn new(pos: u32, kind: SyntaxKind) -> Self {
CompletedMarker { pos, kind }
fn new(start_pos: u32, end_pos: u32, kind: SyntaxKind) -> Self {
CompletedMarker { start_pos, end_pos, kind }
}

/// This method allows to create a new node which starts
Expand All @@ -360,10 +362,10 @@ impl CompletedMarker {
/// distance to `NEWSTART` into forward_parent(=2 in this case);
pub(crate) fn precede(self, p: &mut Parser<'_>) -> Marker {
let new_pos = p.start();
let idx = self.pos as usize;
let idx = self.start_pos as usize;
match &mut p.events[idx] {
Event::Start { forward_parent, .. } => {
*forward_parent = Some(new_pos.pos - self.pos);
*forward_parent = Some(new_pos.pos - self.start_pos);
}
_ => unreachable!(),
}
Expand All @@ -376,7 +378,7 @@ impl CompletedMarker {
let idx = m.pos as usize;
match &mut p.events[idx] {
Event::Start { forward_parent, .. } => {
*forward_parent = Some(self.pos - m.pos);
*forward_parent = Some(self.start_pos - m.pos);
}
_ => unreachable!(),
}
Expand All @@ -386,4 +388,15 @@ impl CompletedMarker {
pub(crate) fn kind(&self) -> SyntaxKind {
self.kind
}

pub(crate) fn last_token(&self, p: &Parser<'_>) -> Option<SyntaxKind> {
let end_pos = self.end_pos as usize;
if end_pos > p.events.len() {
return None;
}
jnyfah marked this conversation as resolved.
Show resolved Hide resolved
p.events[..end_pos].iter().rev().find_map(|event| match event {
jnyfah marked this conversation as resolved.
Show resolved Hide resolved
Event::Token { kind, .. } => Some(*kind),
_ => None,
})
}
}
Loading