Skip to content

Commit

Permalink
Rollup merge of #70908 - estebank:suggest-add, r=nikomatsakis
Browse files Browse the repository at this point in the history 
Provide suggestions for type parameters missing bounds for associated types

When implementing the binary operator traits it is easy to forget to restrict the `Output` associated type. `rustc` now accounts for different cases to lead users in the right direction to add the necessary restrictions. The structured suggestions in the following output are new:

```
error: equality constraints are not yet supported in `where` clauses
  --> $DIR/missing-bounds.rs:37:33
   |
LL | impl<B: Add> Add for E<B> where <B as Add>::Output = B {
   |                                 ^^^^^^^^^^^^^^^^^^^^^^ not supported
   |
   = note: see issue #20041 <#20041> for more information
help: if `Output` is an associated type you're trying to set, use the associated type binding syntax
   |
LL | impl<B: Add> Add for E<B> where B: Add<Output = B> {
   |                                 ^^^^^^^^^^^^^^^^^

error[E0308]: mismatched types
  --> $DIR/missing-bounds.rs:11:11
   |
7  | impl<B> Add for A<B> where B: Add {
   |      - this type parameter
...
11 |         A(self.0 + rhs.0)
   |           ^^^^^^^^^^^^^^ expected type parameter `B`, found associated type
   |
   = note: expected type parameter `B`
             found associated type `<B as std::ops::Add>::Output`
help: consider further restricting this bound
   |
7  | impl<B> Add for A<B> where B: Add + std::ops::Add<Output = B> {
   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0369]: cannot add `B` to `B`
  --> $DIR/missing-bounds.rs:31:21
   |
31 |         Self(self.0 + rhs.0)
   |              ------ ^ ----- B
   |              |
   |              B
   |
help: consider restricting type parameter `B`
   |
27 | impl<B: std::ops::Add<Output = B>> Add for D<B> {
   |       ^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

That output is given for the following cases:

```rust
struct A<B>(B);
impl<B> Add for A<B> where B: Add {
    type Output = Self;

    fn add(self, rhs: Self) -> Self {
        A(self.0 + rhs.0) //~ ERROR mismatched types
    }
}

struct D<B>(B);
impl<B> Add for D<B> {
    type Output = Self;

    fn add(self, rhs: Self) -> Self {
        Self(self.0 + rhs.0) //~ ERROR cannot add `B` to `B`
    }
}

struct E<B>(B);
impl<B: Add> Add for E<B> where <B as Add>::Output = B {
    type Output = Self;

    fn add(self, rhs: Self) -> Self {
        Self(self.0 + rhs.0)
    }
}
```
  • Loading branch information
@Dylan-DPC
Dylan-DPC authored May 6, 2020
2 parents 1836e3b + b17b20c commit ce14d6d
Show file tree
Hide file tree
Showing 16 changed files with 629 additions and 224 deletions.
96 changes: 85 additions & 11 deletions src/librustc_ast_passes/ast_validation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use rustc_session::Session;
use rustc_span::symbol::{kw, sym};
use rustc_span::Span;
use std::mem;
use std::ops::DerefMut;

const MORE_EXTERN: &str =
"for more information, visit https://doc.rust-lang.org/std/keyword.extern.html";
Expand Down Expand Up @@ -1113,17 +1114,7 @@ impl<'a> Visitor<'a> for AstValidator<'a> {

for predicate in &generics.where_clause.predicates {
if let WherePredicate::EqPredicate(ref predicate) = *predicate {
self.err_handler()
.struct_span_err(
predicate.span,
"equality constraints are not yet supported in `where` clauses",
)
.span_label(predicate.span, "not supported")
.note(
"see issue #20041 <https://github.com/rust-lang/rust/issues/20041> \
for more information",
)
.emit();
deny_equality_constraints(self, predicate, generics);
}
}

Expand Down Expand Up @@ -1300,6 +1291,89 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
}
}

/// When encountering an equality constraint in a `where` clause, emit an error. If the code seems
/// like it's setting an associated type, provide an appropriate suggestion.
fn deny_equality_constraints(
this: &mut AstValidator<'_>,
predicate: &WhereEqPredicate,
generics: &Generics,
) {
let mut err = this.err_handler().struct_span_err(
predicate.span,
"equality constraints are not yet supported in `where` clauses",
);
err.span_label(predicate.span, "not supported");

// Given `<A as Foo>::Bar = RhsTy`, suggest `A: Foo<Bar = RhsTy>`.
if let TyKind::Path(Some(qself), full_path) = &predicate.lhs_ty.kind {
if let TyKind::Path(None, path) = &qself.ty.kind {
match &path.segments[..] {
[PathSegment { ident, args: None, .. }] => {
for param in &generics.params {
if param.ident == *ident {
let param = ident;
match &full_path.segments[qself.position..] {
[PathSegment { ident, .. }] => {
// Make a new `Path` from `foo::Bar` to `Foo<Bar = RhsTy>`.
let mut assoc_path = full_path.clone();
// Remove `Bar` from `Foo::Bar`.
assoc_path.segments.pop();
let len = assoc_path.segments.len() - 1;
// Build `<Bar = RhsTy>`.
let arg = AngleBracketedArg::Constraint(AssocTyConstraint {
id: rustc_ast::node_id::DUMMY_NODE_ID,
ident: *ident,
kind: AssocTyConstraintKind::Equality {
ty: predicate.rhs_ty.clone(),
},
span: ident.span,
});
// Add `<Bar = RhsTy>` to `Foo`.
match &mut assoc_path.segments[len].args {
Some(args) => match args.deref_mut() {
GenericArgs::Parenthesized(_) => continue,
GenericArgs::AngleBracketed(args) => {
args.args.push(arg);
}
},
empty_args => {
*empty_args = AngleBracketedArgs {
span: ident.span,
args: vec![arg],
}
.into();
}
}
err.span_suggestion_verbose(
predicate.span,
&format!(
"if `{}` is an associated type you're trying to set, \
use the associated type binding syntax",
ident
),
format!(
"{}: {}",
param,
pprust::path_to_string(&assoc_path)
),
Applicability::MaybeIncorrect,
);
}
_ => {}
};
}
}
}
_ => {}
}
}
}
err.note(
"see issue #20041 <https://github.com/rust-lang/rust/issues/20041> for more information",
);
err.emit();
}

pub fn check_crate(session: &Session, krate: &Crate, lints: &mut LintBuffer) -> bool {
let mut validator = AstValidator {
session,
Expand Down
36 changes: 35 additions & 1 deletion src/librustc_hir/hir.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2626,8 +2626,42 @@ impl Node<'_> {
match self {
Node::TraitItem(TraitItem { generics, .. })
| Node::ImplItem(ImplItem { generics, .. })
| Node::Item(Item { kind: ItemKind::Fn(_, generics, _), .. }) => Some(generics),
| Node::Item(Item {
kind:
ItemKind::Trait(_, _, generics, ..)
| ItemKind::Impl { generics, .. }
| ItemKind::Fn(_, generics, _),
..
}) => Some(generics),
_ => None,
}
}

pub fn hir_id(&self) -> Option<HirId> {
match self {
Node::Item(Item { hir_id, .. })
| Node::ForeignItem(ForeignItem { hir_id, .. })
| Node::TraitItem(TraitItem { hir_id, .. })
| Node::ImplItem(ImplItem { hir_id, .. })
| Node::Field(StructField { hir_id, .. })
| Node::AnonConst(AnonConst { hir_id, .. })
| Node::Expr(Expr { hir_id, .. })
| Node::Stmt(Stmt { hir_id, .. })
| Node::Ty(Ty { hir_id, .. })
| Node::Binding(Pat { hir_id, .. })
| Node::Pat(Pat { hir_id, .. })
| Node::Arm(Arm { hir_id, .. })
| Node::Block(Block { hir_id, .. })
| Node::Local(Local { hir_id, .. })
| Node::MacroDef(MacroDef { hir_id, .. })
| Node::Lifetime(Lifetime { hir_id, .. })
| Node::Param(Param { hir_id, .. })
| Node::GenericParam(GenericParam { hir_id, .. }) => Some(*hir_id),
Node::TraitRef(TraitRef { hir_ref_id, .. }) => Some(*hir_ref_id),
Node::PathSegment(PathSegment { hir_id, .. }) => *hir_id,
Node::Variant(Variant { id, .. }) => Some(*id),
Node::Ctor(variant) => variant.ctor_hir_id(),
Node::Crate(_) | Node::Visibility(_) => None,
}
}
}
184 changes: 183 additions & 1 deletion src/librustc_middle/ty/diagnostics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@
use crate::ty::sty::InferTy;
use crate::ty::TyKind::*;
use crate::ty::TyS;
use crate::ty::{TyCtxt, TyS};
use rustc_errors::{Applicability, DiagnosticBuilder};
use rustc_hir as hir;
use rustc_hir::def_id::DefId;
use rustc_hir::{QPath, TyKind, WhereBoundPredicate, WherePredicate};
use rustc_span::{BytePos, Span};

impl<'tcx> TyS<'tcx> {
/// Similar to `TyS::is_primitive`, but also considers inferred numeric values to be primitive.
Expand Down Expand Up @@ -67,3 +72,180 @@ impl<'tcx> TyS<'tcx> {
}
}
}

/// Suggest restricting a type param with a new bound.
pub fn suggest_constraining_type_param(
tcx: TyCtxt<'_>,
generics: &hir::Generics<'_>,
err: &mut DiagnosticBuilder<'_>,
param_name: &str,
constraint: &str,
def_id: Option<DefId>,
) -> bool {
let param = generics.params.iter().find(|p| p.name.ident().as_str() == param_name);

let param = if let Some(param) = param {
param
} else {
return false;
};

const MSG_RESTRICT_BOUND_FURTHER: &str = "consider further restricting this bound";
let msg_restrict_type = format!("consider restricting type parameter `{}`", param_name);
let msg_restrict_type_further =
format!("consider further restricting type parameter `{}`", param_name);

if def_id == tcx.lang_items().sized_trait() {
// Type parameters are already `Sized` by default.
err.span_label(param.span, &format!("this type parameter needs to be `{}`", constraint));
return true;
}
let mut suggest_restrict = |span| {
err.span_suggestion_verbose(
span,
MSG_RESTRICT_BOUND_FURTHER,
format!(" + {}", constraint),
Applicability::MachineApplicable,
);
};

if param_name.starts_with("impl ") {
// If there's an `impl Trait` used in argument position, suggest
// restricting it:
//
// fn foo(t: impl Foo) { ... }
// --------
// |
// help: consider further restricting this bound with `+ Bar`
//
// Suggestion for tools in this case is:
//
// fn foo(t: impl Foo) { ... }
// --------
// |
// replace with: `impl Foo + Bar`

suggest_restrict(param.span.shrink_to_hi());
return true;
}

if generics.where_clause.predicates.is_empty()
// Given `trait Base<T = String>: Super<T>` where `T: Copy`, suggest restricting in the
// `where` clause instead of `trait Base<T: Copy = String>: Super<T>`.
&& !matches!(param.kind, hir::GenericParamKind::Type { default: Some(_), .. })
{
if let Some(bounds_span) = param.bounds_span() {
// If user has provided some bounds, suggest restricting them:
//
// fn foo<T: Foo>(t: T) { ... }
// ---
// |
// help: consider further restricting this bound with `+ Bar`
//
// Suggestion for tools in this case is:
//
// fn foo<T: Foo>(t: T) { ... }
// --
// |
// replace with: `T: Bar +`
suggest_restrict(bounds_span.shrink_to_hi());
} else {
// If user hasn't provided any bounds, suggest adding a new one:
//
// fn foo<T>(t: T) { ... }
// - help: consider restricting this type parameter with `T: Foo`
err.span_suggestion_verbose(
param.span.shrink_to_hi(),
&msg_restrict_type,
format!(": {}", constraint),
Applicability::MachineApplicable,
);
}

true
} else {
// This part is a bit tricky, because using the `where` clause user can
// provide zero, one or many bounds for the same type parameter, so we
// have following cases to consider:
//
// 1) When the type parameter has been provided zero bounds
//
// Message:
// fn foo<X, Y>(x: X, y: Y) where Y: Foo { ... }
// - help: consider restricting this type parameter with `where X: Bar`
//
// Suggestion:
// fn foo<X, Y>(x: X, y: Y) where Y: Foo { ... }
// - insert: `, X: Bar`
//
//
// 2) When the type parameter has been provided one bound
//
// Message:
// fn foo<T>(t: T) where T: Foo { ... }
// ^^^^^^
// |
// help: consider further restricting this bound with `+ Bar`
//
// Suggestion:
// fn foo<T>(t: T) where T: Foo { ... }
// ^^
// |
// replace with: `T: Bar +`
//
//
// 3) When the type parameter has been provided many bounds
//
// Message:
// fn foo<T>(t: T) where T: Foo, T: Bar {... }
// - help: consider further restricting this type parameter with `where T: Zar`
//
// Suggestion:
// fn foo<T>(t: T) where T: Foo, T: Bar {... }
// - insert: `, T: Zar`

let mut param_spans = Vec::new();

for predicate in generics.where_clause.predicates {
if let WherePredicate::BoundPredicate(WhereBoundPredicate {
span, bounded_ty, ..
}) = predicate
{
if let TyKind::Path(QPath::Resolved(_, path)) = &bounded_ty.kind {
if let Some(segment) = path.segments.first() {
if segment.ident.to_string() == param_name {
param_spans.push(span);
}
}
}
}
}

let where_clause_span = generics.where_clause.span_for_predicates_or_empty_place();
// Account for `fn foo<T>(t: T) where T: Foo,` so we don't suggest two trailing commas.
let mut trailing_comma = false;
if let Ok(snippet) = tcx.sess.source_map().span_to_snippet(where_clause_span) {
trailing_comma = snippet.ends_with(',');
}
let where_clause_span = if trailing_comma {
let hi = where_clause_span.hi();
Span::new(hi - BytePos(1), hi, where_clause_span.ctxt())
} else {
where_clause_span.shrink_to_hi()
};

match &param_spans[..] {
&[&param_span] => suggest_restrict(param_span.shrink_to_hi()),
_ => {
err.span_suggestion_verbose(
where_clause_span,
&msg_restrict_type_further,
format!(", {}: {}", param_name, constraint),
Applicability::MachineApplicable,
);
}
}

true
}
}
Loading

0 comments on commit ce14d6d

Please sign in to comment.