"help: remove the extra argument" prints too much of the function arguments

[jyn514]

Given the following code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=d6dfe3d605639255444883813f8376db

enum CargoMessage {
    CompilerArtifact {
        filenames: Vec<String>,
        target: CargoTarget,
        extra: bool,
    }
}

struct CargoTarget {
    crate_types: Vec<String>,
}

pub fn run_cargo() {
    let is_debug_info = |_| true;
    let is_dylib = |_| true;
    let is_check = true;
    let ok = stream_cargo(vec![], &mut |msg| {
        let (filenames, crate_types) = match msg {
            CargoMessage::CompilerArtifact {
                filenames,
                target: CargoTarget { crate_types },
                ..
            } => (filenames, crate_types),
            _ => return,
        };
        for filename in filenames {
            // Skip files like executables
            if !(filename.ends_with(".rlib")
                || filename.ends_with(".lib")
                || filename.ends_with(".a")
                || is_debug_info(&filename)
                || is_dylib(&filename)
                || (is_check && filename.ends_with(".rmeta")))
            {
                continue;
            }
        }
    });
}

pub fn stream_cargo(
    cb: &mut dyn FnMut(CargoMessage),
) -> bool {
  true
}

The current output is:

error[[E0061]](https://doc.rust-lang.org/stable/error-index.html#E0061): this function takes 1 argument but 2 arguments were supplied
  --> src/lib.rs:17:14
   |
17 |     let ok = stream_cargo(vec![], &mut |msg| {
   |              ^^^^^^^^^^^^ ------ argument of type `Vec<_>` unexpected
   |
note: function defined here
  --> src/lib.rs:41:8
   |
41 | pub fn stream_cargo(
   |        ^^^^^^^^^^^^
42 |     cb: &mut dyn FnMut(CargoMessage),
   |     --------------------------------
help: remove the extra argument
   |
17 ~     let ok = stream_cargo(&mut |msg| {
18 +         let (filenames, crate_types) = match msg {
19 +             CargoMessage::CompilerArtifact {
20 +                 filenames,
21 +                 target: CargoTarget { crate_types },
22 +                 ..
23 +             } => (filenames, crate_types),
24 +             _ => return,
25 +         };
26 +         for filename in filenames {
27 +             // Skip files like executables
28 +             if !(filename.ends_with(".rlib")
29 +                 || filename.ends_with(".lib")
30 +                 || filename.ends_with(".a")
31 +                 || is_debug_info(&filename)
32 +                 || is_dylib(&filename)
33 +                 || (is_check && filename.ends_with(".rmeta")))
34 +             {
35 +                 continue;
36 +             }
37 +         }
38 ~     });
   |

For more information about this error, try `rustc --explain E0061`.

Ideally the output should look like:

error[E0061]: this function takes 1 argument but 2 arguments were supplied
  --> src/lib.rs:17:14
   |
17 |     let ok = stream_cargo(vec![], &mut |msg| {
   |              ^^^^^^^^^^^^ ------ argument of type `Vec<_>` unexpected
   |
note: function defined here
  --> src/lib.rs:41:8
   |
41 | pub fn stream_cargo(
   |        ^^^^^^^^^^^^
42 |     cb: &mut dyn FnMut(CargoMessage),
   |     --------------------------------
help: remove the extra argument
   |
17 ~     let ok = stream_cargo(&mut |msg| {

Printing the entire body of the closure is ... a bit much 😆

[jyn514]

@estebank says

Namely it should be a multipart suggestion that only touches the parts that changed, not the whole arg list.

[Ezrashaw]

@rustbot claim

[jyn514]

@Ezrashaw fyi I think @estebank said they'd already started working on this, you might want to ask him how far he's gotten

[Ezrashaw]

@Ezrashaw fyi I think @estebank said they'd already started working on this, you might want to ask him how far he's gotten

Oh, if he is working on this then that's all fine, not really much point in duplicated work.

[estebank]

@Ezrashaw I did some work on this in #106347, but it only addresses removal of arguments and I would love it if you push it further, like fixing the case where removing multiple trailing arguments leaves behind a trailing comma.

[Ezrashaw]

@Ezrashaw I did some work on this in #106347, but it only addresses removal of arguments and I would love it if you push it further, like fixing the case where removing multiple trailing arguments leaves behind a trailing comma.

Ok, I'll have a look. Might be a little out of my skill level so if you or @jyn514 could mentor be a bit that would be great. I also haven't looked at your PR yet so I assume that would be a good place to start.

@rustbot claim

[Ezrashaw]

@estebank It appears you have fixed the trailing commas?

[estebank]

I figured a cheat to get that to work (when removing remove foo, instead of , foo), but there's still a lot to do around the other suggestions, like swapping arguments and adding arguments (those still use the "full span" approach).

[Ezrashaw]

I've been looking around the relevant bits of compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs and I think I understand it (seems to have aged badly). I really need some mentoring though to understand how to implement it (I'm looking at Error::Missing/ provide the argument).

[estebank]

@Ezrashaw I would try to collect the vec with the suggestions always, and if we detect that it is the straightforward case you emit the targeted suggestion, otherwise you fallback on the current logic. I would look at swap for example: if there're only swapped arg cases, then the logic for coming up with the suggestion would be relatively easy, where you suggest one snippet in the span of the other, as many times as necessary.

[Ezrashaw]

@estebank And then the old full-span logic can be phased out eventually. I'll be working on this, just slowly because I'm new and on holiday.

[WaffleLapkin]

What is the status here? @Ezrashaw are you still working on this? (also cc #109782 as it changed how commas are worked around again)

[Ezrashaw]

I'm not sure exactly what the status is here. I will work on it at some point, but if someone else wants to then that's all good.

AFAIK, Esteban's PR was expanded a bit to cover some extra cases, however there are still some sub-optimal cases where the full span is used.

[estebank]

We should make sure we have a single test that covers all of the different cases, at the very least to notice when anyone changes the handling of these :)

[est31]

Just hit a version of this today. For the following snippet:

fn foo(a: (), b: u32) {}

fn main() {
    foo("hi", (), 42u32, ());
}

rustc gives:

error[E0061]: this function takes 2 arguments but 4 arguments were supplied
 --> src/main.rs:4:5
  |
4 |     foo("hi", (), 42u32, ());
  |     ^^^ ----             -- unexpected argument of type `()`
  |         |
  |         unexpected argument of type `&'static str`
  |
note: function defined here
 --> src/main.rs:1:4
  |
1 | fn foo(a: (), b: u32) {}
  |    ^^^ -----  ------
help: remove the extra arguments
  |
4 -     foo("hi", (), 42u32, ());
4 +     foo(, (), 42u32);
  |

For more information about this error, try `rustc --explain E0061`.

Not sure why @WaffleLapkin 's #109782 didn't work in this instance...