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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion crates/parser/src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::{
/// `Parser` produces a flat list of `Event`s.
/// They are converted to a tree-structure in
/// a separate pass, via `TreeBuilder`.
#[derive(Debug)]
#[derive(Debug, PartialEq)]
pub(crate) enum Event {
/// This event signifies the start of the node.
/// It should be either abandoned (in which case the
Expand Down
10 changes: 6 additions & 4 deletions crates/parser/src/grammar/expressions.rs
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
25 changes: 18 additions & 7 deletions crates/parser/src/parser.rs
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,13 @@ 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;
debug_assert_eq!(p.events[end_pos - 1], Event::Finish);
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,
})
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
SOURCE_FILE
STRUCT
STRUCT_KW "struct"
WHITESPACE " "
NAME
IDENT "X"
WHITESPACE " "
RECORD_FIELD_LIST
L_CURLY "{"
RECORD_FIELD
NAME
IDENT "a"
COLON ":"
WHITESPACE " "
PATH_TYPE
PATH
PATH_SEGMENT
NAME_REF
IDENT "i32"
R_CURLY "}"
WHITESPACE "\n"
FN
FN_KW "fn"
WHITESPACE " "
NAME
IDENT "f"
PARAM_LIST
L_PAREN "("
R_PAREN ")"
WHITESPACE " "
BLOCK_EXPR
STMT_LIST
L_CURLY "{"
WHITESPACE "\n "
LET_STMT
LET_KW "let"
WHITESPACE " "
IDENT_PAT
NAME
IDENT "foo"
WHITESPACE " "
EQ "="
WHITESPACE " "
RECORD_EXPR
PATH
PATH_SEGMENT
NAME_REF
IDENT "X"
WHITESPACE " "
RECORD_EXPR_FIELD_LIST
L_CURLY "{"
WHITESPACE "\n "
RECORD_EXPR_FIELD
NAME_REF
IDENT "a"
COLON ":"
WHITESPACE " "
LITERAL
INT_NUMBER "1"
WHITESPACE "\n "
R_CURLY "}"
WHITESPACE " "
LET_ELSE
ELSE_KW "else"
WHITESPACE " "
BLOCK_EXPR
STMT_LIST
L_CURLY "{"
WHITESPACE "\n "
EXPR_STMT
RETURN_EXPR
RETURN_KW "return"
SEMICOLON ";"
WHITESPACE "\n "
R_CURLY "}"
SEMICOLON ";"
WHITESPACE "\n"
R_CURLY "}"
error 63: right curly brace `}` before `else` in a `let...else` statement not allowed
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
struct X {a: i32}
fn f() {
let foo = X {
a: 1
} else {
return;
};
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
SOURCE_FILE
ERROR
LET_KW "let"
WHITESPACE " "
IDENT_PAT
NAME
IDENT "foo"
WHITESPACE " "
EQ "="
WHITESPACE " "
BIN_EXPR
LITERAL
INT_NUMBER "1"
WHITESPACE " "
PLUS "+"
WHITESPACE " "
BLOCK_EXPR
STMT_LIST
L_CURLY "{"
WHITESPACE "\n "
LITERAL
INT_NUMBER "1"
WHITESPACE "\n"
R_CURLY "}"
WHITESPACE " "
LET_ELSE
ELSE_KW "else"
WHITESPACE " "
BLOCK_EXPR
STMT_LIST
L_CURLY "{"
WHITESPACE "\n "
EXPR_STMT
RETURN_EXPR
RETURN_KW "return"
SEMICOLON ";"
WHITESPACE "\n"
R_CURLY "}"
SEMICOLON ";"
WHITESPACE "\n"
error 0: expected an item
error 23: right curly brace `}` before `else` in a `let...else` statement not allowed
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
let foo = 1 + {
1
} else {
return;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
SOURCE_FILE
FN
FN_KW "fn"
WHITESPACE " "
NAME
IDENT "r"
PARAM_LIST
L_PAREN "("
R_PAREN ")"
WHITESPACE " "
BLOCK_EXPR
STMT_LIST
L_CURLY "{"
WHITESPACE "\n "
LET_STMT
LET_KW "let"
WHITESPACE " "
IDENT_PAT
NAME
IDENT "ok"
WHITESPACE " "
EQ "="
WHITESPACE " "
MACRO_EXPR
MACRO_CALL
PATH
PATH_SEGMENT
NAME_REF
IDENT "format_args"
BANG "!"
TOKEN_TREE
L_PAREN "("
STRING "\"\""
R_PAREN ")"
WHITESPACE " "
LET_ELSE
ELSE_KW "else"
WHITESPACE " "
BLOCK_EXPR
STMT_LIST
L_CURLY "{"
WHITESPACE " "
EXPR_STMT
RETURN_EXPR
RETURN_KW "return"
SEMICOLON ";"
WHITESPACE " "
R_CURLY "}"
SEMICOLON ";"
WHITESPACE "\n\n "
LET_STMT
LET_KW "let"
WHITESPACE " "
IDENT_PAT
NAME
IDENT "bad"
WHITESPACE " "
EQ "="
WHITESPACE " "
MACRO_EXPR
MACRO_CALL
PATH
PATH_SEGMENT
NAME_REF
IDENT "format_args"
BANG "!"
WHITESPACE " "
TOKEN_TREE
L_CURLY "{"
STRING "\"\""
R_CURLY "}"
WHITESPACE " "
LET_ELSE
ELSE_KW "else"
WHITESPACE " "
BLOCK_EXPR
STMT_LIST
L_CURLY "{"
WHITESPACE " "
EXPR_STMT
RETURN_EXPR
RETURN_KW "return"
SEMICOLON ";"
WHITESPACE " "
R_CURLY "}"
SEMICOLON ";"
WHITESPACE "\n"
R_CURLY "}"
WHITESPACE "\n"
error 89: right curly brace `}` before `else` in a `let...else` statement not allowed
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
fn r() {
let ok = format_args!("") else { return; };

let bad = format_args! {""} else { return; };
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
SOURCE_FILE
ERROR
LET_KW "let"
WHITESPACE " "
IDENT_PAT
NAME
IDENT "foo"
WHITESPACE " "
EQ "="
WHITESPACE " "
RANGE_EXPR
LITERAL
INT_NUMBER "1"
DOT2 ".."
BLOCK_EXPR
STMT_LIST
L_CURLY "{"
WHITESPACE "\n "
LITERAL
INT_NUMBER "1"
WHITESPACE "\n"
R_CURLY "}"
WHITESPACE " "
LET_ELSE
ELSE_KW "else"
WHITESPACE " "
BLOCK_EXPR
STMT_LIST
L_CURLY "{"
WHITESPACE "\n "
EXPR_STMT
RETURN_EXPR
RETURN_KW "return"
SEMICOLON ";"
WHITESPACE "\n"
R_CURLY "}"
SEMICOLON ";"
WHITESPACE "\n"
error 0: expected an item
error 22: right curly brace `}` before `else` in a `let...else` statement not allowed
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
let foo = 1..{
1
} else {
return;
};
Loading
Loading