Auto merge of #58608 - pnkfelix:warning-period-for-detecting-nested-i…

…mpl-trait, r=zoxc

Warning period for detecting nested impl trait

Here is some proposed code for making a warning period for the new checking of nested impl trait.

It undoes some of the corrective effects of PR #57730, by using boolean flags to track parts of the analysis that were previously skipped prior to PRs #57730 and #57981 landing.

Cc #57979

src/librustc/lint/builtin.rs

@@ -386,6 +386,12 @@ declare_lint! {
     "ambiguous associated items"
 }
 
+declare_lint! {
+    pub NESTED_IMPL_TRAIT,
+    Warn,
+    "nested occurrence of `impl Trait` type"
+}
+
 /// Does nothing as a lint pass, but registers some `Lint`s
 /// that are used by other parts of the compiler.
 #[derive(Copy, Clone)]
@@ -457,6 +463,7 @@ impl LintPass for HardwiredLints {
             parser::ILL_FORMED_ATTRIBUTE_INPUT,
             DEPRECATED_IN_FUTURE,
             AMBIGUOUS_ASSOCIATED_ITEMS,
+            NESTED_IMPL_TRAIT,
         )
     }
 }
@@ -474,6 +481,7 @@ pub enum BuiltinLintDiagnostics {
     ElidedLifetimesInPaths(usize, Span, bool, Span, String),
     UnknownCrateTypes(Span, String, String),
     UnusedImports(String, Vec<(Span, String)>),
+    NestedImplTrait { outer_impl_trait_span: Span, inner_impl_trait_span: Span },
 }
 
 impl BuiltinLintDiagnostics {
@@ -564,6 +572,12 @@ impl BuiltinLintDiagnostics {
                     );
                 }
             }
+            BuiltinLintDiagnostics::NestedImplTrait {
+                outer_impl_trait_span, inner_impl_trait_span
+            } => {
+                db.span_label(outer_impl_trait_span, "outer `impl Trait`");
+                db.span_label(inner_impl_trait_span, "nested `impl Trait` here");
+            }
         }
     }
 }

src/librustc_lint/lib.rs

@@ -353,6 +353,11 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
             reference: "issue #57593 <https://github.com/rust-lang/rust/issues/57593>",
             edition: None,
         },
+        FutureIncompatibleInfo {
+            id: LintId::of(NESTED_IMPL_TRAIT),
+            reference: "issue #59014 <https://github.com/rust-lang/rust/issues/59014>",
+            edition: None,
+        },
         ]);
 
     // Register renamed and removed lints.

src/librustc_passes/ast_validation.rs

@@ -9,6 +9,7 @@
 use std::mem;
 use syntax::print::pprust;
 use rustc::lint;
+use rustc::lint::builtin::{BuiltinLintDiagnostics, NESTED_IMPL_TRAIT};
 use rustc::session::Session;
 use rustc_data_structures::fx::FxHashMap;
 use syntax::ast::*;
@@ -23,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,
@@ -31,31 +57,83 @@ 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>`
     is_impl_trait_banned: bool,
+
+    // rust-lang/rust#57979: the ban of nested `impl Trait` was buggy
+    // until PRs #57730 and #57981 landed: it would jump directly to
+    // walk_ty rather than visit_ty (or skip recurring entirely for
+    // 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_didnt_record_next_impl_trait: bool,
+    warning_period_57979_impl_trait_in_proj: bool,
 }
 
 impl<'a> AstValidator<'a> {
+    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);
+        self.warning_period_57979_impl_trait_in_proj = old;
+        ret
+    }
+
     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;
     }
 
-    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:
@@ -406,22 +484,41 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
             }
             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 self.warning_period_57979_impl_trait_in_proj {
+                        self.session.buffer_lint(
+                            NESTED_IMPL_TRAIT, ty.id, ty.span,
+                            "`impl Trait` is not allowed in path parameters");
+                    } else {
+                        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 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.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.span, "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");
                 }
+
+                self.with_impl_trait_in_proj_warning(true, |this| this.walk_ty(ty));
+                return;
             }
             _ => {}
         }
@@ -606,18 +703,19 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
                         GenericArg::Const(..) => ParamKindOrd::Const,
                     }, arg.span(), None)
                 }), GenericPosition::Arg, generic_args.span());
+
                 // 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, &data.bindings);
+                    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 {
                     // `-> 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(type_));
+                    self.with_impl_trait(None, |this| this.visit_ty_from_generic_args(type_));
                 }
             }
         }
@@ -719,6 +817,8 @@ 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_didnt_record_next_impl_trait: false,
+        warning_period_57979_impl_trait_in_proj: false,
     };
     visit::walk_crate(&mut validator, krate);
 

src/test/ui/impl-trait/issue-57979-deeply-nested-impl-trait-in-assoc-proj.rs

@@ -0,0 +1,42 @@
+// rust-lang/rust#57979 : the initial support for `impl Trait` didn't
+// properly check syntax hidden behind an associated type projection,
+// but it did catch *some cases*. This is checking that we continue to
+// properly emit errors for those, even with the new
+// future-incompatibility warnings.
+//
+// issue-57979-nested-impl-trait-in-assoc-proj.rs shows the main case
+// that we were previously failing to catch.
+
+struct Deeper<T>(T);
+
+mod allowed {
+    #![allow(nested_impl_trait)]
+
+    pub trait Foo<T> { }
+    pub trait Bar { }
+    pub trait Quux { type Assoc; }
+    pub fn demo(_: impl Quux<Assoc=super::Deeper<impl Foo<impl Bar>>>) { }
+    //~^ ERROR nested `impl Trait` is not allowed
+}
+
+mod warned {
+    #![warn(nested_impl_trait)]
+
+    pub trait Foo<T> { }
+    pub trait Bar { }
+    pub trait Quux { type Assoc; }
+    pub fn demo(_: impl Quux<Assoc=super::Deeper<impl Foo<impl Bar>>>) { }
+    //~^ ERROR nested `impl Trait` is not allowed
+}
+
+mod denied {
+    #![deny(nested_impl_trait)]
+
+    pub trait Foo<T> { }
+    pub trait Bar { }
+    pub trait Quux { type Assoc; }
+    pub fn demo(_: impl Quux<Assoc=super::Deeper<impl Foo<impl Bar>>>) { }
+    //~^ ERROR nested `impl Trait` is not allowed
+}
+
+fn main() { }

src/test/ui/impl-trait/issue-57979-deeply-nested-impl-trait-in-assoc-proj.stderr

@@ -0,0 +1,30 @@
+error[E0666]: nested `impl Trait` is not allowed
+  --> $DIR/issue-57979-deeply-nested-impl-trait-in-assoc-proj.rs:18:59
+   |
+LL |     pub fn demo(_: impl Quux<Assoc=super::Deeper<impl Foo<impl Bar>>>) { }
+   |                                                  ---------^^^^^^^^-
+   |                                                  |        |
+   |                                                  |        nested `impl Trait` here
+   |                                                  outer `impl Trait`
+
+error[E0666]: nested `impl Trait` is not allowed
+  --> $DIR/issue-57979-deeply-nested-impl-trait-in-assoc-proj.rs:28:59
+   |
+LL |     pub fn demo(_: impl Quux<Assoc=super::Deeper<impl Foo<impl Bar>>>) { }
+   |                                                  ---------^^^^^^^^-
+   |                                                  |        |
+   |                                                  |        nested `impl Trait` here
+   |                                                  outer `impl Trait`
+
+error[E0666]: nested `impl Trait` is not allowed
+  --> $DIR/issue-57979-deeply-nested-impl-trait-in-assoc-proj.rs:38:59
+   |
+LL |     pub fn demo(_: impl Quux<Assoc=super::Deeper<impl Foo<impl Bar>>>) { }
+   |                                                  ---------^^^^^^^^-
+   |                                                  |        |
+   |                                                  |        nested `impl Trait` here
+   |                                                  outer `impl Trait`
+
+error: aborting due to 3 previous errors
+
+For more information about this error, try `rustc --explain E0666`.

src/test/ui/impl-trait/issue-57979-impl-trait-in-path.rs

@@ -0,0 +1,37 @@
+// rust-lang/rust#57979 : the initial support for `impl Trait` didn't
+// properly check syntax hidden behind an associated type projection.
+// Here we test behavior of occurrences of `impl Trait` within a path
+// component in that context.
+
+mod allowed {
+    #![allow(nested_impl_trait)]
+
+    pub trait Bar { }
+    pub trait Quux<T> { type Assoc; }
+    pub fn demo(_: impl Quux<(), Assoc=<() as Quux<impl Bar>>::Assoc>) { }
+    impl<T> Quux<T> for () { type Assoc = u32; }
+}
+
+mod warned {
+    #![warn(nested_impl_trait)]
+
+    pub trait Bar { }
+    pub trait Quux<T> { type Assoc; }
+    pub fn demo(_: impl Quux<(), Assoc=<() as Quux<impl Bar>>::Assoc>) { }
+    //~^ WARN `impl Trait` is not allowed in path parameters
+    //~| WARN will become a hard error in a future release!
+    impl<T> Quux<T> for () { type Assoc = u32; }
+}
+
+mod denied {
+    #![deny(nested_impl_trait)]
+
+    pub trait Bar { }
+    pub trait Quux<T> { type Assoc; }
+    pub fn demo(_: impl Quux<(), Assoc=<() as Quux<impl Bar>>::Assoc>) { }
+    //~^ ERROR `impl Trait` is not allowed in path parameters
+    //~| WARN will become a hard error in a future release!
+    impl<T> Quux<T> for () { type Assoc = u32; }
+}
+
+fn main() { }