I agree that the help message is desirable for the cross-crate case (such as the serde example in the issue), since there the solution for the user is always to change the implementation (vs. e.g. changing the trait).

But here, it feels overly verbose? We're showing come_on_a_little_more_effort three times.

I agree. In the case of local traits, the trait definition may be wrong, so it would be good to just point out that the two are different.

Could you add a test for missing and extra &self as well?

Also, please add a test like:

trait NonIdentArguments {
    fn pattern_types_in_arguments(_: i32, (_a, _b): (i32, i32)) {}
}

struct MyStruct6;
impl NonIdentArguments for MyStruct6 {
    fn pattern_types_in_arguments() {}
}

Adding or removing &self from the implementation causes E0185 and E0186 errors instead of E0050. So I don't see any suggestions working on this pr, but would it be a good idea to still add that test case?

error[E0185]: method `no_params` has a `&self` declaration in the impl, but not in the trait
  --> $DIR/trait-impl-param-mismatched-cross-crate.rs:45:5
   |
LL |     fn no_params(&self) {}
   |     ^^^^^^^^^^^^^^^^^^^ `&self` used in impl
   |
   = note: `no_params` from trait: `fn()`

error[E0186]: method `has_self` has a `&self` declaration in the trait, but not in the impl
  --> $DIR/trait-impl-param-mismatched-cross-crate.rs:48:5
   |
LL |     fn has_self() {}
   |     ^^^^^^^^^^^^^ expected `&self` in impl
   |
   = note: `has_self` from trait: `fn(&Self)`

Or would it be better to add HELP to E0185 and E0186 as well?
(I think it's going to get a bit longer)

Ah, that's fine then for now.

Maybe add a test for self: &Self as well, just to ensure that that works?

Nit: We already checked trait_number_args != impl_number_args above, this case should be unreachable.

If you want to make it clear, use:

match trait_number_args.cmp(impl_number_args) {
    Ordering::Less => { ... }
    Ordering::Greater => { ... }
    Ordering::Equal => unreachable!(),
}

But I'm fine with just doing if trait_number_args > impl_number_args { ... } else { ... }.

I think it would be better to use .cmp and unreachable! rather than if {} else {}

Instead of always making the span the entire argument list, maybe you can shrink it to only be the relevant parts?

Something like:

let (span, replacement) = if trait_number_args > impl_number_args {
    // Span is right before the end parenthesis:
    // fn foo(a: i32 ) {}
    //              ^
    let span = impl_inputs_span.shrink_to_hi();
    let replacement = ", b: u32";
    (span, replacement)
} else {
    // Span of the arguments that there are too many of:
    // fn foo(a: i32, b: u32) {}
    //              ^^^^^^^^
    let span = impl_inputs_span.with_lo(impl_m_sig.decl.inputs[trait_number_args - 1].span);
    let replacement = "";
    (span, replacement)
};

Nit: Use !trait_m.def_id.is_local() instead.

Nit: I don't think you need to guard span_to_snippet with is_dummy?

I feel like inspecting the source and looking for parentheses with find_param_bounds is brittle. For example, I think it would find the wrong span on something like:

trait Foo<T> {}
fn foo<T: Foo<()>>() {}

That said, I recognize that it's gonna be annoying to find this span in any case, because the span of the parentheses that enclose parameters seems to be dropped early in the compilation pipeline (it's not even part of ast::Fn).

Maybe a more robust approach would be to:

• If there are parameters, grab the span from the first parameter identifier and last parameter type, and combine them.
• If there are no parameters, grab the span of the output, and walk backwards until we find ) and (.

In any case, maybe you could inline find_param_bounds, that might make it easier to follow what's going on?

And maybe you could add a comment about what it is that we're actually looking for? Something like:

// Find the span of the space between the parentheses in a method.
// fn foo(...) {}
//        ^^^

To be clear, I'm not sure what's the best option here. Neither the current solution, nor the solution that I'm proposing are gonna handle every edge-case, and maybe the best would be a blend somewhere in between.

No matter what, I'd lean towards whatever solution is the simplest in terms of LOC, it is just a suggestion that we're implementing, it doesn't have to be perfect (especially given that we're using Applicability::MaybeIncorrect).

the trait is fn pattern_types_in_arguments(_: i32, (_a, _b): (i32, i32)) {}
but replacing (_a, _b) with _