Fix malformed suggestion for E0061 when method is a macro token in macro context

[Kivooeo]

fixes #140512

before

3  -         <Self>::$method(8)
3  +         <Self>::<Self>::$method(8, /* u8 */)

now

3  |         <Self>::$method(8, /* u8 */)
   |                          ++++++++++

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
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

[mejrs]

Just checking if it starts with "<Self>::" will not work for the general case. Imagine something like

macro_rules! delegate {
    ($from:ident, $method:ident) => {
        <$from>::$method(8)
    }
}

struct Bar;

impl Bar {
    fn foo(a: u8, b: u8) {
    
    }
    fn bar() {
        delegate!(Bar, foo);
    }
}

help: provide the argument
   |
3  -         <$from>::$method(8)
3  +         <$from>::<$from>::$method(8, /* u8 */)
   |

It would be better to just find the arg span (i.e. (8) in this example). Most likely you will need to find a better way to do what

let (mut suggestion, suggestion_span) = if let Some(call_span) =
                full_call_span.find_ancestor_inside_same_ctxt(error_span)
            {
                ("(".to_string(), call_span.shrink_to_hi().to(error_span.shrink_to_hi()))
            }

does.

[Kivooeo]

@mejrs
the problem that suggestion var for macro looks like this
&suggestion = "<Self>::$method(" when for non-macro like this &suggestion = "("
but i made some changes, now should be more flexible

[mejrs]

Okay, I've checked this out and I think the best solution is to just simplify this entire construct. For some reason this diagnostic insists on getting the span of the function/method and then writing down the name again. We really need the span of the arguments, so let's do just that:

Let's delete this conditional:

let (mut suggestion, suggestion_span) = if let Some(call_span) =
                full_call_span.find_ancestor_inside_same_ctxt(error_span)
            {
                   //...
            } else {
                // ...
            };

And instead write:

            let suggestion_span = if let Some(args_span) = error_span.trim_start(full_call_span) {
                // Span of the braces, e.g. `(a, b, c)`.
                args_span
            } else {
                // The arg span of a function call that wasn't even given braces
                // like what might happen with delegation reuse.
                // e.g. `reuse HasSelf::method;` should suggest `reuse HasSelf::method($args);`.
                full_call_span.shrink_to_hi()
            };

            let mut suggestion = String::from("(");

[Kivooeo]

@mejrs yep, your solution runs like perfect, so I will leave it like this with comments because I found them informative for rust developers in future, and yeah, really wondering why this was overcomplicated initially

[mejrs]

I'm not sure. It feels like the difference between "this is the code you need to write" and "this is the change you need to make". So perhaps that has changed in the diagnostics machinery since the original code was written?

Can you update the test with a case where the delegate macro is defined in another crate? That's where this suggestion should not be emitted; we should avoid giving suggestions that would require the user to modify a dependency. See https://rustc-dev-guide.rust-lang.org/tests/compiletest.html#building-auxiliary-crates for how to do that.

[Kivooeo]

@mejrs should be fine now

[mejrs]

@mejrs should be fine now

This is not what I meant. The original test (with the macro defined in the same file) is fine. You should add a test where the macro is defined in another crate. And in this case the compiler should not give the suggestion (but it currently does). Most likely you'll need to check https://doc.rust-lang.org/nightly/nightly-rustc/rustc_span/struct.Span.html#method.in_external_macro

[Kivooeo]

@mejrs let me check if get you correctly, so if macro is located in foreign crate, we should skip this suggestion, right?

[mejrs]

@mejrs let me check if get you correctly, so if macro is located in foreign crate, we should skip this suggestion, right?

Right, we should avoid suggestions that would involve modifying a dependency.

[davidtwco]

Can you add a test like #140591 (review) where it isn't explicitly <Self> too?

[Kivooeo]

@davidtwco mind if i add only one test case with from?