Skip to content

Commit

Permalink
Auto merge of #67665 - Patryk27:master, r=zackmdavis
Browse files Browse the repository at this point in the history 
Improve reporting errors and suggestions for trait bounds

Fix #66802

- When printing errors for unsized function parameter, properly point at the parameter instead of function's body.
- Improve `consider further restricting this bound` (and related) messages by separating human-oriented hints from the machine-oriented ones.
  • Loading branch information
@bors
bors committed Feb 9, 2020
2 parents 71c7e14 + a8d34c1 commit 840bdc3
Show file tree
Hide file tree
Showing 71 changed files with 1,039 additions and 375 deletions.
228 changes: 228 additions & 0 deletions src/librustc/traits/error_reporting/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder};
use rustc_hir as hir;
use rustc_hir::def_id::{DefId, LOCAL_CRATE};
use rustc_hir::{QPath, TyKind, WhereBoundPredicate, WherePredicate};
use rustc_span::source_map::SourceMap;
use rustc_span::{ExpnKind, Span, DUMMY_SP};
use std::fmt;
use syntax::ast;
Expand Down Expand Up @@ -1426,3 +1428,229 @@ impl ArgKind {
}
}
}

/// 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,
source_map: &SourceMap,
span: Span,
def_id: Option<DefId>,
) -> bool {
const MSG_RESTRICT_BOUND_FURTHER: &str = "consider further restricting this bound with";
const MSG_RESTRICT_TYPE: &str = "consider restricting this type parameter with";
const MSG_RESTRICT_TYPE_FURTHER: &str = "consider further restricting this type parameter with";

let param = generics.params.iter().filter(|p| p.name.ident().as_str() == param_name).next();

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

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;
}

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`

err.span_help(param.span, &format!("{} `+ {}`", MSG_RESTRICT_BOUND_FURTHER, constraint));

err.tool_only_span_suggestion(
param.span,
MSG_RESTRICT_BOUND_FURTHER,
format!("{} + {}", param_name, constraint),
Applicability::MachineApplicable,
);

return true;
}

if generics.where_clause.predicates.is_empty() {
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 +`

err.span_help(
bounds_span,
&format!("{} `+ {}`", MSG_RESTRICT_BOUND_FURTHER, constraint),
);

let span_hi = param.span.with_hi(span.hi());
let span_with_colon = source_map.span_through_char(span_hi, ':');

if span_hi != param.span && span_with_colon != span_hi {
err.tool_only_span_suggestion(
span_with_colon,
MSG_RESTRICT_BOUND_FURTHER,
format!("{}: {} + ", param_name, constraint),
Applicability::MachineApplicable,
);
}
} 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_help(
param.span,
&format!("{} `{}: {}`", MSG_RESTRICT_TYPE, param_name, constraint),
);

err.tool_only_span_suggestion(
param.span,
MSG_RESTRICT_TYPE,
format!("{}: {}", param_name, 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().shrink_to_hi();

match &param_spans[..] {
&[] => {
err.span_help(
param.span,
&format!("{} `where {}: {}`", MSG_RESTRICT_TYPE, param_name, constraint),
);

err.tool_only_span_suggestion(
where_clause_span,
MSG_RESTRICT_TYPE,
format!(", {}: {}", param_name, constraint),
Applicability::MachineApplicable,
);
}

&[&param_span] => {
err.span_help(
param_span,
&format!("{} `+ {}`", MSG_RESTRICT_BOUND_FURTHER, constraint),
);

let span_hi = param_span.with_hi(span.hi());
let span_with_colon = source_map.span_through_char(span_hi, ':');

if span_hi != param_span && span_with_colon != span_hi {
err.tool_only_span_suggestion(
span_with_colon,
MSG_RESTRICT_BOUND_FURTHER,
format!("{}: {} +", param_name, constraint),
Applicability::MachineApplicable,
);
}
}

_ => {
err.span_help(
param.span,
&format!(
"{} `where {}: {}`",
MSG_RESTRICT_TYPE_FURTHER, param_name, constraint,
),
);

err.tool_only_span_suggestion(
where_clause_span,
MSG_RESTRICT_BOUND_FURTHER,
format!(", {}: {}", param_name, constraint),
Applicability::MachineApplicable,
);
}
}

true
}
}
92 changes: 8 additions & 84 deletions src/librustc/traits/error_reporting/suggestions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ use super::{
};

use crate::infer::InferCtxt;
use crate::traits::error_reporting::suggest_constraining_type_param;
use crate::traits::object_safety::object_safety_violations;
use crate::ty::TypeckTables;
use crate::ty::{self, AdtKind, DefIdTree, ToPredicate, Ty, TyCtxt, TypeFoldable, WithConstness};

use rustc_errors::{
error_code, pluralize, struct_span_err, Applicability, DiagnosticBuilder, Style,
};
Expand All @@ -16,7 +16,6 @@ use rustc_hir::def::DefKind;
use rustc_hir::def_id::DefId;
use rustc_hir::intravisit::Visitor;
use rustc_hir::Node;
use rustc_span::source_map::SourceMap;
use rustc_span::symbol::{kw, sym};
use rustc_span::{MultiSpan, Span, DUMMY_SP};
use std::fmt;
Expand Down Expand Up @@ -430,12 +429,16 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
.span_take_while(span, |c| c.is_whitespace() || *c == '&');

let remove_refs = refs_remaining + 1;
let format_str =
format!("consider removing {} leading `&`-references", remove_refs);

let msg = if remove_refs == 1 {
"consider removing the leading `&`-reference".to_string()
} else {
format!("consider removing {} leading `&`-references", remove_refs)
};

err.span_suggestion_short(
sp,
&format_str,
&msg,
String::new(),
Applicability::MachineApplicable,
);
Expand Down Expand Up @@ -1652,85 +1655,6 @@ impl<'a, 'tcx> InferCtxt<'a, '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,
source_map: &SourceMap,
span: Span,
def_id: Option<DefId>,
) -> bool {
let restrict_msg = "consider further restricting this bound";
if let Some(param) =
generics.params.iter().filter(|p| p.name.ident().as_str() == param_name).next()
{
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),
);
} else if param_name.starts_with("impl ") {
// `impl Trait` in argument:
// `fn foo(x: impl Trait) {}` → `fn foo(t: impl Trait + Trait2) {}`
err.span_suggestion(
param.span,
restrict_msg,
// `impl CurrentTrait + MissingTrait`
format!("{} + {}", param_name, constraint),
Applicability::MachineApplicable,
);
} else if generics.where_clause.predicates.is_empty() && param.bounds.is_empty() {
// If there are no bounds whatsoever, suggest adding a constraint
// to the type parameter:
// `fn foo<T>(t: T) {}` → `fn foo<T: Trait>(t: T) {}`
err.span_suggestion(
param.span,
"consider restricting this bound",
format!("{}: {}", param_name, constraint),
Applicability::MachineApplicable,
);
} else if !generics.where_clause.predicates.is_empty() {
// There is a `where` clause, so suggest expanding it:
// `fn foo<T>(t: T) where T: Debug {}` →
// `fn foo<T>(t: T) where T: Debug, T: Trait {}`
err.span_suggestion(
generics.where_clause.span().unwrap().shrink_to_hi(),
&format!("consider further restricting type parameter `{}`", param_name),
format!(", {}: {}", param_name, constraint),
Applicability::MachineApplicable,
);
} else {
// If there is no `where` clause lean towards constraining to the
// type parameter:
// `fn foo<X: Bar, T>(t: T, x: X) {}` → `fn foo<T: Trait>(t: T) {}`
// `fn foo<T: Bar>(t: T) {}` → `fn foo<T: Bar + Trait>(t: T) {}`
let sp = param.span.with_hi(span.hi());
let span = source_map.span_through_char(sp, ':');
if sp != param.span && sp != span {
// Only suggest if we have high certainty that the span
// covers the colon in `foo<T: Trait>`.
err.span_suggestion(
span,
restrict_msg,
format!("{}: {} + ", param_name, constraint),
Applicability::MachineApplicable,
);
} else {
err.span_label(
param.span,
&format!("consider adding a `where {}: {}` bound", param_name, constraint),
);
}
}
return true;
}
false
}

/// Collect all the returned expressions within the input expression.
/// Used to point at the return spans when we want to suggest some change to them.
#[derive(Default)]
Expand Down
12 changes: 11 additions & 1 deletion src/librustc_hir/hir.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,16 @@ pub struct GenericParam<'hir> {
pub kind: GenericParamKind<'hir>,
}

impl GenericParam<'hir> {
pub fn bounds_span(&self) -> Option<Span> {
self.bounds.iter().fold(None, |span, bound| {
let span = span.map(|s| s.to(bound.span())).unwrap_or_else(|| bound.span());

Some(span)
})
}
}

#[derive(Default)]
pub struct GenericParamCount {
pub lifetimes: usize,
Expand Down Expand Up @@ -513,7 +523,7 @@ pub enum SyntheticTyParamKind {
#[derive(RustcEncodable, RustcDecodable, Debug, HashStable_Generic)]
pub struct WhereClause<'hir> {
pub predicates: &'hir [WherePredicate<'hir>],
// Only valid if predicates isn't empty.
// Only valid if predicates aren't empty.
pub span: Span,
}

Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use rustc::mir::{
FakeReadCause, Local, LocalDecl, LocalInfo, LocalKind, Location, Operand, Place, PlaceRef,
ProjectionElem, Rvalue, Statement, StatementKind, TerminatorKind, VarBindingForm,
};
use rustc::traits::error_reporting::suggestions::suggest_constraining_type_param;
use rustc::traits::error_reporting::suggest_constraining_type_param;
use rustc::ty::{self, Ty};
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::{Applicability, DiagnosticBuilder};
Expand Down
Loading

0 comments on commit 840bdc3

Please sign in to comment.