Do not suggest importing hash_map::HashMap or btree_map::BTreeMap

@ayushmishra2005

struct S {
    m: HashMap<String, ()>,
}

error[E0412]: cannot find type `HashMap` in this scope
 --> src/main.rs:2:8
  |
2 |     m: HashMap<String, ()>,
  |        ^^^^^^^ not found in this scope
  |
help: consider importing one of these items
  |
1 | use std::collections::HashMap;
  |
1 | use std::collections::hash_map::HashMap;
  |

The std::collections::hash_map::HashMap suggestion here is just noise, it's (almost?) never what we actually want the person to import.

This issue has been assigned to @ayushmishra2005 via this comment.

@rustbot

@rustbot claim

Remove noisy suggestion of hash_map rust-lang#72642

@ayushmishra2005

@ayushmishra2005 responding to your comment here rather than on the PR - here are some rough pointers of what I'd look into (not having dug much into this myself, I can't say exactly what will be the solution):

So, let's start at the beginning - the suggestion that we see comes from this line:

rust/src/librustc_resolve/diagnostics.rs

Line 1498 in 0aa6751

let msg = format!("consider importing {} {}{}", determiner, kind, instead);

You can find that yourself using grep or just search on GitHub for the wording used in the suggestion (or some subset of it - as you can see, a bunch of this particular message is format strings). It looks like it gets the suggestions to print from a path_strings variable, which itself comes from the candidates argument.

That's in a function called show_candidates, searching for that - it's used once, here:

rust/src/librustc_resolve/lib.rs

Lines 2579 to 2591 in f6072ca

    
           fn report_with_use_injections(&mut self, krate: &Crate) { 
        
               for UseError { mut err, candidates, def_id, better, suggestion } in 
        
                   self.use_injections.drain(..) 
        
               { 
        
                   let (span, found_use) = UsePlacementFinder::check(&self.definitions, krate, def_id); 
        
                   if !candidates.is_empty() { 
        
                       diagnostics::show_candidates(&mut err, span, &candidates, better, found_use); 
        
                   } else if let Some((span, msg, sugg, appl)) = suggestion { 
        
                       err.span_suggestion(span, msg, sugg, appl); 
        
                   } 
        
                   err.emit(); 
        
               } 
        
           }

We can see that the candidates come from deconstructing a UseError struct. Fortunately, searching for UseError turns up only one location where it is constructed:

rust/src/librustc_resolve/late.rs

Lines 1621 to 1629 in 91fb72a

    
           let report_errors = |this: &mut Self, res: Option<Res>| { 
        
               let (err, candidates) = this.smart_resolve_report_errors(path, span, source, res); 
        
               let def_id = this.parent_scope.module.normal_ancestor_id; 
        
               let better = res.is_some(); 
        
               let suggestion = 
        
                   if res.is_none() { this.report_missing_type_error(path) } else { None }; 
        
               this.r.use_injections.push(UseError { err, candidates, def_id, better, suggestion }); 
        
               PartialRes::new(Res::Err) 
        
           };

Going even further back, we see that candidates is the result of calling smart_resolve_report_errors (defined here) - looking through that function, we can see that candidates is produced by calling lookup_import_candidates and then filtering the results (making sure that the suggestion isn't what we think the name already is and know to be incorrect):

rust/src/librustc_resolve/late/diagnostics.rs

Lines 213 to 223 in f93bb2a

    
           let candidates = self 
        
               .r 
        
               .lookup_import_candidates(ident, ns, is_expected) 
        
               .drain(..) 
        
               .filter(|ImportSuggestion { did, .. }| { 
        
                   match (did, res.and_then(|res| res.opt_def_id())) { 
        
                       (Some(suggestion_did), Some(actual_did)) => *suggestion_did != actual_did, 
        
                       _ => true, 
        
                   } 
        
               }) 
        
               .collect::<Vec<_>>();

lookup_import_candidates (defined here) basically just calls lookup_import_candidates_from_module (defined here).

A rough read of this function suggests that it starts at some arbitrary starting module, looks over every name defined in that module for names that match what the user wrote, and if the current name is itself a module, it adds that to the list of modules to search.

I hope that you can see how this could start with something like std; and then look at std::collections, and std::fmt; and then std::collections::HashMap - and that matches the name that the user wrote, so it adds it as as candidate - and std::collections::HashSet - and that doesn't match the name, and so on; and then eventually std::collections::hash_map; before finding std::collections::hash_map::HashMap - which also matches the name that the user wrote! So we've ended up with two suggestions - it just also happens that they are suggesting the same thing - std::collections::HashMap is a re-export of std::collections::hash_map::HashMap.

I suspect that we could determine that both of these items are actually the same by comparing their DefIds - which this code finds just before adding the candidate:

rust/src/librustc_resolve/diagnostics.rs

Lines 679 to 683 in 0aa6751

    
           let did = match res { 
        
               Res::Def(DefKind::Ctor(..), did) => this.parent(did), 
        
               _ => res.opt_def_id(), 
        
           }; 
        
           candidates.push(ImportSuggestion { did, descr: res.descr(), path });

So what I'd probably end up doing is experimenting with how expensive it would be to iterate over each of the candidates before adding a new candidate (we know an error occurs at this point, so we've got some room to impact perf), and checking for duplicate suggestions; or maybe accumulating suggestions into a HashMap before converting to a Vec (using the DefId as a key, only replacing the value if the path would be shorter?).

edit: oh, and check out the rustc-dev-guide for getting your local environment set-up and as a reference.

@davidtwco

@davidtwco Great Thanks. Let me work on it

Rollup merge of rust-lang#72947 - ayushmishra2005:remove_noisy_sugges…

Rollup merge of rust-lang#73023 - ayushmishra2005:remove_noisy_sugges…

Navigation Menu

Repository navigation

Do not suggest importing hash_map::HashMap or btree_map::BTreeMap #72642

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	fn report_with_use_injections(&mut self, krate: &Crate) {
	for UseError { mut err, candidates, def_id, better, suggestion } in
	self.use_injections.drain(..)
	{
	let (span, found_use) = UsePlacementFinder::check(&self.definitions, krate, def_id);
	if !candidates.is_empty() {
	diagnostics::show_candidates(&mut err, span, &candidates, better, found_use);
	} else if let Some((span, msg, sugg, appl)) = suggestion {
	err.span_suggestion(span, msg, sugg, appl);
	}
	err.emit();
	}
	}

	let report_errors = \|this: &mut Self, res: Option<Res>\| {
	let (err, candidates) = this.smart_resolve_report_errors(path, span, source, res);
	let def_id = this.parent_scope.module.normal_ancestor_id;
	let better = res.is_some();
	let suggestion =
	if res.is_none() { this.report_missing_type_error(path) } else { None };
	this.r.use_injections.push(UseError { err, candidates, def_id, better, suggestion });
	PartialRes::new(Res::Err)
	};

	let candidates = self
	.r
	.lookup_import_candidates(ident, ns, is_expected)
	.drain(..)
	.filter(\|ImportSuggestion { did, .. }\| {
	match (did, res.and_then(\|res\| res.opt_def_id())) {
	(Some(suggestion_did), Some(actual_did)) => *suggestion_did != actual_did,
	_ => true,
	}
	})
	.collect::<Vec<_>>();

	let did = match res {
	Res::Def(DefKind::Ctor(..), did) => this.parent(did),
	_ => res.opt_def_id(),
	};
	candidates.push(ImportSuggestion { did, descr: res.descr(), path });

Do not suggest importing hash_map::HashMap or btree_map::BTreeMap #72642

Description

Activity

ayushmishra2005 commented on May 27, 2020

davidtwco commented on Jun 2, 2020

ayushmishra2005 commented on Jun 3, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions