From 13dc584db39e5f36755b7b051a908277f5e5505f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Fri, 18 Jan 2019 12:35:14 +0100 Subject: [PATCH 1/2] Merge visitors in AST validation --- src/librustc_passes/ast_validation.rs | 255 +++++++++++--------------- 1 file changed, 111 insertions(+), 144 deletions(-) diff --git a/src/librustc_passes/ast_validation.rs b/src/librustc_passes/ast_validation.rs index 3d0e46d998622..20f63b142997c 100644 --- a/src/librustc_passes/ast_validation.rs +++ b/src/librustc_passes/ast_validation.rs @@ -20,9 +20,79 @@ use errors::Applicability; struct AstValidator<'a> { session: &'a Session, + + // Used to ban nested `impl Trait`, e.g., `impl Into`. + // Nested `impl Trait` _is_ allowed in associated type position, + // e.g `impl Iterator` + outer_impl_trait: Option, + + // Used to ban `impl Trait` in path projections like `::Item` + // or `Foo::Bar` + is_impl_trait_banned: bool, } impl<'a> AstValidator<'a> { + fn with_banned_impl_trait(&mut self, f: F) + where F: FnOnce(&mut Self) + { + let old_is_impl_trait_banned = self.is_impl_trait_banned; + self.is_impl_trait_banned = true; + f(self); + self.is_impl_trait_banned = old_is_impl_trait_banned; + } + + fn with_impl_trait(&mut self, outer_impl_trait: Option, f: F) + where F: FnOnce(&mut Self) + { + let old_outer_impl_trait = self.outer_impl_trait; + self.outer_impl_trait = outer_impl_trait; + f(self); + self.outer_impl_trait = old_outer_impl_trait; + } + + // Mirrors visit::walk_ty, but tracks relevant state + fn walk_ty(&mut self, t: &'a Ty) { + match t.node { + TyKind::ImplTrait(..) => { + self.with_impl_trait(Some(t.span), |this| visit::walk_ty(this, t)) + } + TyKind::Path(ref qself, ref path) => { + // We allow these: + // - `Option` + // - `option::Option` + // - `option::Option::Foo + // + // But not these: + // - `::Foo` + // - `option::Option::Foo`. + // + // To implement this, we disallow `impl Trait` from `qself` + // (for cases like `::Foo>`) + // but we allow `impl Trait` in `GenericArgs` + // iff there are no more PathSegments. + if let Some(ref qself) = *qself { + // `impl Trait` in `qself` is always illegal + self.with_banned_impl_trait(|this| this.visit_ty(&qself.ty)); + } + + // Note that there should be a call to visit_path here, + // so if any logic is added to process `Path`s a call to it should be + // added both in visit_path and here. This code mirrors visit::walk_path. + for (i, segment) in path.segments.iter().enumerate() { + // Allow `impl Trait` iff we're on the final path segment + if i == path.segments.len() - 1 { + self.visit_path_segment(path.span, segment); + } else { + self.with_banned_impl_trait(|this| { + this.visit_path_segment(path.span, segment) + }); + } + } + } + _ => visit::walk_ty(self, t), + } + } + fn err_handler(&self) -> &errors::Handler { &self.session.diagnostic() } @@ -267,6 +337,19 @@ impl<'a> Visitor<'a> for AstValidator<'a> { self.no_questions_in_bounds(bounds, "trait object types", false); } TyKind::ImplTrait(_, ref bounds) => { + if self.is_impl_trait_banned { + struct_span_err!(self.session, ty.span, E0667, + "`impl Trait` is not allowed in path parameters").emit(); + } + + if let Some(outer_impl_trait) = self.outer_impl_trait { + struct_span_err!(self.session, ty.span, E0666, + "nested `impl Trait` is not allowed") + .span_label(outer_impl_trait, "outer `impl Trait`") + .span_label(ty.span, "nested `impl Trait` here") + .emit(); + + } if !bounds.iter() .any(|b| if let GenericBound::Trait(..) = *b { true } else { false }) { self.err_handler().span_err(ty.span, "at least one trait must be specified"); @@ -275,7 +358,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> { _ => {} } - visit::walk_ty(self, ty) + self.walk_ty(ty) } fn visit_label(&mut self, label: &'a Label) { @@ -414,6 +497,28 @@ impl<'a> Visitor<'a> for AstValidator<'a> { visit::walk_foreign_item(self, fi) } + // Mirrors visit::walk_generic_args, but tracks relevant state + fn visit_generic_args(&mut self, _: Span, generic_args: &'a GenericArgs) { + match *generic_args { + GenericArgs::AngleBracketed(ref data) => { + walk_list!(self, visit_generic_arg, &data.args); + // Type bindings such as `Item=impl Debug` in `Iterator` + // are allowed to contain nested `impl Trait`. + self.with_impl_trait(None, |this| { + walk_list!(this, visit_assoc_type_binding, &data.bindings); + }); + } + GenericArgs::Parenthesized(ref data) => { + walk_list!(self, visit_ty, &data.inputs); + if let Some(ref type_) = data.output { + // `-> Foo` syntax is essentially an associated type binding, + // so it is also allowed to contain nested `impl Trait`. + self.with_impl_trait(None, |this| visit::walk_ty(this, type_)); + } + } + } + } + fn visit_generics(&mut self, generics: &'a Generics) { let mut seen_non_lifetime_param = false; let mut seen_default = None; @@ -490,148 +595,10 @@ impl<'a> Visitor<'a> for AstValidator<'a> { } } -// Bans nested `impl Trait`, e.g., `impl Into`. -// Nested `impl Trait` _is_ allowed in associated type position, -// e.g `impl Iterator` -struct NestedImplTraitVisitor<'a> { - session: &'a Session, - outer_impl_trait: Option, -} - -impl<'a> NestedImplTraitVisitor<'a> { - fn with_impl_trait(&mut self, outer_impl_trait: Option, f: F) - where F: FnOnce(&mut NestedImplTraitVisitor<'a>) - { - let old_outer_impl_trait = self.outer_impl_trait; - self.outer_impl_trait = outer_impl_trait; - f(self); - self.outer_impl_trait = old_outer_impl_trait; - } -} - - -impl<'a> Visitor<'a> for NestedImplTraitVisitor<'a> { - fn visit_ty(&mut self, t: &'a Ty) { - if let TyKind::ImplTrait(..) = t.node { - if let Some(outer_impl_trait) = self.outer_impl_trait { - struct_span_err!(self.session, t.span, E0666, - "nested `impl Trait` is not allowed") - .span_label(outer_impl_trait, "outer `impl Trait`") - .span_label(t.span, "nested `impl Trait` here") - .emit(); - - } - self.with_impl_trait(Some(t.span), |this| visit::walk_ty(this, t)); - } else { - visit::walk_ty(self, t); - } - } - fn visit_generic_args(&mut self, _: Span, generic_args: &'a GenericArgs) { - match *generic_args { - GenericArgs::AngleBracketed(ref data) => { - for arg in &data.args { - self.visit_generic_arg(arg) - } - for type_binding in &data.bindings { - // Type bindings such as `Item=impl Debug` in `Iterator` - // are allowed to contain nested `impl Trait`. - self.with_impl_trait(None, |this| visit::walk_ty(this, &type_binding.ty)); - } - } - GenericArgs::Parenthesized(ref data) => { - for type_ in &data.inputs { - self.visit_ty(type_); - } - if let Some(ref type_) = data.output { - // `-> Foo` syntax is essentially an associated type binding, - // so it is also allowed to contain nested `impl Trait`. - self.with_impl_trait(None, |this| visit::walk_ty(this, type_)); - } - } - } - } - - fn visit_mac(&mut self, _mac: &Spanned) { - // covered in AstValidator - } -} - -// Bans `impl Trait` in path projections like `::Item` or `Foo::Bar`. -struct ImplTraitProjectionVisitor<'a> { - session: &'a Session, - is_banned: bool, -} - -impl<'a> ImplTraitProjectionVisitor<'a> { - fn with_ban(&mut self, f: F) - where F: FnOnce(&mut ImplTraitProjectionVisitor<'a>) - { - let old_is_banned = self.is_banned; - self.is_banned = true; - f(self); - self.is_banned = old_is_banned; - } -} - -impl<'a> Visitor<'a> for ImplTraitProjectionVisitor<'a> { - fn visit_ty(&mut self, t: &'a Ty) { - match t.node { - TyKind::ImplTrait(..) => { - if self.is_banned { - struct_span_err!(self.session, t.span, E0667, - "`impl Trait` is not allowed in path parameters").emit(); - } - } - TyKind::Path(ref qself, ref path) => { - // We allow these: - // - `Option` - // - `option::Option` - // - `option::Option::Foo - // - // But not these: - // - `::Foo` - // - `option::Option::Foo`. - // - // To implement this, we disallow `impl Trait` from `qself` - // (for cases like `::Foo>`) - // but we allow `impl Trait` in `GenericArgs` - // iff there are no more PathSegments. - if let Some(ref qself) = *qself { - // `impl Trait` in `qself` is always illegal - self.with_ban(|this| this.visit_ty(&qself.ty)); - } - - for (i, segment) in path.segments.iter().enumerate() { - // Allow `impl Trait` iff we're on the final path segment - if i == path.segments.len() - 1 { - visit::walk_path_segment(self, path.span, segment); - } else { - self.with_ban(|this| - visit::walk_path_segment(this, path.span, segment)); - } - } - } - _ => visit::walk_ty(self, t), - } - } - - fn visit_mac(&mut self, _mac: &Spanned) { - // covered in AstValidator - } -} - pub fn check_crate(session: &Session, krate: &Crate) { - visit::walk_crate( - &mut NestedImplTraitVisitor { - session, - outer_impl_trait: None, - }, krate); - - visit::walk_crate( - &mut ImplTraitProjectionVisitor { - session, - is_banned: false, - }, krate); - - visit::walk_crate(&mut AstValidator { session }, krate) + visit::walk_crate(&mut AstValidator { + session, + outer_impl_trait: None, + is_impl_trait_banned: false, + }, krate) } From a5d4aeddc8e2ecf1279f2138788777a29fedc3c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?John=20K=C3=A5re=20Alsaker?= Date: Sat, 19 Jan 2019 04:29:26 +0100 Subject: [PATCH 2/2] Address some comments --- src/librustc_passes/ast_validation.rs | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/src/librustc_passes/ast_validation.rs b/src/librustc_passes/ast_validation.rs index 20f63b142997c..d1a3d7c1f81e0 100644 --- a/src/librustc_passes/ast_validation.rs +++ b/src/librustc_passes/ast_validation.rs @@ -6,6 +6,7 @@ // This pass is supposed to perform only simple checks not requiring name resolution // or type checking or some other kind of complex analysis. +use std::mem; use rustc::lint; use rustc::session::Session; use syntax::ast::*; @@ -32,22 +33,16 @@ struct AstValidator<'a> { } impl<'a> AstValidator<'a> { - fn with_banned_impl_trait(&mut self, f: F) - where F: FnOnce(&mut Self) - { - let old_is_impl_trait_banned = self.is_impl_trait_banned; - self.is_impl_trait_banned = true; + 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_is_impl_trait_banned; + self.is_impl_trait_banned = old; } - fn with_impl_trait(&mut self, outer_impl_trait: Option, f: F) - where F: FnOnce(&mut Self) - { - let old_outer_impl_trait = self.outer_impl_trait; - self.outer_impl_trait = outer_impl_trait; + fn with_impl_trait(&mut self, outer_impl_trait: Option, f: impl FnOnce(&mut Self)) { + let old = mem::replace(&mut self.outer_impl_trait, outer_impl_trait); f(self); - self.outer_impl_trait = old_outer_impl_trait; + self.outer_impl_trait = old; } // Mirrors visit::walk_ty, but tracks relevant state