Revised warning-downgrade strategy for nested impl trait.

Instead of a sticky-boolean flag that would downgrade errors to
warnings during further recursion into the type (which is overly broad
because we were not missing errors at arbitrarily deep levels), this
instead tracks state closer to what the original bug actually was.

In particular, the actual original bug was that we were failing to
record the existence of an outer `impl Trait` solely when it occurred
as an *immediate child* during the walk of the child types in
`visit_generic_args`.

Therefore, the correct way to precisely model when that bug would
manifest itself (and thus downgrade the error-to-warning accordingly)
is to track when those outer `impl Trait` cases were previously
unrecorded.

That's what this code does, by storing a flag with the recorded outer
`impl Trait` indicating at which point in the compiler's control flow
it had been stored.

I will note that this commit passes the current test suite. A
follow-up commit will also include tests illustrating the cases that
this commit gets right (and were handled incorrectly by the previous
sticky boolean).

Author: pnkfelix

src/librustc_passes/ast_validation.rs

@@ -24,6 +24,31 @@ use syntax_pos::Span;
 use errors::Applicability;
 use log::debug;
 
+#[derive(Copy, Clone, Debug)]
+struct OuterImplTrait {
+    span: Span,
+
+    /// rust-lang/rust#57979: a bug in original implementation caused
+    /// us to fail sometimes to record an outer `impl Trait`.
+    /// Therefore, in order to reliably issue a warning (rather than
+    /// an error) in the *precise* places where we are newly injecting
+    /// the diagnostic, we have to distinguish between the places
+    /// where the outer `impl Trait` has always been recorded, versus
+    /// the places where it has only recently started being recorded.
+    only_recorded_since_pull_request_57730: bool,
+}
+
+impl OuterImplTrait {
+    /// This controls whether we should downgrade the nested impl
+    /// trait diagnostic to a warning rather than an error, based on
+    /// whether the outer impl trait had been improperly skipped in
+    /// earlier implementations of the analysis on the stable
+    /// compiler.
+    fn should_warn_instead_of_error(&self) -> bool {
+        self.only_recorded_since_pull_request_57730
+    }
+}
+
 struct AstValidator<'a> {
     session: &'a Session,
     has_proc_macro_decls: bool,
@@ -32,7 +57,7 @@ struct AstValidator<'a> {
     // Used to ban nested `impl Trait`, e.g., `impl Into<impl Debug>`.
     // Nested `impl Trait` _is_ allowed in associated type position,
     // e.g `impl Iterator<Item=impl Debug>`
-    outer_impl_trait: Option<Span>,
+    outer_impl_trait: Option<OuterImplTrait>,
 
     // Used to ban `impl Trait` in path projections like `<impl Iterator>::Item`
     // or `Foo::Bar<impl Trait>`
@@ -44,18 +69,11 @@ struct AstValidator<'a> {
     // impl trait in projections), and thus miss some cases. We track
     // whether we should downgrade to a warning for short-term via
     // these booleans.
-    warning_period_57979_nested_impl_trait: bool,
+    warning_period_57979_didnt_record_next_impl_trait: bool,
     warning_period_57979_impl_trait_in_proj: bool,
 }
 
 impl<'a> AstValidator<'a> {
-    fn with_nested_impl_trait_warning<T>(&mut self, v: bool, f: impl FnOnce(&mut Self) -> T) -> T {
-        let old = mem::replace(&mut self.warning_period_57979_nested_impl_trait, v);
-        let ret = f(self);
-        self.warning_period_57979_nested_impl_trait = old;
-        ret
-    }
-
     fn with_impl_trait_in_proj_warning<T>(&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);
@@ -69,17 +87,53 @@ impl<'a> AstValidator<'a> {
         self.is_impl_trait_banned = old;
     }
 
-    fn with_impl_trait(&mut self, outer_impl_trait: Option<Span>, f: impl FnOnce(&mut Self)) {
-        let old = mem::replace(&mut self.outer_impl_trait, outer_impl_trait);
+    fn with_impl_trait(&mut self, outer: Option<OuterImplTrait>, f: impl FnOnce(&mut Self)) {
+        let old = mem::replace(&mut self.outer_impl_trait, outer);
         f(self);
         self.outer_impl_trait = old;
     }
 
+    fn visit_assoc_type_binding_from_generic_args(&mut self, type_binding: &'a TypeBinding) {
+        // rust-lang/rust#57979: bug in old visit_generic_args called
+        // walk_ty rather than visit_ty, skipping outer `impl Trait`
+        // if it happened to occur at `type_binding.ty`
+        if let TyKind::ImplTrait(..) = type_binding.ty.node {
+            self.warning_period_57979_didnt_record_next_impl_trait = true;
+        }
+        self.visit_assoc_type_binding(type_binding);
+    }
+
+    fn visit_ty_from_generic_args(&mut self, ty: &'a Ty) {
+        // rust-lang/rust#57979: bug in old visit_generic_args called
+        // walk_ty rather than visit_ty, skippping outer `impl Trait`
+        // if it happened to occur at `ty`
+        if let TyKind::ImplTrait(..) = ty.node {
+            self.warning_period_57979_didnt_record_next_impl_trait = true;
+        }
+        self.visit_ty(ty);
+    }
+
+    fn outer_impl_trait(&mut self, span: Span) -> OuterImplTrait {
+        let only_recorded_since_pull_request_57730 =
+            self.warning_period_57979_didnt_record_next_impl_trait;
+
+        // (this flag is designed to be set to true and then only
+        // reach the construction point for the outer impl trait once,
+        // so its safe and easiest to unconditionally reset it to
+        // false)
+        self.warning_period_57979_didnt_record_next_impl_trait = false;
+
+        OuterImplTrait {
+            span, only_recorded_since_pull_request_57730,
+        }
+    }
+
     // 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))
+                let outer_impl_trait = self.outer_impl_trait(t.span);
+                self.with_impl_trait(Some(outer_impl_trait), |this| visit::walk_ty(this, t))
             }
             TyKind::Path(ref qself, ref path) => {
                 // We allow these:
@@ -441,18 +495,18 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
                 }
 
                 if let Some(outer_impl_trait) = self.outer_impl_trait {
-                    if self.warning_period_57979_nested_impl_trait {
+                    if outer_impl_trait.should_warn_instead_of_error() {
                         self.session.buffer_lint_with_diagnostic(
                             NESTED_IMPL_TRAIT, ty.id, ty.span,
                             "nested `impl Trait` is not allowed",
                             BuiltinLintDiagnostics::NestedImplTrait {
-                                outer_impl_trait_span: outer_impl_trait,
+                                outer_impl_trait_span: outer_impl_trait.span,
                                 inner_impl_trait_span: ty.span,
                             });
                     } else {
                         struct_span_err!(self.session, ty.span, E0666,
                             "nested `impl Trait` is not allowed")
-                            .span_label(outer_impl_trait, "outer `impl Trait`")
+                            .span_label(outer_impl_trait.span, "outer `impl Trait`")
                             .span_label(ty.span, "nested `impl Trait` here")
                             .emit();
                     }
@@ -650,22 +704,18 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
                     }, arg.span(), None)
                 }), GenericPosition::Arg, generic_args.span());
 
-                self.with_nested_impl_trait_warning(true, |this| {
-                    // Type bindings such as `Item=impl Debug` in `Iterator<Item=Debug>`
-                    // are allowed to contain nested `impl Trait`.
-                    this.with_impl_trait(None, |this| {
-                        walk_list!(this, visit_assoc_type_binding, &data.bindings);
-                    });
+                // Type bindings such as `Item=impl Debug` in `Iterator<Item=Debug>`
+                // are allowed to contain nested `impl Trait`.
+                self.with_impl_trait(None, |this| {
+                    walk_list!(this, visit_assoc_type_binding_from_generic_args, &data.bindings);
                 });
             }
             GenericArgs::Parenthesized(ref data) => {
                 walk_list!(self, visit_ty, &data.inputs);
                 if let Some(ref type_) = data.output {
-                    self.with_nested_impl_trait_warning(true, |this| {
-                        // `-> Foo` syntax is essentially an associated type binding,
-                        // so it is also allowed to contain nested `impl Trait`.
-                        this.with_impl_trait(None, |this| this.visit_ty(type_));
-                    });
+                    // `-> Foo` syntax is essentially an associated type binding,
+                    // so it is also allowed to contain nested `impl Trait`.
+                    self.with_impl_trait(None, |this| this.visit_ty_from_generic_args(type_));
                 }
             }
         }
@@ -767,7 +817,7 @@ pub fn check_crate(session: &Session, krate: &Crate) -> (bool, bool) {
         has_global_allocator: false,
         outer_impl_trait: None,
         is_impl_trait_banned: false,
-        warning_period_57979_nested_impl_trait: false,
+        warning_period_57979_didnt_record_next_impl_trait: false,
         warning_period_57979_impl_trait_in_proj: false,
     };
     visit::walk_crate(&mut validator, krate);