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

Improve invalid let expression handling #115677

Merged
merged 5 commits into from
Sep 14, 2023
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
4 changes: 2 additions & 2 deletions compiler/rustc_ast/src/ast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use rustc_macros::HashStable_Generic;
use rustc_serialize::{Decodable, Decoder, Encodable, Encoder};
use rustc_span::source_map::{respan, Spanned};
use rustc_span::symbol::{kw, sym, Ident, Symbol};
use rustc_span::{Span, DUMMY_SP};
use rustc_span::{ErrorGuaranteed, Span, DUMMY_SP};
use std::fmt;
use std::mem;
use thin_vec::{thin_vec, ThinVec};
Expand Down Expand Up @@ -1426,7 +1426,7 @@ pub enum ExprKind {
/// of `if` / `while` expressions. (e.g., `if let 0 = x { .. }`).
///
/// `Span` represents the whole `let pat = expr` statement.
Let(P<Pat>, P<Expr>, Span),
Let(P<Pat>, P<Expr>, Span, Option<ErrorGuaranteed>),
/// An `if` block, with an optional `else` block.
///
/// `if expr { block } else { expr }`
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast/src/mut_visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1366,7 +1366,7 @@ pub fn noop_visit_expr<T: MutVisitor>(
vis.visit_ty(ty);
}
ExprKind::AddrOf(_, _, ohs) => vis.visit_expr(ohs),
ExprKind::Let(pat, scrutinee, _) => {
ExprKind::Let(pat, scrutinee, _, _) => {
vis.visit_pat(pat);
vis.visit_expr(scrutinee);
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast/src/util/classify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ pub fn expr_trailing_brace(mut expr: &ast::Expr) -> Option<&ast::Expr> {
| AssignOp(_, _, e)
| Binary(_, _, e)
| Break(_, Some(e))
| Let(_, e, _)
| Let(_, e, _, _)
| Range(_, Some(e), _)
| Ret(Some(e))
| Unary(_, e)
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast/src/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -827,7 +827,7 @@ pub fn walk_expr<'a, V: Visitor<'a>>(visitor: &mut V, expression: &'a Expr) {
visitor.visit_expr(subexpression);
visitor.visit_ty(typ)
}
ExprKind::Let(pat, expr, _) => {
ExprKind::Let(pat, expr, _, _) => {
visitor.visit_pat(pat);
visitor.visit_expr(expr);
}
Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_ast_lowering/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,13 +152,14 @@ impl<'hir> LoweringContext<'_, 'hir> {
let ohs = self.lower_expr(ohs);
hir::ExprKind::AddrOf(*k, *m, ohs)
}
ExprKind::Let(pat, scrutinee, span) => {
ExprKind::Let(pat, scrutinee, span, is_recovered) => {
hir::ExprKind::Let(self.arena.alloc(hir::Let {
hir_id: self.next_id(),
span: self.lower_span(*span),
pat: self.lower_pat(pat),
ty: None,
init: self.lower_expr(scrutinee),
is_recovered: *is_recovered,
}))
}
ExprKind::If(cond, then, else_opt) => {
Expand Down Expand Up @@ -558,13 +559,14 @@ impl<'hir> LoweringContext<'_, 'hir> {
fn lower_arm(&mut self, arm: &Arm) -> hir::Arm<'hir> {
let pat = self.lower_pat(&arm.pat);
let guard = arm.guard.as_ref().map(|cond| {
if let ExprKind::Let(pat, scrutinee, span) = &cond.kind {
if let ExprKind::Let(pat, scrutinee, span, is_recovered) = &cond.kind {
hir::Guard::IfLet(self.arena.alloc(hir::Let {
hir_id: self.next_id(),
span: self.lower_span(*span),
pat: self.lower_pat(pat),
ty: None,
init: self.lower_expr(scrutinee),
is_recovered: *is_recovered,
}))
} else {
hir::Guard::If(self.lower_expr(cond))
Expand Down
10 changes: 0 additions & 10 deletions compiler/rustc_ast_passes/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -117,16 +117,6 @@ ast_passes_forbidden_default =
`default` is only allowed on items in trait impls
.label = `default` because of this

ast_passes_forbidden_let =
`let` expressions are not supported here
.note = only supported directly in conditions of `if` and `while` expressions
.not_supported_or = `||` operators are not supported in let chain expressions
.not_supported_parentheses = `let`s wrapped in parentheses are not supported in a context with let chains

ast_passes_forbidden_let_stable =
expected expression, found statement (`let`)
.note = variable declaration using `let` is a statement

ast_passes_forbidden_lifetime_bound =
lifetime bounds cannot be used in this context

Expand Down
103 changes: 0 additions & 103 deletions compiler/rustc_ast_passes/src/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,12 @@ use rustc_ast::{walk_list, StaticItem};
use rustc_ast_pretty::pprust::{self, State};
use rustc_data_structures::fx::FxIndexMap;
use rustc_feature::Features;
use rustc_macros::Subdiagnostic;
use rustc_parse::validate_attr;
use rustc_session::lint::builtin::{
DEPRECATED_WHERE_CLAUSE_LOCATION, MISSING_ABI, PATTERNS_IN_FNS_WITHOUT_BODY,
};
use rustc_session::lint::{BuiltinLintDiagnostics, LintBuffer};
use rustc_session::Session;
use rustc_span::source_map::Spanned;
use rustc_span::symbol::{kw, sym, Ident};
use rustc_span::Span;
use rustc_target::spec::abi;
Expand Down Expand Up @@ -69,9 +67,6 @@ struct AstValidator<'a> {
/// or `Foo::Bar<impl Trait>`
is_impl_trait_banned: bool,

/// See [ForbiddenLetReason]
forbidden_let_reason: Option<ForbiddenLetReason>,

lint_buffer: &'a mut LintBuffer,
}

Expand Down Expand Up @@ -118,26 +113,6 @@ impl<'a> AstValidator<'a> {
self.with_tilde_const(Some(ctx), f)
}

fn with_let_management(
&mut self,
forbidden_let_reason: Option<ForbiddenLetReason>,
f: impl FnOnce(&mut Self, Option<ForbiddenLetReason>),
) {
let old = mem::replace(&mut self.forbidden_let_reason, forbidden_let_reason);
f(self, old);
self.forbidden_let_reason = old;
}

/// Emits an error banning the `let` expression provided in the given location.
fn ban_let_expr(&self, expr: &'a Expr, forbidden_let_reason: ForbiddenLetReason) {
let sess = &self.session;
if sess.opts.unstable_features.is_nightly_build() {
sess.emit_err(errors::ForbiddenLet { span: expr.span, reason: forbidden_let_reason });
} else {
sess.emit_err(errors::ForbiddenLetStable { span: expr.span });
}
}

fn check_type_alias_where_clause_location(
&mut self,
ty_alias: &TyAlias,
Expand Down Expand Up @@ -779,67 +754,6 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
validate_attr::check_attr(&self.session.parse_sess, attr);
}

fn visit_expr(&mut self, expr: &'a Expr) {
self.with_let_management(Some(ForbiddenLetReason::GenericForbidden), |this, forbidden_let_reason| {
match &expr.kind {
ExprKind::Binary(Spanned { node: BinOpKind::Or, span }, lhs, rhs) => {
let local_reason = Some(ForbiddenLetReason::NotSupportedOr(*span));
this.with_let_management(local_reason, |this, _| this.visit_expr(lhs));
this.with_let_management(local_reason, |this, _| this.visit_expr(rhs));
}
ExprKind::If(cond, then, opt_else) => {
this.visit_block(then);
walk_list!(this, visit_expr, opt_else);
this.with_let_management(None, |this, _| this.visit_expr(cond));
return;
}
ExprKind::Let(..) if let Some(elem) = forbidden_let_reason => {
this.ban_let_expr(expr, elem);
},
ExprKind::Match(scrutinee, arms) => {
this.visit_expr(scrutinee);
for arm in arms {
this.visit_expr(&arm.body);
this.visit_pat(&arm.pat);
walk_list!(this, visit_attribute, &arm.attrs);
if let Some(guard) = &arm.guard {
this.with_let_management(None, |this, _| {
this.visit_expr(guard)
});
}
}
}
ExprKind::Paren(local_expr) => {
fn has_let_expr(expr: &Expr) -> bool {
match &expr.kind {
ExprKind::Binary(_, lhs, rhs) => has_let_expr(lhs) || has_let_expr(rhs),
ExprKind::Let(..) => true,
_ => false,
}
}
let local_reason = if has_let_expr(local_expr) {
Some(ForbiddenLetReason::NotSupportedParentheses(local_expr.span))
}
else {
forbidden_let_reason
};
this.with_let_management(local_reason, |this, _| this.visit_expr(local_expr));
}
ExprKind::Binary(Spanned { node: BinOpKind::And, .. }, ..) => {
this.with_let_management(forbidden_let_reason, |this, _| visit::walk_expr(this, expr));
return;
}
ExprKind::While(cond, then, opt_label) => {
walk_list!(this, visit_label, opt_label);
this.visit_block(then);
this.with_let_management(None, |this, _| this.visit_expr(cond));
return;
}
_ => visit::walk_expr(this, expr),
}
});
}

fn visit_ty(&mut self, ty: &'a Ty) {
self.visit_ty_common(ty);
self.deny_anon_struct_or_union(ty);
Expand Down Expand Up @@ -1601,26 +1515,9 @@ pub fn check_crate(
outer_impl_trait: None,
disallow_tilde_const: None,
is_impl_trait_banned: false,
forbidden_let_reason: Some(ForbiddenLetReason::GenericForbidden),
lint_buffer: lints,
};
visit::walk_crate(&mut validator, krate);

validator.has_proc_macro_decls
}

/// Used to forbid `let` expressions in certain syntactic locations.
#[derive(Clone, Copy, Subdiagnostic)]
pub(crate) enum ForbiddenLetReason {
/// `let` is not valid and the source environment is not important
GenericForbidden,
/// A let chain with the `||` operator
#[note(ast_passes_not_supported_or)]
NotSupportedOr(#[primary_span] Span),
/// A let chain with invalid parentheses
///
/// For example, `let 1 = 1 && (expr && expr)` is allowed
/// but `(let 1 = 1 && (let 1 = 1 && (let 1 = 1))) && let a = 1` is not
#[note(ast_passes_not_supported_parentheses)]
NotSupportedParentheses(#[primary_span] Span),
}
19 changes: 0 additions & 19 deletions compiler/rustc_ast_passes/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,8 @@ use rustc_errors::AddToDiagnostic;
use rustc_macros::{Diagnostic, Subdiagnostic};
use rustc_span::{symbol::Ident, Span, Symbol};

use crate::ast_validation::ForbiddenLetReason;
use crate::fluent_generated as fluent;

#[derive(Diagnostic)]
#[diag(ast_passes_forbidden_let)]
#[note]
pub struct ForbiddenLet {
#[primary_span]
pub span: Span,
#[subdiagnostic]
pub(crate) reason: ForbiddenLetReason,
}

#[derive(Diagnostic)]
#[diag(ast_passes_forbidden_let_stable)]
#[note]
pub struct ForbiddenLetStable {
#[primary_span]
pub span: Span,
}

#[derive(Diagnostic)]
#[diag(ast_passes_keyword_lifetime)]
pub struct KeywordLifetime {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast_pretty/src/pprust/state/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ impl<'a> State<'a> {
self.end();
self.word(")");
}
ast::ExprKind::Let(pat, scrutinee, _) => {
ast::ExprKind::Let(pat, scrutinee, _, _) => {
self.print_let(pat, scrutinee);
}
ast::ExprKind::If(test, blk, elseopt) => self.print_if(test, blk, elseopt.as_deref()),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_builtin_macros/src/assert/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ impl<'cx, 'a> Context<'cx, 'a> {
self.manage_cond_expr(prefix);
self.manage_cond_expr(suffix);
}
ExprKind::Let(_, local_expr, _) => {
ExprKind::Let(_, local_expr, _, _) => {
self.manage_cond_expr(local_expr);
}
ExprKind::Match(local_expr, _) => {
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_hir/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use rustc_macros::HashStable_Generic;
use rustc_span::hygiene::MacroKind;
use rustc_span::source_map::Spanned;
use rustc_span::symbol::{kw, sym, Ident, Symbol};
use rustc_span::ErrorGuaranteed;
use rustc_span::{def_id::LocalDefId, BytePos, Span, DUMMY_SP};
use rustc_target::asm::InlineAsmRegOrRegClass;
use rustc_target::spec::abi::Abi;
Expand Down Expand Up @@ -1415,6 +1416,9 @@ pub struct Let<'hir> {
pub pat: &'hir Pat<'hir>,
pub ty: Option<&'hir Ty<'hir>>,
pub init: &'hir Expr<'hir>,
/// `Some` when this let expressions is not in a syntanctically valid location.
/// Used to prevent building MIR in such situations.
pub is_recovered: Option<ErrorGuaranteed>,
}

#[derive(Debug, Clone, Copy, HashStable_Generic)]
Expand Down
6 changes: 5 additions & 1 deletion compiler/rustc_hir_analysis/src/check/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ fn resolve_block<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, blk: &'tcx h
// From now on, we continue normally.
visitor.cx = prev_cx;
}
hir::StmtKind::Local(..) | hir::StmtKind::Item(..) => {
hir::StmtKind::Local(..) => {
// Each declaration introduces a subscope for bindings
// introduced by the declaration; this subscope covers a
// suffix of the block. Each subscope in a block has the
Expand All @@ -163,6 +163,10 @@ fn resolve_block<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, blk: &'tcx h
visitor.cx.var_parent = visitor.cx.parent;
visitor.visit_stmt(statement)
}
hir::StmtKind::Item(..) => {
// Don't create scopes for items, since they won't be
// lowered to THIR and MIR.
}
hir::StmtKind::Expr(..) | hir::StmtKind::Semi(..) => visitor.visit_stmt(statement),
}
}
Expand Down
7 changes: 6 additions & 1 deletion compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1203,7 +1203,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// otherwise check exactly as a let statement
self.check_decl(let_expr.into());
// but return a bool, for this is a boolean expression
self.tcx.types.bool
if let Some(error_guaranteed) = let_expr.is_recovered {
self.set_tainted_by_errors(error_guaranteed);
Ty::new_error(self.tcx, error_guaranteed)
} else {
self.tcx.types.bool
}
}

fn check_expr_loop(
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_typeck/src/gather_locals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ impl<'a> From<&'a hir::Local<'a>> for Declaration<'a> {

impl<'a> From<&'a hir::Let<'a>> for Declaration<'a> {
fn from(let_expr: &'a hir::Let<'a>) -> Self {
let hir::Let { hir_id, pat, ty, span, init } = *let_expr;
let hir::Let { hir_id, pat, ty, span, init, is_recovered: _ } = *let_expr;
Declaration { hir_id, pat, ty, span, init: Some(init), origin: DeclOrigin::LetExpr }
}
}
Expand Down
12 changes: 5 additions & 7 deletions compiler/rustc_lint/src/unused.rs
Original file line number Diff line number Diff line change
Expand Up @@ -816,8 +816,7 @@ trait UnusedDelimLint {
let (value, ctx, followed_by_block, left_pos, right_pos, is_kw) = match e.kind {
// Do not lint `unused_braces` in `if let` expressions.
If(ref cond, ref block, _)
if !matches!(cond.kind, Let(_, _, _))
|| Self::LINT_EXPR_IN_PATTERN_MATCHING_CTX =>
if !matches!(cond.kind, Let(..)) || Self::LINT_EXPR_IN_PATTERN_MATCHING_CTX =>
{
let left = e.span.lo() + rustc_span::BytePos(2);
let right = block.span.lo();
Expand All @@ -826,8 +825,7 @@ trait UnusedDelimLint {

// Do not lint `unused_braces` in `while let` expressions.
While(ref cond, ref block, ..)
if !matches!(cond.kind, Let(_, _, _))
|| Self::LINT_EXPR_IN_PATTERN_MATCHING_CTX =>
if !matches!(cond.kind, Let(..)) || Self::LINT_EXPR_IN_PATTERN_MATCHING_CTX =>
{
let left = e.span.lo() + rustc_span::BytePos(5);
let right = block.span.lo();
Expand Down Expand Up @@ -1003,7 +1001,7 @@ impl UnusedDelimLint for UnusedParens {
self.emit_unused_delims_expr(cx, value, ctx, left_pos, right_pos, is_kw)
}
}
ast::ExprKind::Let(_, ref expr, _) => {
ast::ExprKind::Let(_, ref expr, _, _) => {
self.check_unused_delims_expr(
cx,
expr,
Expand Down Expand Up @@ -1067,7 +1065,7 @@ impl EarlyLintPass for UnusedParens {
}

match e.kind {
ExprKind::Let(ref pat, _, _) | ExprKind::ForLoop(ref pat, ..) => {
ExprKind::Let(ref pat, _, _, _) | ExprKind::ForLoop(ref pat, ..) => {
self.check_unused_parens_pat(cx, pat, false, false, (true, true));
}
// We ignore parens in cases like `if (((let Some(0) = Some(1))))` because we already
Expand Down Expand Up @@ -1311,7 +1309,7 @@ impl UnusedDelimLint for UnusedBraces {
}
}
}
ast::ExprKind::Let(_, ref expr, _) => {
ast::ExprKind::Let(_, ref expr, _, _) => {
self.check_unused_delims_expr(
cx,
expr,
Expand Down
Loading