From 98e40170be3d8016437d421828ffc36b43f676c2 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 20 Feb 2019 22:24:32 +0100 Subject: [PATCH 1/5] Temporarily emulate the (accidentally) omitted recursion during impl Trait check. Note that the two previous visitors were omitting slightly different recursive calls, so I need two flags to properly emulate them. --- src/librustc/lint/builtin.rs | 14 +++++ src/librustc_lint/lib.rs | 5 ++ src/librustc_passes/ast_validation.rs | 80 ++++++++++++++++++++++----- 3 files changed, 84 insertions(+), 15 deletions(-) diff --git a/src/librustc/lint/builtin.rs b/src/librustc/lint/builtin.rs index 655707ff9bd0d..ad0ed39185c1c 100644 --- a/src/librustc/lint/builtin.rs +++ b/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"); + } } } } diff --git a/src/librustc_lint/lib.rs b/src/librustc_lint/lib.rs index 5c243e1389073..5634faff00e6a 100644 --- a/src/librustc_lint/lib.rs +++ b/src/librustc_lint/lib.rs @@ -353,6 +353,11 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) { reference: "issue #57593 ", edition: None, }, + FutureIncompatibleInfo { + id: LintId::of(NESTED_IMPL_TRAIT), + reference: "issue #59014 ", + edition: None, + }, ]); // Register renamed and removed lints. diff --git a/src/librustc_passes/ast_validation.rs b/src/librustc_passes/ast_validation.rs index f96fc3b897f80..d5b02de969896 100644 --- a/src/librustc_passes/ast_validation.rs +++ b/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::*; @@ -36,9 +37,32 @@ struct AstValidator<'a> { // Used to ban `impl Trait` in path projections like `::Item` // or `Foo::Bar` is_impl_trait_banned: bool, + + // rust-lang/rust#57979: the ban of nested `impl Trait` was buggy + // until sometime after PR #57730 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_nested_impl_trait: bool, + warning_period_57979_impl_trait_in_proj: bool, } impl<'a> AstValidator<'a> { + fn with_nested_impl_trait_warning(&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(&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); @@ -406,22 +430,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 self.warning_period_57979_nested_impl_trait { + 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, + 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(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 +649,23 @@ 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` - // are allowed to contain nested `impl Trait`. - self.with_impl_trait(None, |this| { - walk_list!(this, visit_assoc_type_binding, &data.bindings); + + self.with_nested_impl_trait_warning(true, |this| { + // Type bindings such as `Item=impl Debug` in `Iterator` + // are allowed to contain nested `impl Trait`. + this.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| this.visit_ty(type_)); + 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_)); + }); } } } @@ -719,6 +767,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_nested_impl_trait: false, + warning_period_57979_impl_trait_in_proj: false, }; visit::walk_crate(&mut validator, krate); From d6cee67c27cae727e24f266f76d6613e2781f7b6 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 22 Feb 2019 15:10:12 +0100 Subject: [PATCH 2/5] Unit (and regression) tests for warning cycle code. --- .../issue-57979-impl-trait-in-path.rs | 37 ++++++++++++++++ .../issue-57979-impl-trait-in-path.stderr | 30 +++++++++++++ ...e-57979-nested-impl-trait-in-assoc-proj.rs | 37 ++++++++++++++++ ...979-nested-impl-trait-in-assoc-proj.stderr | 36 ++++++++++++++++ src/test/ui/issues/issue-57979.rs | 42 ------------------- src/test/ui/issues/issue-57979.stderr | 17 -------- 6 files changed, 140 insertions(+), 59 deletions(-) create mode 100644 src/test/ui/impl-trait/issue-57979-impl-trait-in-path.rs create mode 100644 src/test/ui/impl-trait/issue-57979-impl-trait-in-path.stderr create mode 100644 src/test/ui/impl-trait/issue-57979-nested-impl-trait-in-assoc-proj.rs create mode 100644 src/test/ui/impl-trait/issue-57979-nested-impl-trait-in-assoc-proj.stderr delete mode 100644 src/test/ui/issues/issue-57979.rs delete mode 100644 src/test/ui/issues/issue-57979.stderr diff --git a/src/test/ui/impl-trait/issue-57979-impl-trait-in-path.rs b/src/test/ui/impl-trait/issue-57979-impl-trait-in-path.rs new file mode 100644 index 0000000000000..84fcb5e2880a7 --- /dev/null +++ b/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 { type Assoc; } + pub fn demo(_: impl Quux<(), Assoc=<() as Quux>::Assoc>) { } + impl Quux for () { type Assoc = u32; } +} + +mod warned { + #![warn(nested_impl_trait)] + + pub trait Bar { } + pub trait Quux { type Assoc; } + pub fn demo(_: impl Quux<(), Assoc=<() as Quux>::Assoc>) { } + //~^ WARN `impl Trait` is not allowed in path parameters + //~| WARN will become a hard error in a future release! + impl Quux for () { type Assoc = u32; } +} + +mod denied { + #![deny(nested_impl_trait)] + + pub trait Bar { } + pub trait Quux { type Assoc; } + pub fn demo(_: impl Quux<(), Assoc=<() as Quux>::Assoc>) { } + //~^ ERROR `impl Trait` is not allowed in path parameters + //~| WARN will become a hard error in a future release! + impl Quux for () { type Assoc = u32; } +} + +fn main() { } diff --git a/src/test/ui/impl-trait/issue-57979-impl-trait-in-path.stderr b/src/test/ui/impl-trait/issue-57979-impl-trait-in-path.stderr new file mode 100644 index 0000000000000..982ecba291f79 --- /dev/null +++ b/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>::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 + +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>::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 + +error: aborting due to previous error + diff --git a/src/test/ui/impl-trait/issue-57979-nested-impl-trait-in-assoc-proj.rs b/src/test/ui/impl-trait/issue-57979-nested-impl-trait-in-assoc-proj.rs new file mode 100644 index 0000000000000..5c20ffc7c6724 --- /dev/null +++ b/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 { } + pub trait Bar { } + pub trait Quux { type Assoc; } + pub fn demo(_: impl Quux>) { } +} + +mod warned { + #![warn(nested_impl_trait)] + + pub trait Foo { } + pub trait Bar { } + pub trait Quux { type Assoc; } + pub fn demo(_: impl Quux>) { } + //~^ 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 { } + pub trait Bar { } + pub trait Quux { type Assoc; } + pub fn demo(_: impl Quux>) { } + //~^ ERROR nested `impl Trait` is not allowed + //~| WARN will become a hard error in a future release! +} + +fn main() { } diff --git a/src/test/ui/impl-trait/issue-57979-nested-impl-trait-in-assoc-proj.stderr b/src/test/ui/impl-trait/issue-57979-nested-impl-trait-in-assoc-proj.stderr new file mode 100644 index 0000000000000..508aea2432132 --- /dev/null +++ b/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>) { } + | ---------^^^^^^^^- + | | | + | | 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 + +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>) { } + | ---------^^^^^^^^- + | | | + | | 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 + +error: aborting due to previous error + diff --git a/src/test/ui/issues/issue-57979.rs b/src/test/ui/issues/issue-57979.rs deleted file mode 100644 index abd46b60ab194..0000000000000 --- a/src/test/ui/issues/issue-57979.rs +++ /dev/null @@ -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 { .. } -// ``` -// -// Basically there are two hidden variables here, let's call them `X` -// and `Y`, and we must prove that: -// -// ``` -// X: Foo -// 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 -// Y: Borrow> -// Z: AsRef<[u8]> -// ``` - -use std::borrow::Borrow; - -pub struct Data(TBody); - -pub fn collect(_: impl IntoIterator>>>) { - //~^ ERROR - unimplemented!() -} diff --git a/src/test/ui/issues/issue-57979.stderr b/src/test/ui/issues/issue-57979.stderr deleted file mode 100644 index 488f30ab7c5a7..0000000000000 --- a/src/test/ui/issues/issue-57979.stderr +++ /dev/null @@ -1,17 +0,0 @@ -error[E0666]: nested `impl Trait` is not allowed - --> $DIR/issue-57979.rs:39:61 - | -LL | pub fn collect(_: impl IntoIterator>>>) { - | -----------------^^^^^^^^^^^^^^^^-- - | | | - | | 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`. From c99303351dd67df8657d9f11ca00d65dcceb8438 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 11 Mar 2019 15:14:24 +0100 Subject: [PATCH 3/5] 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). --- src/librustc_passes/ast_validation.rs | 104 +++++++++++++++++++------- 1 file changed, 77 insertions(+), 27 deletions(-) diff --git a/src/librustc_passes/ast_validation.rs b/src/librustc_passes/ast_validation.rs index d5b02de969896..1dc5892159295 100644 --- a/src/librustc_passes/ast_validation.rs +++ b/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`. // Nested `impl Trait` _is_ allowed in associated type position, // e.g `impl Iterator` - outer_impl_trait: Option, + outer_impl_trait: Option, // Used to ban `impl Trait` in path projections like `::Item` // or `Foo::Bar` @@ -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(&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(&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, 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, 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` - // 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` + // 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); From 837856d120ec3350f1e9d194801a6a89e5d71630 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Mon, 11 Mar 2019 16:30:40 +0100 Subject: [PATCH 4/5] Test illustrating that the nested_impl_trait lint should only catch shallow cases. --- ...-deeply-nested-impl-trait-in-assoc-proj.rs | 42 +++++++++++++++++++ ...ply-nested-impl-trait-in-assoc-proj.stderr | 30 +++++++++++++ 2 files changed, 72 insertions(+) create mode 100644 src/test/ui/impl-trait/issue-57979-deeply-nested-impl-trait-in-assoc-proj.rs create mode 100644 src/test/ui/impl-trait/issue-57979-deeply-nested-impl-trait-in-assoc-proj.stderr diff --git a/src/test/ui/impl-trait/issue-57979-deeply-nested-impl-trait-in-assoc-proj.rs b/src/test/ui/impl-trait/issue-57979-deeply-nested-impl-trait-in-assoc-proj.rs new file mode 100644 index 0000000000000..5eef6a39325fe --- /dev/null +++ b/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); + +mod allowed { + #![allow(nested_impl_trait)] + + pub trait Foo { } + pub trait Bar { } + pub trait Quux { type Assoc; } + pub fn demo(_: impl Quux>>) { } + //~^ ERROR nested `impl Trait` is not allowed +} + +mod warned { + #![warn(nested_impl_trait)] + + pub trait Foo { } + pub trait Bar { } + pub trait Quux { type Assoc; } + pub fn demo(_: impl Quux>>) { } + //~^ ERROR nested `impl Trait` is not allowed +} + +mod denied { + #![deny(nested_impl_trait)] + + pub trait Foo { } + pub trait Bar { } + pub trait Quux { type Assoc; } + pub fn demo(_: impl Quux>>) { } + //~^ ERROR nested `impl Trait` is not allowed +} + +fn main() { } diff --git a/src/test/ui/impl-trait/issue-57979-deeply-nested-impl-trait-in-assoc-proj.stderr b/src/test/ui/impl-trait/issue-57979-deeply-nested-impl-trait-in-assoc-proj.stderr new file mode 100644 index 0000000000000..2b6f15e6d3eb2 --- /dev/null +++ b/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>>) { } + | ---------^^^^^^^^- + | | | + | | 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>>) { } + | ---------^^^^^^^^- + | | | + | | 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>>) { } + | ---------^^^^^^^^- + | | | + | | nested `impl Trait` here + | outer `impl Trait` + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0666`. From 0a03ca74935efab546298a8394a0ed0b31ccb646 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 12 Mar 2019 13:00:50 +0100 Subject: [PATCH 5/5] Addressed review feedback regarding comment phrasing. --- src/librustc_passes/ast_validation.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/librustc_passes/ast_validation.rs b/src/librustc_passes/ast_validation.rs index 1dc5892159295..b85429cd3cfdc 100644 --- a/src/librustc_passes/ast_validation.rs +++ b/src/librustc_passes/ast_validation.rs @@ -64,8 +64,8 @@ struct AstValidator<'a> { is_impl_trait_banned: bool, // rust-lang/rust#57979: the ban of nested `impl Trait` was buggy - // until sometime after PR #57730 landed: it would jump directly - // to walk_ty rather than visit_ty (or skip recurring entirely for + // 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.