Provide a note if method lookup fails and there are static definitions with the same name. #13685
Conversation
| t.is_str() | ||
| //~^ ERROR type `T` does not implement any method in scope named `is_str` | ||
| //~^^ NOTE found defined static methods, did you forget a self parameter? | ||
| //~^^^ NOTE candidate #1 is `std::path::BytesContainer::is_str` |
There was a problem hiding this comment.
Could you move the dependence from libstd into this test? The standard library often changes over time and may cause this to be accidentally deleted at some point.
There was a problem hiding this comment.
Sure, I can replicate a similar test in the file. But, do you have a good reference for a test using an external crates definitions? I'd like to ensure that methods from other crates are listed.
|
cc @nikomatsakis I can't tell how badly this impacts method lookup. |
|
@eddyb Actually, scratch that, there was a reason why I couldn't use a Hashmap at one point, but looking back on this freshly I don't see whatever reason I had. So, I'll update to a |
|
Updated commit includes above feedback and is rebased on master (fixed conflicts with |
|
This looks very helpful, thanks. There's some docs here for cross-crate tests. |
| @@ -5152,7 +5154,7 @@ impl<'a> Resolver<'a> { | |||
| Some(DefTrait(trait_def_id)) => trait_def_id, | |||
| Some(..) | None => continue, | |||
| }; | |||
| if self.method_set.borrow().contains(&(name, did)) { | |||
| if self.method_map.borrow().contains_key(&(name, did)) { | |||
There was a problem hiding this comment.
These changes to resolve look like they'll start resolving static functions as candidates for methods (possibly). Can you make sure that code like this still compiles?
trait Foo {
fn new() {}
}
trait Bar {
fn new(&self) {}
}
impl Bar for int {}
impl Foo for int {}
fn main() {
1i.new();
}There was a problem hiding this comment.
This only checks for static methods on failed method lookup (and it's going to report an error regardless of finding a static, it just sugars the error), so this code will compile fine as the method lookup won't fail.
I couldn't find a runpass test for this name overlap, so I just added one for posterity. (In latest commit)
|
This looks great, thanks @Ryman! |
…s with the same name.
Closes #7575. I don't think the change from a contains lookup to an iteration of the HashSet in the resolver should be much of a burden as the set of methods with the same name should be relatively small.
Closes #7575.
I don't think the change from a contains lookup to an iteration of the HashSet in the resolver should be much of a burden as the set of methods with the same name should be relatively small.