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

Recover from if (let ...) in parsing, instead of lowering #83407

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions compiler/rustc_ast/src/ast.rs
Expand Up @@ -1145,6 +1145,17 @@ impl Expr {
expr
}

pub fn peel_parens_owned(self) -> Expr {
let mut expr = self;
loop {
let Expr { id, kind, span, attrs, tokens } = expr;
match kind {
ExprKind::Paren(inner) => expr = inner.into_inner(),
kind => return Expr { id, kind, span, attrs, tokens },
}
}
}

/// Attempts to reparse as `Ty` (for diagnostic purposes).
pub fn to_ty(&self) -> Option<P<Ty>> {
let kind = match &self.kind {
Expand Down
36 changes: 0 additions & 36 deletions compiler/rustc_ast_lowering/src/expr.rs
Expand Up @@ -97,23 +97,6 @@ impl<'hir> LoweringContext<'_, 'hir> {
ExprKind::Let(ref pat, ref scrutinee) => {
self.lower_expr_if_let(e.span, pat, scrutinee, then, else_opt.as_deref())
}
ExprKind::Paren(ref paren) => match paren.peel_parens().kind {
ExprKind::Let(ref pat, ref scrutinee) => {
// A user has written `if (let Some(x) = foo) {`, we want to avoid
// confusing them with mentions of nightly features.
// If this logic is changed, you will also likely need to touch
// `unused::UnusedParens::check_expr`.
self.if_let_expr_with_parens(cond, &paren.peel_parens());
self.lower_expr_if_let(
e.span,
pat,
scrutinee,
then,
else_opt.as_deref(),
)
}
_ => self.lower_expr_if(cond, then, else_opt.as_deref()),
},
_ => self.lower_expr_if(cond, then, else_opt.as_deref()),
},
ExprKind::While(ref cond, ref body, opt_label) => self
Expand Down Expand Up @@ -370,25 +353,6 @@ impl<'hir> LoweringContext<'_, 'hir> {
hir::ExprKind::Call(f, self.lower_exprs(&real_args))
}

fn if_let_expr_with_parens(&mut self, cond: &Expr, paren: &Expr) {
let start = cond.span.until(paren.span);
let end = paren.span.shrink_to_hi().until(cond.span.shrink_to_hi());
self.sess
.struct_span_err(
vec![start, end],
"invalid parentheses around `let` expression in `if let`",
)
.multipart_suggestion(
"`if let` needs to be written without parentheses",
vec![(start, String::new()), (end, String::new())],
rustc_errors::Applicability::MachineApplicable,
)
.emit();
// Ideally, we'd remove the feature gating of a `let` expression since we are already
// complaining about it here, but `feature_gate::check_crate` has already run by now:
// self.sess.parse_sess.gated_spans.ungate_last(sym::let_chains, paren.span);
}

/// Emit an error and lower `ast::ExprKind::Let(pat, scrutinee)` into:
/// ```rust
/// match scrutinee { pats => true, _ => false }
Expand Down
31 changes: 30 additions & 1 deletion compiler/rustc_parse/src/parser/expr.rs
Expand Up @@ -1765,7 +1765,20 @@ impl<'a> Parser<'a> {
/// Parses an `if` expression (`if` token already eaten).
fn parse_if_expr(&mut self, attrs: AttrVec) -> PResult<'a, P<Expr>> {
let lo = self.prev_token.span;
let cond = self.parse_cond_expr()?;
let mut cond = self.parse_cond_expr()?.into_inner();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the into_inner necessary? IIUC, you need that to be able to reassing, but wouldn't changing line 1777 to be cond = P(cond.peel_parens_owned()); work as well and make this "unwrapping/rewrapping" unnecessary in the happy path? I'm just concerned about doing unnecessary work that might marginally slow down compile times.


if let ExprKind::Paren(paren) = &cond.kind {
Copy link
Contributor

Choose a reason for hiding this comment

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

We still need the feature gate check we talked about here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's blocked currently. Have you seen my comment above? #83407 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hadn't, looking into it. (The parser is indeed not supposed to be able to use that to avoid diverging parsing based on feature gates.)

let peeled = paren.peel_parens();
if let ExprKind::Let(_, _) = peeled.kind {
// A user has written `if (let <pat> = <expr>)` (with some number of parens), we
// want to avoid confusing them with mentions of nightly features. If this logic is
// changed, you will also likely need to touch `unused::UnusedParens::check_expr`.
self.if_let_expr_with_parens(&cond, peeled);
cond = cond.peel_parens_owned();
}
}

let cond = P(cond);

// Verify that the parsed `if` condition makes sense as a condition. If it is a block, then
// verify that the last statement is either an implicit return (no `;`) or an explicit
Expand Down Expand Up @@ -1794,6 +1807,22 @@ impl<'a> Parser<'a> {
Ok(self.mk_expr(lo.to(self.prev_token.span), ExprKind::If(cond, thn, els), attrs))
}

fn if_let_expr_with_parens(&mut self, cond: &Expr, paren: &Expr) {
let start = cond.span.until(paren.span);
let end = paren.span.shrink_to_hi().until(cond.span.shrink_to_hi());
self.struct_span_err(
vec![start, end],
"invalid parentheses around `let` expression in `if let`",
)
.multipart_suggestion(
"`if let` needs to be written without parentheses",
vec![(start, String::new()), (end, String::new())],
rustc_errors::Applicability::MachineApplicable,
)
.emit();
self.sess.gated_spans.ungate_last(sym::let_chains, paren.span);
}

fn error_missing_if_cond(&self, lo: Span, span: Span) -> P<ast::Block> {
let sp = self.sess.source_map().next_point(lo);
self.struct_span_err(sp, "missing condition for `if` expression")
Expand Down
9 changes: 3 additions & 6 deletions src/test/ui/rfc-2497-if-let-chains/feature-gate.rs
Expand Up @@ -12,12 +12,10 @@ fn _if() {
if let 0 = 1 {} // Stable!

if (let 0 = 1) {}
//~^ ERROR `let` expressions in this position are experimental [E0658]
//~| ERROR invalid parentheses around `let` expression in `if let`
//~^ ERROR invalid parentheses around `let` expression in `if let`

if (((let 0 = 1))) {}
//~^ ERROR `let` expressions in this position are experimental [E0658]
//~| ERROR invalid parentheses around `let` expression in `if let`
//~^ ERROR invalid parentheses around `let` expression in `if let`

if true && let 0 = 1 {}
//~^ ERROR `let` expressions in this position are experimental [E0658]
Expand Down Expand Up @@ -125,8 +123,7 @@ fn _macros() {
//~| ERROR `let` expressions are not supported here
//~| ERROR `let` expressions are not supported here
use_expr!((let 0 = 1));
//~^ ERROR `let` expressions in this position are experimental [E0658]
//~| ERROR invalid parentheses around `let` expression in `if let`
//~^ ERROR invalid parentheses around `let` expression in `if let`
//~| ERROR `let` expressions are not supported here
#[cfg(FALSE)] (let 0 = 1);
//~^ ERROR `let` expressions in this position are experimental [E0658]
Expand Down