From a7b00f5807dc02f9990c649572d76a91a850110c Mon Sep 17 00:00:00 2001 From: Mazdak Farrokhzad Date: Wed, 15 May 2019 16:02:51 +0200 Subject: [PATCH] let_chains: Handle disallowing of let chains in places lowering won't support. --- src/librustc_passes/ast_validation.rs | 124 ++++++++++++++------------ src/librustc_passes/lib.rs | 1 + 2 files changed, 67 insertions(+), 58 deletions(-) diff --git a/src/librustc_passes/ast_validation.rs b/src/librustc_passes/ast_validation.rs index 00b6db0ed9a..96f7c985379 100644 --- a/src/librustc_passes/ast_validation.rs +++ b/src/librustc_passes/ast_validation.rs @@ -17,13 +17,11 @@ use syntax::attr; use syntax::feature_gate::is_builtin_attr; use syntax::source_map::Spanned; use syntax::symbol::{kw, sym}; -use syntax::ptr::P; use syntax::visit::{self, Visitor}; use syntax::{span_err, struct_span_err, walk_list}; use syntax_ext::proc_macro_decls::is_proc_macro_attr; use syntax_pos::{Span, MultiSpan}; use errors::{Applicability, FatalError}; -use log::debug; #[derive(Copy, Clone, Debug)] struct OuterImplTrait { @@ -76,20 +74,34 @@ struct AstValidator<'a> { /// these booleans. warning_period_57979_didnt_record_next_impl_trait: bool, warning_period_57979_impl_trait_in_proj: bool, + + /// Used to ban `let` expressions in inappropriate places. + is_let_allowed: bool, +} + +/// With the `new` value in `store`, +/// runs and returns the `scoped` computation, +/// resetting the old value of `store` after, +/// and returning the result of `scoped`. +fn with( + this: &mut C, + new: S, + store: impl Fn(&mut C) -> &mut S, + scoped: impl FnOnce(&mut C) -> T +) -> T { + let old = mem::replace(store(this), new); + let ret = scoped(this); + *store(this) = old; + ret } impl<'a> AstValidator<'a> { fn with_impl_trait_in_proj_warning(&mut self, v: bool, f: impl FnOnce(&mut Self) -> T) -> T { - let old = mem::replace(&mut self.warning_period_57979_impl_trait_in_proj, v); - let ret = f(self); - self.warning_period_57979_impl_trait_in_proj = old; - ret + with(self, v, |this| &mut this.warning_period_57979_impl_trait_in_proj, f) } fn with_banned_impl_trait(&mut self, f: impl FnOnce(&mut Self)) { - let old = mem::replace(&mut self.is_impl_trait_banned, true); - f(self); - self.is_impl_trait_banned = old; + with(self, true, |this| &mut this.is_impl_trait_banned, f) } fn with_banned_assoc_ty_bound(&mut self, f: impl FnOnce(&mut Self)) { @@ -99,9 +111,11 @@ impl<'a> AstValidator<'a> { } fn with_impl_trait(&mut self, outer: Option, f: impl FnOnce(&mut Self)) { - let old = mem::replace(&mut self.outer_impl_trait, outer); - f(self); - self.outer_impl_trait = old; + with(self, outer, |this| &mut this.outer_impl_trait, f) + } + + fn with_let_allowed(&mut self, v: bool, f: impl FnOnce(&mut Self)) { + with(self, v, |this| &mut this.is_let_allowed, f) } fn visit_assoc_ty_constraint_from_generic_args(&mut self, constraint: &'a AssocTyConstraint) { @@ -319,52 +333,36 @@ impl<'a> AstValidator<'a> { } } - /// With eRFC 2497, we need to check whether an expression is ambiguous and warn or error - /// depending on the edition, this function handles that. - fn while_if_let_ambiguity(&self, expr: &P) { - if let Some((span, op_kind)) = self.while_if_let_expr_ambiguity(&expr) { - let mut err = self.err_handler().struct_span_err( - span, &format!("ambiguous use of `{}`", op_kind.to_string()) - ); - - err.note( - "this will be a error until the `let_chains` feature is stabilized" - ); - err.note( - "see rust-lang/rust#53668 for more information" - ); - - if let Ok(snippet) = self.session.source_map().span_to_snippet(span) { - err.span_suggestion( - span, "consider adding parentheses", format!("({})", snippet), - Applicability::MachineApplicable, - ); - } - - err.emit(); - } - } - - /// With eRFC 2497 adding if-let chains, there is a requirement that the parsing of - /// `&&` and `||` in a if-let statement be unambiguous. This function returns a span and - /// a `BinOpKind` (either `&&` or `||` depending on what was ambiguous) if it is determined - /// that the current expression parsed is ambiguous and will break in future. - fn while_if_let_expr_ambiguity(&self, expr: &P) -> Option<(Span, BinOpKind)> { - debug!("while_if_let_expr_ambiguity: expr.node: {:?}", expr.node); + /// Visits the `expr` and adjusts whether `let $pat = $expr` is allowed in decendants. + /// Returns whether we walked into `expr` or not. + /// If we did, walking should not happen again. + fn visit_expr_with_let_maybe_allowed(&mut self, expr: &'a Expr) -> bool { match &expr.node { - ExprKind::Binary(op, _, _) if op.node == BinOpKind::And || op.node == BinOpKind::Or => { - Some((expr.span, op.node)) - }, - ExprKind::Range(ref lhs, ref rhs, _) => { - let lhs_ambiguous = lhs.as_ref() - .and_then(|lhs| self.while_if_let_expr_ambiguity(lhs)); - let rhs_ambiguous = rhs.as_ref() - .and_then(|rhs| self.while_if_let_expr_ambiguity(rhs)); - - lhs_ambiguous.or(rhs_ambiguous) + // Assuming the context permits, `($expr)` does not impose additional constraints. + ExprKind::Paren(_) => visit::walk_expr(self, expr), + // Assuming the context permits, + // l && r` allows decendants in `l` and `r` to be `let` expressions. + ExprKind::Binary(op, ..) if op.node == BinOpKind::And => visit::walk_expr(self, expr), + // However, we do allow it in the condition of the `if` expression. + // We do not allow `let` in `then` and `opt_else` directly. + ExprKind::If(ref cond, ref then, ref opt_else) => { + self.with_let_allowed(false, |this| { + this.visit_block(then); + walk_list!(this, visit_expr, opt_else); + }); + self.with_let_allowed(true, |this| this.visit_expr(cond)); } - _ => None, + // The same logic applies to `While`. + ExprKind::While(ref cond, ref then, ref opt_label) => { + walk_list!(self, visit_label, opt_label); + self.with_let_allowed(false, |this| this.visit_block(then)); + self.with_let_allowed(true, |this| this.visit_expr(cond)); + } + // Don't walk into `expr` and defer further checks to the caller. + _ => return false, } + + true } fn check_fn_decl(&self, fn_decl: &FnDecl) { @@ -494,18 +492,27 @@ fn validate_generics_order<'a>( impl<'a> Visitor<'a> for AstValidator<'a> { fn visit_expr(&mut self, expr: &'a Expr) { match expr.node { + ExprKind::Let(_, _) if !self.is_let_allowed => { + self.err_handler() + .struct_span_err(expr.span, "`let` expressions are not supported here") + .note("only supported directly in conditions of `if`- and `while`-expressions") + .note("as well as when nested within `&&` and parenthesis in those conditions") + .emit(); + } + _ if self.visit_expr_with_let_maybe_allowed(&expr) => { + // Prevent `walk_expr` to happen since we've already done that. + return; + } ExprKind::Closure(_, _, _, ref fn_decl, _, _) => { self.check_fn_decl(fn_decl); } - ExprKind::IfLet(_, ref expr, _, _) | ExprKind::WhileLet(_, ref expr, _, _) => - self.while_if_let_ambiguity(&expr), ExprKind::InlineAsm(..) if !self.session.target.target.options.allow_asm => { span_err!(self.session, expr.span, E0472, "asm! is unsupported on this target"); } _ => {} } - visit::walk_expr(self, expr) + self.with_let_allowed(false, |this| visit::walk_expr(this, expr)); } fn visit_ty(&mut self, ty: &'a Ty) { @@ -917,6 +924,7 @@ pub fn check_crate(session: &Session, krate: &Crate) -> (bool, bool) { is_assoc_ty_bound_banned: false, warning_period_57979_didnt_record_next_impl_trait: false, warning_period_57979_impl_trait_in_proj: false, + is_let_allowed: false, }; visit::walk_crate(&mut validator, krate); diff --git a/src/librustc_passes/lib.rs b/src/librustc_passes/lib.rs index bf2f7634e55..5f3d7159be6 100644 --- a/src/librustc_passes/lib.rs +++ b/src/librustc_passes/lib.rs @@ -8,6 +8,7 @@ #![feature(in_band_lifetimes)] #![feature(nll)] +#![feature(bind_by_move_pattern_guards)] #![feature(rustc_diagnostic_macros)] #![recursion_limit="256"]