Add a diagnostic for similarly named traits

[rperier]

cc #133123

This is a first proposal, suggestions are welcome

[rustbot]

r? @lcnr

rustbot has assigned @lcnr.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

This PR modifies run-make tests.

cc @jieyouxu

[rperier]

Fixed by using predicate_must_hold_modulo_regions

[lcnr]

skip_binder is pretty much never correct:

https://rustc-dev-guide.rust-lang.org/ty_module/early_binder.html?highlight=Binder#earlybinder-and-instantiating-parameters

also see https://rustc-dev-guide.rust-lang.org/typing_parameter_envs.html

U will also need to properly handle the generic parameters of the traits here, e.g. what if we've got trait Trait {} and a separate trait Trait<T> {}

Also, check predicate must hold for the other trait, not per impl.

[rperier]

Mhhhh, I see. I have to document myself about this part, thanks for the links.

Also, check predicate must hold for the other trait, not per impl.

Would you have a usecase for this ? You're probably right, but I don't get it.

thanks !

[lcnr]

well, right now given other::Trait with 2 impls, for each impl you separately check this_trait_self_ty: other::Trait. This is unnecessary, we simply care about

does this_trait_self_ty implement other::Trait

The reason why it implements that trait doens't really matter for us. My usecase is that doing it per impls is unnecessary work and more involved to implement than the alternative.

[rperier]

The current version is the following. I no longer browse the impls, so it is no longer per impl. It's mostly working (other::Trait or other::Trait<T> are working), however I still get weird behaviours with super traits and the GenericArgs . That's why I did not push the commit yet, some tests are broken, I am investigating.

UPDATE: switch to the last commit directly and see update below regarding generics and ICE

pub(crate) fn suggest_impl_similarly_named_trait(
        &self,
        err: &mut Diag<'_>,
        obligation: &PredicateObligation<'tcx>,
        trait_predicate: ty::PolyTraitPredicate<'tcx>,
    ) {
        let trait_def_id = trait_predicate.def_id();
        let trait_name = self.tcx.item_name(trait_def_id);
        if let Some(other_trait_def_id) = self.tcx.all_traits_including_private().find(|def_id| {
            if trait_def_id != *def_id && trait_name == self.tcx.item_name(def_id) {
                if let Some(pred) = self.tcx.predicates_of(*def_id).instantiate(self.tcx, trait_predicate.skip_binder().trait_ref.args).predicates.iter().find(|clause| {
                    clause.as_trait_clause().map_or(false, |trait_clause| trait_clause.def_id() == *def_id)
                })
                {
                    let pred = pred.as_trait_clause().unwrap();
                    self.predicate_must_hold_modulo_regions(&Obligation::new(
                        self.tcx,
                        obligation.cause.clone(),
                        obligation.param_env,
                        pred,
                    ))
                } else {
                    false
                }
            } else {
                false
            }
        }) {
            err.note(format!(
                "`{}` implements similarly named `{}`, but not `{}`",
                trait_predicate.self_ty(),
                self.tcx.def_path_str(other_trait_def_id),
                trait_predicate.print_modifiers_and_trait_path()
            ));
        }
        ()
    }

[rperier]

This is another proposal, feedbacks are welcomed. I am not convinced about the part regarding the generic args. The idea being that you cannot select a similarly named trait with different generics (or different constraints), it does not makes sense and might cause ICE.

[rperier]

This is another proposal, I have simplified the code. This is to show you the code before I merge it with the other suggestion. I have to analyze the other suggestion you were talking about and try to merge my code with it now.

[lcnr]

the impl lgtm now, I don't think we should have these two similar suggestions however (similarly named trait vs similarly named trait from different crate version), and believe they should be merged into 1

[rperier]

OK, thanks for your feedbacks. I will merge with the other suggestion and fix param.kind during the weekend

[rperier]

Merged into the suggestion we have discussed above. I have kept each note/notice independent, because I have the feeling that the three notes/notices might still be emitted independently when you think about it. You might have similarly named traits within the same crate but also in different crates. You might have cases when similarly named traits are found but not traits with the same path (this is typically the case in some ui tests)