Auto merge of #58608 - pnkfelix:warning-period-for-detecting-nested-impl-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,10 +703,11 @@ 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) => {
@@ -617,7 +715,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
                 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() { }

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

@@ -0,0 +1,30 @@
+warning: `impl Trait` is not allowed in path parameters
+  --> $DIR/issue-57979-impl-trait-in-path.rs:20:52
+   |
+LL |     pub fn demo(_: impl Quux<(), Assoc=<() as Quux<impl Bar>>::Assoc>) { }
+   |                                                    ^^^^^^^^
+   |
+note: lint level defined here
+  --> $DIR/issue-57979-impl-trait-in-path.rs:16:13
+   |
+LL |     #![warn(nested_impl_trait)]
+   |             ^^^^^^^^^^^^^^^^^
+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+   = note: for more information, see issue #59014 <https://github.com/rust-lang/rust/issues/59014>
+
+error: `impl Trait` is not allowed in path parameters
+  --> $DIR/issue-57979-impl-trait-in-path.rs:31:52
+   |
+LL |     pub fn demo(_: impl Quux<(), Assoc=<() as Quux<impl Bar>>::Assoc>) { }
+   |                                                    ^^^^^^^^
+   |
+note: lint level defined here
+  --> $DIR/issue-57979-impl-trait-in-path.rs:27:13
+   |
+LL |     #![deny(nested_impl_trait)]
+   |             ^^^^^^^^^^^^^^^^^
+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+   = note: for more information, see issue #59014 <https://github.com/rust-lang/rust/issues/59014>
+
+error: aborting due to previous error
+

src/test/ui/impl-trait/issue-57979-nested-impl-trait-in-assoc-proj.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 an
+// `impl Trait` in that context.
+
+mod allowed {
+    #![allow(nested_impl_trait)]
+
+    pub trait Foo<T> { }
+    pub trait Bar { }
+    pub trait Quux { type Assoc; }
+    pub fn demo(_: impl Quux<Assoc=impl Foo<impl Bar>>) { }
+}
+
+mod warned {
+    #![warn(nested_impl_trait)]
+
+    pub trait Foo<T> { }
+    pub trait Bar { }
+    pub trait Quux { type Assoc; }
+    pub fn demo(_: impl Quux<Assoc=impl Foo<impl Bar>>) { }
+    //~^ WARN nested `impl Trait` is not allowed
+    //~| WARN will become a hard error in a future release!
+}
+
+mod denied {
+    #![deny(nested_impl_trait)]
+
+    pub trait Foo<T> { }
+    pub trait Bar { }
+    pub trait Quux { type Assoc; }
+    pub fn demo(_: impl Quux<Assoc=impl Foo<impl Bar>>) { }
+    //~^ ERROR nested `impl Trait` is not allowed
+    //~| WARN will become a hard error in a future release!
+}
+
+fn main() { }

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

@@ -0,0 +1,36 @@
+warning: nested `impl Trait` is not allowed
+  --> $DIR/issue-57979-nested-impl-trait-in-assoc-proj.rs:21:45
+   |
+LL |     pub fn demo(_: impl Quux<Assoc=impl Foo<impl Bar>>) { }
+   |                                    ---------^^^^^^^^-
+   |                                    |        |
+   |                                    |        nested `impl Trait` here
+   |                                    outer `impl Trait`
+   |
+note: lint level defined here
+  --> $DIR/issue-57979-nested-impl-trait-in-assoc-proj.rs:16:13
+   |
+LL |     #![warn(nested_impl_trait)]
+   |             ^^^^^^^^^^^^^^^^^
+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+   = note: for more information, see issue #59014 <https://github.com/rust-lang/rust/issues/59014>
+
+error: nested `impl Trait` is not allowed
+  --> $DIR/issue-57979-nested-impl-trait-in-assoc-proj.rs:32:45
+   |
+LL |     pub fn demo(_: impl Quux<Assoc=impl Foo<impl Bar>>) { }
+   |                                    ---------^^^^^^^^-
+   |                                    |        |
+   |                                    |        nested `impl Trait` here
+   |                                    outer `impl Trait`
+   |
+note: lint level defined here
+  --> $DIR/issue-57979-nested-impl-trait-in-assoc-proj.rs:27:13
+   |
+LL |     #![deny(nested_impl_trait)]
+   |             ^^^^^^^^^^^^^^^^^
+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+   = note: for more information, see issue #59014 <https://github.com/rust-lang/rust/issues/59014>
+
+error: aborting due to previous error
+

src/test/ui/issues/issue-57979.rs

@@ -1,42 +0,0 @@
-// Regression test for #57979. This situation is meant to be an error.
-// As noted in the issue thread, we decided to forbid nested impl
-// trait of this kind:
-//
-// ```rust
-// fn foo() -> impl Foo<impl Bar> { .. }
-// ```
-//
-// Basically there are two hidden variables here, let's call them `X`
-// and `Y`, and we must prove that:
-//
-// ```
-// X: Foo<Y>
-// Y: Bar
-// ```
-//
-// However, the user is only giving us the return type `X`. It's true
-// that in some cases, we can infer `Y` from `X`, because `X` only
-// implements `Foo` for one type (and indeed the compiler does
-// inference of this kind), but I do recall that we intended to forbid
-// this -- in part because such inference is fragile, and there is not
-// necessarily a way for the user to be more explicit should the
-// inference fail (so you could get stuck with no way to port your
-// code forward if, for example, more impls are added to an existing
-// type).
-//
-// The same seems to apply in this situation. Here there are three impl traits, so we have
-//
-// ```
-// X: IntoIterator<Item = Y>
-// Y: Borrow<Data<Z>>
-// Z: AsRef<[u8]>
-// ```
-
-use std::borrow::Borrow;
-
-pub struct Data<TBody>(TBody);
-
-pub fn collect(_: impl IntoIterator<Item = impl Borrow<Data<impl AsRef<[u8]>>>>) {
-    //~^ ERROR
-    unimplemented!()
-}

src/test/ui/issues/issue-57979.stderr

@@ -1,17 +0,0 @@
-error[E0666]: nested `impl Trait` is not allowed
-  --> $DIR/issue-57979.rs:39:61
-   |
-LL | pub fn collect(_: impl IntoIterator<Item = impl Borrow<Data<impl AsRef<[u8]>>>>) {
-   |                                            -----------------^^^^^^^^^^^^^^^^--
-   |                                            |                |
-   |                                            |                nested `impl Trait` here
-   |                                            outer `impl Trait`
-
-error[E0601]: `main` function not found in crate `issue_57979`
-   |
-   = note: consider adding a `main` function to `$DIR/issue-57979.rs`
-
-error: aborting due to 2 previous errors
-
-Some errors occurred: E0601, E0666.
-For more information about an error, try `rustc --explain E0601`.