Skip to content

Suggest defining type parameter when appropriate #68447

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 27, 2020
Merged

Suggest defining type parameter when appropriate #68447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions src/librustc_resolve/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1415,11 +1415,15 @@ crate fn show_candidates(
better: bool,
found_use: bool,
) {
if candidates.is_empty() {
return;
}
// we want consistent results across executions, but candidates are produced
// by iterating through a hash map, so make sure they are ordered:
let mut path_strings: Vec<_> =
candidates.into_iter().map(|c| path_names_to_string(&c.path)).collect();
path_strings.sort();
path_strings.dedup();

let better = if better { "better " } else { "" };
let msg_diff = match path_strings.len() {
Expand All @@ -1444,6 +1448,7 @@ crate fn show_candidates(
msg.push('\n');
msg.push_str(&candidate);
}
err.note(&msg);
}
}

Expand Down
110 changes: 61 additions & 49 deletions src/librustc_resolve/late.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ impl<'a> PathSource<'a> {
}

#[derive(Default)]
struct DiagnosticMetadata {
struct DiagnosticMetadata<'ast> {
/// The current trait's associated types' ident, used for diagnostic suggestions.
current_trait_assoc_types: Vec<Ident>,

Expand All @@ -333,6 +333,13 @@ struct DiagnosticMetadata {
/// The current self item if inside an ADT (used for better errors).
current_self_item: Option<NodeId>,

/// The current trait (used to suggest).
current_item: Option<&'ast Item>,

/// When processing generics and encountering a type not found, suggest introducing a type
/// param.
currently_processing_generics: bool,

/// The current enclosing function (used for better errors).
current_function: Option<Span>,

Expand All @@ -347,7 +354,7 @@ struct DiagnosticMetadata {
current_let_binding: Option<(Span, Option<Span>, Option<Span>)>,
}

struct LateResolutionVisitor<'a, 'b> {
struct LateResolutionVisitor<'a, 'b, 'ast> {
r: &'b mut Resolver<'a>,

/// The module that represents the current item scope.
Expand All @@ -364,30 +371,32 @@ struct LateResolutionVisitor<'a, 'b> {
current_trait_ref: Option<(Module<'a>, TraitRef)>,

/// Fields used to add information to diagnostic errors.
diagnostic_metadata: DiagnosticMetadata,
diagnostic_metadata: DiagnosticMetadata<'ast>,
}

/// Walks the whole crate in DFS order, visiting each item, resolving names as it goes.
impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> {
fn visit_item(&mut self, item: &'tcx Item) {
impl<'a, 'ast> Visitor<'ast> for LateResolutionVisitor<'a, '_, 'ast> {
fn visit_item(&mut self, item: &'ast Item) {
let prev = replace(&mut self.diagnostic_metadata.current_item, Some(item));
self.resolve_item(item);
self.diagnostic_metadata.current_item = prev;
}
fn visit_arm(&mut self, arm: &'tcx Arm) {
fn visit_arm(&mut self, arm: &'ast Arm) {
self.resolve_arm(arm);
}
fn visit_block(&mut self, block: &'tcx Block) {
fn visit_block(&mut self, block: &'ast Block) {
self.resolve_block(block);
}
fn visit_anon_const(&mut self, constant: &'tcx AnonConst) {
fn visit_anon_const(&mut self, constant: &'ast AnonConst) {
debug!("visit_anon_const {:?}", constant);
self.with_constant_rib(|this| {
visit::walk_anon_const(this, constant);
});
}
fn visit_expr(&mut self, expr: &'tcx Expr) {
fn visit_expr(&mut self, expr: &'ast Expr) {
self.resolve_expr(expr, None);
}
fn visit_local(&mut self, local: &'tcx Local) {
fn visit_local(&mut self, local: &'ast Local) {
let local_spans = match local.pat.kind {
// We check for this to avoid tuple struct fields.
PatKind::Wild => None,
Expand All @@ -401,7 +410,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> {
self.resolve_local(local);
self.diagnostic_metadata.current_let_binding = original;
}
fn visit_ty(&mut self, ty: &'tcx Ty) {
fn visit_ty(&mut self, ty: &'ast Ty) {
match ty.kind {
TyKind::Path(ref qself, ref path) => {
self.smart_resolve_path(ty.id, qself.as_ref(), path, PathSource::Type);
Expand All @@ -417,7 +426,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> {
}
visit::walk_ty(self, ty);
}
fn visit_poly_trait_ref(&mut self, tref: &'tcx PolyTraitRef, m: &'tcx TraitBoundModifier) {
fn visit_poly_trait_ref(&mut self, tref: &'ast PolyTraitRef, m: &'ast TraitBoundModifier) {
self.smart_resolve_path(
tref.trait_ref.ref_id,
None,
Expand All @@ -426,7 +435,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> {
);
visit::walk_poly_trait_ref(self, tref, m);
}
fn visit_foreign_item(&mut self, foreign_item: &'tcx ForeignItem) {
fn visit_foreign_item(&mut self, foreign_item: &'ast ForeignItem) {
match foreign_item.kind {
ForeignItemKind::Fn(_, ref generics) => {
self.with_generic_param_rib(generics, ItemRibKind(HasGenericParams::Yes), |this| {
Expand All @@ -443,7 +452,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> {
}
}
}
fn visit_fn(&mut self, fn_kind: FnKind<'tcx>, declaration: &'tcx FnDecl, sp: Span, _: NodeId) {
fn visit_fn(&mut self, fn_kind: FnKind<'ast>, declaration: &'ast FnDecl, sp: Span, _: NodeId) {
let previous_value = replace(&mut self.diagnostic_metadata.current_function, Some(sp));
debug!("(resolving function) entering function");
let rib_kind = match fn_kind {
Expand Down Expand Up @@ -472,7 +481,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> {
self.diagnostic_metadata.current_function = previous_value;
}

fn visit_generics(&mut self, generics: &'tcx Generics) {
fn visit_generics(&mut self, generics: &'ast Generics) {
// For type parameter defaults, we have to ban access
// to following type parameters, as the InternalSubsts can only
// provide previous type parameters as they're built. We
Expand Down Expand Up @@ -534,11 +543,12 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> {
}
}

fn visit_generic_arg(&mut self, arg: &'tcx GenericArg) {
fn visit_generic_arg(&mut self, arg: &'ast GenericArg) {
debug!("visit_generic_arg({:?})", arg);
let prev = replace(&mut self.diagnostic_metadata.currently_processing_generics, true);
match arg {
GenericArg::Type(ref ty) => {
// We parse const arguments as path types as we cannot distiguish them durring
// We parse const arguments as path types as we cannot distiguish them during
// parsing. We try to resolve that ambiguity by attempting resolution the type
// namespace first, and if that fails we try again in the value namespace. If
// resolution in the value namespace succeeds, we have an generic const argument on
Expand All @@ -556,7 +566,6 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> {
)
.is_some()
};

if !check_ns(TypeNS) && check_ns(ValueNS) {
// This must be equivalent to `visit_anon_const`, but we cannot call it
// directly due to visitor lifetimes so we have to copy-paste some code.
Expand All @@ -574,6 +583,7 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> {
this.visit_path(path, ty.id);
});

self.diagnostic_metadata.currently_processing_generics = prev;
return;
}
}
Expand All @@ -584,11 +594,12 @@ impl<'a, 'tcx> Visitor<'tcx> for LateResolutionVisitor<'a, '_> {
GenericArg::Lifetime(lt) => self.visit_lifetime(lt),
GenericArg::Const(ct) => self.visit_anon_const(ct),
}
self.diagnostic_metadata.currently_processing_generics = prev;
}
}

impl<'a, 'b> LateResolutionVisitor<'a, '_> {
fn new(resolver: &'b mut Resolver<'a>) -> LateResolutionVisitor<'a, 'b> {
impl<'a, 'b, 'ast> LateResolutionVisitor<'a, 'b, 'ast> {
fn new(resolver: &'b mut Resolver<'a>) -> LateResolutionVisitor<'a, 'b, 'ast> {
// During late resolution we only track the module component of the parent scope,
// although it may be useful to track other components as well for diagnostics.
let graph_root = resolver.graph_root;
Expand Down Expand Up @@ -724,7 +735,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
None
}

fn resolve_adt(&mut self, item: &Item, generics: &Generics) {
fn resolve_adt(&mut self, item: &'ast Item, generics: &'ast Generics) {
debug!("resolve_adt");
self.with_current_self_item(item, |this| {
this.with_generic_param_rib(generics, ItemRibKind(HasGenericParams::Yes), |this| {
Expand Down Expand Up @@ -778,7 +789,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
}
}

fn resolve_item(&mut self, item: &Item) {
fn resolve_item(&mut self, item: &'ast Item) {
let name = item.ident.name;
debug!("(resolving item) resolving {} ({:?})", name, item.kind);

Expand Down Expand Up @@ -1024,16 +1035,15 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
let mut new_id = None;
if let Some(trait_ref) = opt_trait_ref {
let path: Vec<_> = Segment::from_path(&trait_ref.path);
let res = self
.smart_resolve_path_fragment(
trait_ref.ref_id,
None,
&path,
trait_ref.path.span,
PathSource::Trait(AliasPossibility::No),
CrateLint::SimplePath(trait_ref.ref_id),
)
.base_res();
let res = self.smart_resolve_path_fragment(
trait_ref.ref_id,
None,
&path,
trait_ref.path.span,
PathSource::Trait(AliasPossibility::No),
CrateLint::SimplePath(trait_ref.ref_id),
);
let res = res.base_res();
if res != Res::Err {
new_id = Some(res.def_id());
let span = trait_ref.path.span;
Expand Down Expand Up @@ -1070,11 +1080,11 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {

fn resolve_implementation(
&mut self,
generics: &Generics,
opt_trait_reference: &Option<TraitRef>,
self_type: &Ty,
generics: &'ast Generics,
opt_trait_reference: &'ast Option<TraitRef>,
self_type: &'ast Ty,
item_id: NodeId,
impl_items: &[AssocItem],
impl_items: &'ast [AssocItem],
) {
debug!("resolve_implementation");
// If applicable, create a rib for the type parameters.
Expand Down Expand Up @@ -1179,7 +1189,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
}
}

fn resolve_params(&mut self, params: &[Param]) {
fn resolve_params(&mut self, params: &'ast [Param]) {
let mut bindings = smallvec![(PatBoundCtx::Product, Default::default())];
for Param { pat, ty, .. } in params {
self.resolve_pattern(pat, PatternSource::FnParam, &mut bindings);
Expand All @@ -1188,7 +1198,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
}
}

fn resolve_local(&mut self, local: &Local) {
fn resolve_local(&mut self, local: &'ast Local) {
// Resolve the type.
walk_list!(self, visit_ty, &local.ty);

Expand Down Expand Up @@ -1307,7 +1317,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
}

/// Check the consistency of the outermost or-patterns.
fn check_consistent_bindings_top(&mut self, pat: &Pat) {
fn check_consistent_bindings_top(&mut self, pat: &'ast Pat) {
pat.walk(&mut |pat| match pat.kind {
PatKind::Or(ref ps) => {
self.check_consistent_bindings(ps);
Expand All @@ -1317,7 +1327,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
})
}

fn resolve_arm(&mut self, arm: &Arm) {
fn resolve_arm(&mut self, arm: &'ast Arm) {
self.with_rib(ValueNS, NormalRibKind, |this| {
this.resolve_pattern_top(&arm.pat, PatternSource::Match);
walk_list!(this, visit_expr, &arm.guard);
Expand All @@ -1326,14 +1336,14 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
}

/// Arising from `source`, resolve a top level pattern.
fn resolve_pattern_top(&mut self, pat: &Pat, pat_src: PatternSource) {
fn resolve_pattern_top(&mut self, pat: &'ast Pat, pat_src: PatternSource) {
let mut bindings = smallvec![(PatBoundCtx::Product, Default::default())];
self.resolve_pattern(pat, pat_src, &mut bindings);
}

fn resolve_pattern(
&mut self,
pat: &Pat,
pat: &'ast Pat,
pat_src: PatternSource,
bindings: &mut SmallVec<[(PatBoundCtx, FxHashSet<Ident>); 1]>,
) {
Expand Down Expand Up @@ -1544,7 +1554,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
id: NodeId,
qself: Option<&QSelf>,
path: &Path,
source: PathSource<'_>,
source: PathSource<'ast>,
) {
self.smart_resolve_path_fragment(
id,
Expand All @@ -1562,7 +1572,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
qself: Option<&QSelf>,
path: &[Segment],
span: Span,
source: PathSource<'_>,
source: PathSource<'ast>,
crate_lint: CrateLint,
) -> PartialRes {
let ns = source.namespace();
Expand All @@ -1573,7 +1583,9 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
let def_id = this.parent_scope.module.normal_ancestor_id;
let node_id = this.r.definitions.as_local_node_id(def_id).unwrap();
let better = res.is_some();
this.r.use_injections.push(UseError { err, candidates, node_id, better });
let suggestion =
if res.is_none() { this.report_missing_type_error(path) } else { None };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, it's now in smart_resolve_path, I like it much better.
This is what I though about in #68447 (comment) basically, except the reporting condition is different.
As long as it's contained inside diagnostics.rs I don't care too much about the precise conditions though.

this.r.use_injections.push(UseError { err, candidates, node_id, better, suggestion });
PartialRes::new(Res::Err)
};

Expand Down Expand Up @@ -1838,11 +1850,11 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
}
}

fn resolve_labeled_block(&mut self, label: Option<Label>, id: NodeId, block: &Block) {
fn resolve_labeled_block(&mut self, label: Option<Label>, id: NodeId, block: &'ast Block) {
self.with_resolved_label(label, id, |this| this.visit_block(block));
}

fn resolve_block(&mut self, block: &Block) {
fn resolve_block(&mut self, block: &'ast Block) {
debug!("(resolving block) entering block");
// Move down in the graph, if there's an anonymous module rooted here.
let orig_module = self.parent_scope.module;
Expand Down Expand Up @@ -1885,7 +1897,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
debug!("(resolving block) leaving block");
}

fn resolve_expr(&mut self, expr: &Expr, parent: Option<&Expr>) {
fn resolve_expr(&mut self, expr: &'ast Expr, parent: Option<&'ast Expr>) {
// First, record candidate traits for this expression if it could
// result in the invocation of a method call.

Expand Down Expand Up @@ -2023,7 +2035,7 @@ impl<'a, 'b> LateResolutionVisitor<'a, '_> {
}
}

fn record_candidate_traits_for_expr_if_necessary(&mut self, expr: &Expr) {
fn record_candidate_traits_for_expr_if_necessary(&mut self, expr: &'ast Expr) {
match expr.kind {
ExprKind::Field(_, ident) => {
// FIXME(#6890): Even though you can't treat a method like a
Expand Down
Loading