Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warning period for detecting nested impl trait #58608

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/librustc/lint/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -457,6 +463,7 @@ impl LintPass for HardwiredLints {
parser::ILL_FORMED_ATTRIBUTE_INPUT,
DEPRECATED_IN_FUTURE,
AMBIGUOUS_ASSOCIATED_ITEMS,
NESTED_IMPL_TRAIT,
)
}
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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");
}
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions src/librustc_lint/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
128 changes: 114 additions & 14 deletions src/librustc_passes/ast_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand All @@ -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,
Expand All @@ -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:
Expand Down Expand Up @@ -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;
}
_ => {}
}
Expand Down Expand Up @@ -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_));
}
}
}
Expand Down Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
@@ -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() { }
Original file line number Diff line number Diff line change
@@ -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`.
37 changes: 37 additions & 0 deletions src/test/ui/impl-trait/issue-57979-impl-trait-in-path.rs
Original file line number Diff line number Diff line change
@@ -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() { }
Loading