Skip to content

Commit

Permalink
Rollup merge of rust-lang#69707 - estebank:impl-trait-missing-bounds,…
Browse files Browse the repository at this point in the history
… r=Centril

Handle `impl Trait` where `Trait` has an assoc type with missing bounds

When encountering a type parameter that needs more bounds the trivial case is `T` `where T: Bound`, but it can also be an `impl Trait` param that needs to be decomposed to a type param for cleaner code. For example, given

```rust
fn foo(constraints: impl Iterator) {
    for constraint in constraints {
        println!("{:?}", constraint);
    }
}
```

the previous output was

```
error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
1 | fn foo(constraints: impl Iterator) {
  |                                    - help: consider further restricting the associated type: `where <impl Iterator as std::iter::Iterator>::Item: std::fmt::Debug`
2 |     for constraint in constraints {
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
```

which is incorrect as `where <impl Iterator as std::iter::Iterator>::Item: std::fmt::Debug` is not valid syntax nor would it restrict the positional `impl Iterator` parameter if it were.

The output being introduced is

```
error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
help: introduce a type parameter with a trait bound instead of using `impl Trait`
   |
LL | fn foo<T: Iterator>(constraints: T) where <T as std::iter::Iterator>::Item: std::fmt::Debug  {
   |       ^^^^^^^^^^^^^              ^  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

This suggestion is correct and lead the user in the right direction: because you have an associated type restriction you can no longer use `impl Trait`, the only reasonable alternative is to introduce a named type parameter, bound by `Trait` and with a `where` binding on the associated type for the new type parameter `as Trait` for the missing bound.

*Ideally*, we would want to suggest something like the following, but that is not valid syntax today

```
error[E0277]: `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
 --> src/main.rs:3:26
  |
3 |         println!("{:?}", constraint);
  |                          ^^^^^^^^^^ `<impl Iterator as std::iter::Iterator>::Item` cannot be formatted using `{:?}` because it doesn't implement `std::fmt::Debug`
  |
  = help: the trait `std::fmt::Debug` is not implemented for `<impl Iterator as std::iter::Iterator>::Item`
  = note: required by `std::fmt::Debug::fmt`
help: introduce a type parameter with a trait bound instead of using `impl Trait`
   |
LL | fn foo(constraints: impl Iterator<Item: std::fmt::Debug>) {
   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

Fix rust-lang#69638.
  • Loading branch information
Dylan-DPC authored Apr 10, 2020
2 parents dae7bf3 + b46a7cb commit 6e49b91
Show file tree
Hide file tree
Showing 7 changed files with 304 additions and 50 deletions.
2 changes: 2 additions & 0 deletions src/librustc_trait_selection/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
#![feature(in_band_lifetimes)]
#![feature(crate_visibility_modifier)]
#![feature(or_patterns)]
#![feature(str_strip)]
#![feature(option_zip)]
#![recursion_limit = "512"] // For rustdoc

#[macro_use]
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_trait_selection/traits/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
// which is somewhat confusing.
self.suggest_restricting_param_bound(
&mut err,
&trait_ref,
trait_ref,
obligation.cause.body_id,
);
} else {
Expand Down
206 changes: 174 additions & 32 deletions src/librustc_trait_selection/traits/error_reporting/suggestions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use rustc_middle::ty::TypeckTables;
use rustc_middle::ty::{
self, AdtKind, DefIdTree, ToPredicate, Ty, TyCtxt, TypeFoldable, WithConstness,
};
use rustc_span::symbol::{kw, sym};
use rustc_span::symbol::{kw, sym, Symbol};
use rustc_span::{MultiSpan, Span, DUMMY_SP};
use std::fmt;

Expand All @@ -27,7 +27,7 @@ pub trait InferCtxtExt<'tcx> {
fn suggest_restricting_param_bound(
&self,
err: &mut DiagnosticBuilder<'_>,
trait_ref: &ty::PolyTraitRef<'_>,
trait_ref: ty::PolyTraitRef<'_>,
body_id: hir::HirId,
);

Expand Down Expand Up @@ -148,11 +148,128 @@ pub trait InferCtxtExt<'tcx> {
fn suggest_new_overflow_limit(&self, err: &mut DiagnosticBuilder<'_>);
}

fn predicate_constraint(generics: &hir::Generics<'_>, pred: String) -> (Span, String) {
(
generics.where_clause.span_for_predicates_or_empty_place().shrink_to_hi(),
format!(
"{} {} ",
if !generics.where_clause.predicates.is_empty() { "," } else { " where" },
pred,
),
)
}

/// Type parameter needs more bounds. The trivial case is `T` `where T: Bound`, but
/// it can also be an `impl Trait` param that needs to be decomposed to a type
/// param for cleaner code.
fn suggest_restriction(
generics: &hir::Generics<'_>,
msg: &str,
err: &mut DiagnosticBuilder<'_>,
fn_sig: Option<&hir::FnSig<'_>>,
projection: Option<&ty::ProjectionTy<'_>>,
trait_ref: ty::PolyTraitRef<'_>,
) {
let span = generics.where_clause.span_for_predicates_or_empty_place();
if span.from_expansion() || span.desugaring_kind().is_some() {
return;
}
// Given `fn foo(t: impl Trait)` where `Trait` requires assoc type `A`...
if let Some((bound_str, fn_sig)) =
fn_sig.zip(projection).and_then(|(sig, p)| match p.self_ty().kind {
// Shenanigans to get the `Trait` from the `impl Trait`.
ty::Param(param) => {
// `fn foo(t: impl Trait)`
// ^^^^^ get this string
param.name.as_str().strip_prefix("impl").map(|s| (s.trim_start().to_string(), sig))
}
_ => None,
})
{
// We know we have an `impl Trait` that doesn't satisfy a required projection.

// Find all of the ocurrences of `impl Trait` for `Trait` in the function arguments'
// types. There should be at least one, but there might be *more* than one. In that
// case we could just ignore it and try to identify which one needs the restriction,
// but instead we choose to suggest replacing all instances of `impl Trait` with `T`
// where `T: Trait`.
let mut ty_spans = vec![];
let impl_trait_str = format!("impl {}", bound_str);
for input in fn_sig.decl.inputs {
if let hir::TyKind::Path(hir::QPath::Resolved(
None,
hir::Path { segments: [segment], .. },
)) = input.kind
{
if segment.ident.as_str() == impl_trait_str.as_str() {
// `fn foo(t: impl Trait)`
// ^^^^^^^^^^ get this to suggest `T` instead

// There might be more than one `impl Trait`.
ty_spans.push(input.span);
}
}
}

let type_param_name = generics.params.next_type_param_name(Some(&bound_str));
// The type param `T: Trait` we will suggest to introduce.
let type_param = format!("{}: {}", type_param_name, bound_str);

// FIXME: modify the `trait_ref` instead of string shenanigans.
// Turn `<impl Trait as Foo>::Bar: Qux` into `<T as Foo>::Bar: Qux`.
let pred = trait_ref.without_const().to_predicate().to_string();
let pred = pred.replace(&impl_trait_str, &type_param_name);
let mut sugg = vec![
match generics
.params
.iter()
.filter(|p| match p.kind {
hir::GenericParamKind::Type {
synthetic: Some(hir::SyntheticTyParamKind::ImplTrait),
..
} => false,
_ => true,
})
.last()
{
// `fn foo(t: impl Trait)`
// ^ suggest `<T: Trait>` here
None => (generics.span, format!("<{}>", type_param)),
// `fn foo<A>(t: impl Trait)`
// ^^^ suggest `<A, T: Trait>` here
Some(param) => (
param.bounds_span().unwrap_or(param.span).shrink_to_hi(),
format!(", {}", type_param),
),
},
// `fn foo(t: impl Trait)`
// ^ suggest `where <T as Trait>::A: Bound`
predicate_constraint(generics, pred),
];
sugg.extend(ty_spans.into_iter().map(|s| (s, type_param_name.to_string())));

// Suggest `fn foo<T: Trait>(t: T) where <T as Trait>::A: Bound`.
// FIXME: once `#![feature(associated_type_bounds)]` is stabilized, we should suggest
// `fn foo(t: impl Trait<A: Bound>)` instead.
err.multipart_suggestion(
"introduce a type parameter with a trait bound instead of using `impl Trait`",
sugg,
Applicability::MaybeIncorrect,
);
} else {
// Trivial case: `T` needs an extra bound: `T: Bound`.
let (sp, sugg) =
predicate_constraint(generics, trait_ref.without_const().to_predicate().to_string());
let appl = Applicability::MachineApplicable;
err.span_suggestion(sp, &format!("consider further restricting {}", msg), sugg, appl);
}
}

impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
fn suggest_restricting_param_bound(
&self,
mut err: &mut DiagnosticBuilder<'_>,
trait_ref: &ty::PolyTraitRef<'_>,
trait_ref: ty::PolyTraitRef<'_>,
body_id: hir::HirId,
) {
let self_ty = trait_ref.self_ty();
Expand All @@ -162,27 +279,6 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
_ => return,
};

let suggest_restriction =
|generics: &hir::Generics<'_>, msg, err: &mut DiagnosticBuilder<'_>| {
let span = generics.where_clause.span_for_predicates_or_empty_place();
if !span.from_expansion() && span.desugaring_kind().is_none() {
err.span_suggestion(
generics.where_clause.span_for_predicates_or_empty_place().shrink_to_hi(),
&format!("consider further restricting {}", msg),
format!(
"{} {} ",
if !generics.where_clause.predicates.is_empty() {
","
} else {
" where"
},
trait_ref.without_const().to_predicate(),
),
Applicability::MachineApplicable,
);
}
};

// FIXME: Add check for trait bound that is already present, particularly `?Sized` so we
// don't suggest `T: Sized + ?Sized`.
let mut hir_id = body_id;
Expand All @@ -194,27 +290,47 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
..
}) if param_ty && self_ty == self.tcx.types.self_param => {
// Restricting `Self` for a single method.
suggest_restriction(&generics, "`Self`", err);
suggest_restriction(&generics, "`Self`", err, None, projection, trait_ref);
return;
}

hir::Node::TraitItem(hir::TraitItem {
generics,
kind: hir::TraitItemKind::Fn(..),
kind: hir::TraitItemKind::Fn(fn_sig, ..),
..
})
| hir::Node::ImplItem(hir::ImplItem {
generics,
kind: hir::ImplItemKind::Fn(..),
kind: hir::ImplItemKind::Fn(fn_sig, ..),
..
})
| hir::Node::Item(
hir::Item { kind: hir::ItemKind::Fn(_, generics, _), .. }
| hir::Item { kind: hir::ItemKind::Trait(_, _, generics, _, _), .. }
| hir::Node::Item(hir::Item {
kind: hir::ItemKind::Fn(fn_sig, generics, _), ..
}) if projection.is_some() => {
// Missing restriction on associated type of type parameter (unmet projection).
suggest_restriction(
&generics,
"the associated type",
err,
Some(fn_sig),
projection,
trait_ref,
);
return;
}
hir::Node::Item(
hir::Item { kind: hir::ItemKind::Trait(_, _, generics, _, _), .. }
| hir::Item { kind: hir::ItemKind::Impl { generics, .. }, .. },
) if projection.is_some() => {
// Missing associated type bound.
suggest_restriction(&generics, "the associated type", err);
// Missing restriction on associated type of type parameter (unmet projection).
suggest_restriction(
&generics,
"the associated type",
err,
None,
projection,
trait_ref,
);
return;
}

Expand Down Expand Up @@ -1588,3 +1704,29 @@ impl<'v> Visitor<'v> for ReturnsVisitor<'v> {
hir::intravisit::walk_body(self, body);
}
}

pub trait NextTypeParamName {
fn next_type_param_name(&self, name: Option<&str>) -> String;
}

impl NextTypeParamName for &[hir::GenericParam<'_>] {
fn next_type_param_name(&self, name: Option<&str>) -> String {
// This is the whitelist of possible parameter names that we might suggest.
let name = name.and_then(|n| n.chars().next()).map(|c| c.to_string().to_uppercase());
let name = name.as_ref().map(|s| s.as_str());
let possible_names = [name.unwrap_or("T"), "T", "U", "V", "X", "Y", "Z", "A", "B", "C"];
let used_names = self
.iter()
.filter_map(|p| match p.name {
hir::ParamName::Plain(ident) => Some(ident.name),
_ => None,
})
.collect::<Vec<_>>();

possible_names
.iter()
.find(|n| !used_names.contains(&Symbol::intern(n)))
.unwrap_or(&"ParamName")
.to_string()
}
}
16 changes: 2 additions & 14 deletions src/librustc_typeck/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ use rustc_session::parse::feature_err;
use rustc_span::symbol::{kw, sym, Symbol};
use rustc_span::{Span, DUMMY_SP};
use rustc_target::spec::abi;
use rustc_trait_selection::traits::error_reporting::suggestions::NextTypeParamName;

mod type_of;

Expand Down Expand Up @@ -135,20 +136,7 @@ crate fn placeholder_type_error(
if placeholder_types.is_empty() {
return;
}
// This is the whitelist of possible parameter names that we might suggest.
let possible_names = ["T", "K", "L", "A", "B", "C"];
let used_names = generics
.iter()
.filter_map(|p| match p.name {
hir::ParamName::Plain(ident) => Some(ident.name),
_ => None,
})
.collect::<Vec<_>>();

let type_name = possible_names
.iter()
.find(|n| !used_names.contains(&Symbol::intern(n)))
.unwrap_or(&"ParamName");
let type_name = generics.next_type_param_name(None);

let mut sugg: Vec<_> =
placeholder_types.iter().map(|sp| (*sp, (*type_name).to_string())).collect();
Expand Down
44 changes: 44 additions & 0 deletions src/test/ui/suggestions/impl-trait-with-missing-bounds.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// The double space in `impl Iterator` is load bearing! We want to make sure we don't regress by
// accident if the internal string representation changes.
#[rustfmt::skip]
fn foo(constraints: impl Iterator) {
for constraint in constraints {
qux(constraint);
//~^ ERROR `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
}
}

fn bar<T>(t: T, constraints: impl Iterator) where T: std::fmt::Debug {
for constraint in constraints {
qux(t);
qux(constraint);
//~^ ERROR `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
}
}

fn baz(t: impl std::fmt::Debug, constraints: impl Iterator) {
for constraint in constraints {
qux(t);
qux(constraint);
//~^ ERROR `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
}
}

fn bat<I, T: std::fmt::Debug>(t: T, constraints: impl Iterator, _: I) {
for constraint in constraints {
qux(t);
qux(constraint);
//~^ ERROR `<impl Iterator as std::iter::Iterator>::Item` doesn't implement `std::fmt::Debug`
}
}

fn bak(constraints: impl Iterator + std::fmt::Debug) {
for constraint in constraints {
qux(constraint);
//~^ ERROR `<impl Iterator + std::fmt::Debug as std::iter::Iterator>::Item` doesn't implement
}
}

fn qux(_: impl std::fmt::Debug) {}

fn main() {}
Loading

0 comments on commit 6e49b91

Please sign in to comment.