Repository navigation
Recover shorthand struct literals in if conditions - #162295
chenyukang wants to merge 3 commits into
Conversation
|
The parser was modified, potentially altering the grammar of (stable) Rust cc @fmease |
rustbot
added
S-waiting-on-review
|
r? @khyperia rustbot has assigned @khyperia. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| (self.look_ahead(1, |t| t.is_ident()) | ||
| && self.look_ahead(2, |t| t == &token::Comma || t == &token::Colon)) | ||
| // When struct literals are prohibited, `{ ident }` is normally parsed as a block. A | ||
| // second opening brace strongly suggests that the first pair belongs to a struct | ||
| // literal and the second starts the surrounding body. | ||
| || (self.restrictions.contains(Restrictions::NO_STRUCT_LITERAL) | ||
| && self.look_ahead(1, Token::is_non_reserved_ident) | ||
| && self.look_ahead(2, |token| *token == token::CloseBrace) | ||
| && self.look_ahead(3, |token| *token == token::OpenBrace)) |
There was a problem hiding this comment.
This modifies the stable grammar of Rust and is thus not acceptable.
It regresses stable code like the following:
fn scope(cond: bool) {
let u = ();
if cond {
u
}
{
println!();
}
}or smaller:
#[cfg(false)] fn f() { if x { x } {} }There was a problem hiding this comment.
aha, yes! I didn't notice is_likely_struct_lit is on happy path, let me think about it.
There was a problem hiding this comment.
I updated the code.
This fix adds a lightweight extra { check after parsing each if or while body in error_on_ambiguous_struct_literal. If another block follows, it inspects the parsed AST to detect whether a shorthand struct literal was misinterpreted as the body.
I am not entirely satisfied with adding work to the normal parsing path, but I have not found a cleaner recovery point.
rustbot
added
S-waiting-on-author
e8c0c3c to
ac7bb57
Compare
fmease
added
S-waiting-on-review
| Ok(self.mk_expr(lo.to(self.prev_token.span), ExprKind::If(cond, thn, els))) | ||
| } | ||
|
|
||
| fn error_on_ambiguous_struct_literal(&self, head: &Expr, body: &ast::Block) -> PResult<'a, ()> { |
There was a problem hiding this comment.
Could you rename this to recover_from_*, move it into parser/expr/diagnostics.rs & make it pub(super) there?
| Ok(self.mk_expr(lo.to(self.prev_token.span), ExprKind::If(cond, thn, els))) | ||
| } | ||
|
|
||
| fn error_on_ambiguous_struct_literal(&self, head: &Expr, body: &ast::Block) -> PResult<'a, ()> { |
There was a problem hiding this comment.
As you're probably aware of, this isn't the only function in this module that tries to recover from "ambiguous struct literals", we also have maybe_parse_struct_expr (with predicate is_likely_struct_lit).
It's been a while for me since I last looked at the impl of maybe_parse_struct_expr but I'm wondering is there are way to
- somehow consolidate those two approaches or
- remove
maybe_parse_struct_exprentirely & useerror_on_ambiguous_struct_literalin more places / make it smarter & more general or - if that doesn't work out rename both functions in such a way that it is clear for the reader how these two functions differ (e.g., what "patterns of code" they're trying to recover from)
There was a problem hiding this comment.
maybe_parse_struct_expr contains happy path code, so seems not good for the first two options, I extract a recover function from it, see commit:
is it clearer?
fmease
added
S-waiting-on-author
ac7bb57 to
e470f2d
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
e470f2d to
6612f74
Compare
fmease
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #165375) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
Fixes #147877