New lint: ambiguous_method_names

[sjwang05]

Closes #11499

This lint checks for methods in trait impls and struct impls with the same name. See lint description for more info and examples.

changelog:
new lint: [ambiguous_method_calls]

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

• e43fedd
• 88e2716

[y21]

Is there a reason for using statics instead of storing the map directly in the AmbiguousMethodCalls struct? The lint structs can have fields and take &mut self in their check methods so its state can be accessed and mutated at any point, which means the Mutex could go too.

A problem with the current approach is that it depends on the order in which it visits the items. For example, if a Base.ambiguous(); expression appears before an ambiguous method implementation, I think it wouldn't lint on it because clippy hasn't seen it yet.

I wonder if maintaining these name maps is necessary though. Rustc already has its own maps for trait/inherent impls per crate for its analysises and typechecking, and they can be accessed through the TyCtxt.

For example, there's the all_local_trait_impls query that returns all trait impls for a given type's DefId. Maybe when visiting a method in an inherent impl, this could be used to look for traits that have methods with the same name.
I'm not sure if there's a better query to use for this... A problem that I can see here is that it probably wouldn't lint when calling ambiguous unqualified trait methods from foreign crates.

And for linting method call expressions, it could perhaps use all_local_trait_impls to get all trait impls + inherent_impls for the normal impls, then check if there's duplicate entries between the two.

But maybe having these two maps is faster over having to go through the impls and compare a bunch of Symbols that it justifies having them.

[sjwang05]

Thanks for the suggestions!

I moved the maps to be stored directly in the AmbiguousMethodCalls struct per your suggestions--I actually didn't know the lint structs could have fields, but it makes sense in retrospect.

For the visiting order problem, I decided to defer checking ambiguity to check_crate_post, so linting happens only after visiting all implementations and exprs. I feel like the current method of maintaining a map of all method calls (and implementations, for that matter) could get pretty expensive for large projects/crates, though.

As for using rustc's own maps, I'm currently trying to figure something out with the all_traits and all_local_trait_impls queries, and iterating through methods in trait impls to find those that are ambiguous with inherent methods (i.e., same name and same parent type) with something like this in check_fn:

let hir_id = cx.tcx.hir().local_def_id_to_hir_id(def_id);
let parent_id = cx.tcx.hir().get_parent_item(hir_id);
let parent_ty = cx.tcx.type_of(parent_id.to_def_id()).skip_binder();

let is_inherent_impl = !is_trait_impl_item(cx, hir_id);
if is_inherent_impl {
    for tr in cx.tcx.all_traits() {
        if implements_trait(cx, parent_ty, tr, &[]) {
            if let Some(impl_blocks) = cx.tcx.all_local_trait_impls(()).get(&tr) {
                for block in impl_blocks {
                    if let Some(hir::Node::Item(item)) =
                        cx.tcx.hir().find(cx.tcx.hir().local_def_id_to_hir_id(*block))
                    {
                        if let hir::ItemKind::Impl(impl_struct) = item.kind {
                            for item in impl_struct.items {
                                let item_hir_id =
                                    cx.tcx.hir().local_def_id_to_hir_id(item.id.owner_id.def_id);
                                let item_parent_id = cx.tcx.hir().get_parent_item(item_hir_id);
                                let item_parent_ty = cx.tcx.type_of(item_parent_id.to_def_id()).skip_binder();
                                if item.ident.name == ident.name && parent_ty == item_parent_ty{
                                    span_lint(cx, AMBIGUOUS_METHOD_CALLS, item.span, "trait impl");
                                    span_lint(cx, AMBIGUOUS_METHOD_CALLS, span, "struct impl");
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

It's still very rough, and I haven't gotten to using this approach with linting method calls yet. I'll try experimenting some more with this approach over the next few days--I definitely agree that the maps shouldn't be maintained unless needed.

A problem that I can see here is that it probably wouldn't lint when calling ambiguous unqualified trait methods from foreign crates.

I think both the current approach and the rustc-based approach fall short in this regard, unfortunately.

[y21]

so linting happens only after visiting all implementations and exprs.

Yep that's a neat way to solve it, FWIW some other lints follow the same idea. For example unused_async doesn't lint immediately once it sees an async function without .await exprs, but instead stores the relevant stuff for later + delays linting until check_crate_post so that we can be sure there aren't any path exprs to an async function in a context that requires a fn() -> impl Future.

If going with manual maps, there's one subtle but important thing: the iteration order of a HashMap is not stable, so the order in which it then emits lints can "randomly" change. That makes it annoying for e.g. ui tests where it checks for the exact stderr literally and a different order could then make the tests fail.
I ran into the exact same problem before ^^ see the relevant comment by Jarcho here #11200 (comment)

The inherent_methods field could just be a Vec, right? Seems like it's only inserted into and iterated over.

As for using rustc's own maps, I'm currently trying to figure something out with the all_traits and all_local_trait_impls queries, and iterating through methods in trait impls to find those that are ambiguous with inherent methods (i.e., same name and same parent type) with something like this in check_fn

Oh huh I was under the impression that the key of all_local_trait_impls's map was the DefId of the type and not the trait, which would have been way more convenient (wouldn't require going through all of the traits). Definitely changes the way I think about this now. All of the trait impl queries on the tyctxt seem to search by a trait id (unless I missed something), which might make this more complicated then..

[sjwang05]

The inherent_methods field could just be a Vec, right? Seems like it's only inserted into and iterated over.

Yeah, I think I was initially worried about getting the correct spans for each inherent impl (hence the self.inherent_methods.get(k)), but now that I think about it, this could also be accomplished using something like a Vec<(String, Symbol, Span)> instead of a FxHashMap as you suggested--I believe this also solves the random ordering problem.

I was under the impression that the key of all_local_trait_impls's map was the DefId of the type and not the trait

That tripped me up as well when I was working on the rustc-based approach. I think this fact would also make linting call sites a lot more difficult, though I haven’t gotten around to writing that yet—do you think it’s still worth pursuing, or should we just stick with manual maps? I’m personally more in favor of manual maps—I feel like querying all_local_trait_impls, all_traits, and inherent_impls all at the same time when linting method calls could get complicated pretty quickly.

[Jarcho]

Two inherent methods can share the same name assuming the impl blocks have different generic arguments. e.g.

struct S<T>(T);
impl S<i32> { fn f(self) {} }
impl S<u32> { fn f(self) {} }

If you aren't tracking the call sites, then I don't think you need to do anything to account for this, but I would have a test in case future changes need to do something special here.

[Jarcho]

This tracks way more than needed. You can check if the target method is an inherent method of a locally defined type before tracking it.

You'll also need to check if the method is named directly as a path.

[Jarcho]

Looks like this would be better split between check_impl_item and check_item. check_impl_item would pick up inherent methods, check_item would pick up trait impls.

Note for trait methods you'll always have to look at the trait definition to find default methods since they don't necessarily appear in the impl block.

[Jarcho]

This should be a single lint call. I would lint on the inherent method since that's the most likely one to rename. The trait can be trait method(s) can be called out in a note. The call sites are probably not worth mentioning due to the amount of unhelpful noise that it can generate.

[sjwang05]

Thanks for all the feedback!

Regarding structs with generic arguments and method call sites, I decided to remove call site linting as you suggested, so I don't think that'll be a concern for now. I've added a few test cases regardless.

I've also split up the code in check_fn to check_item and check_impl_item, and I updated check_crate_post accordingly. However, implements_trait ICEs when a trait has generic arguments and the arguments supplied to the function differ from those of the trait. I looked at other lints that used this function, but I couldn't find any relevant workarounds. Do you have any suggestions on how I should proceed? Thanks!

[sjwang05]

Disregard the part about the ICE, actually--I was able to find a fix 🙂 . I also added some tests to check for the ICE.

[bors]

☔ The latest upstream changes (presumably #11556) made this pull request unmergeable. Please resolve the merge conflicts.

[lengyijun]

FYI there is a similar lint same_name_method

[sjwang05]

Huh, I wasn't aware. If y'all think it'd be best to close this PR as a duplicate, I'm perfectly fine doing so.

[Jarcho]

@lengyijun Thanks for pointing that out. That is exactly the same thing.

@sjwang05 Sorry you got this far into the lint before this came up. That's a failure on our part for not triaging the issue properly.