More accurate spans for arg removal suggestion #106347

estebank · 2023-01-01T07:15:13Z

Partially address #106304.

rustbot · 2023-01-01T07:15:18Z

(rustbot has picked a reviewer for you, use r? to override)

petrochenkov · 2023-01-02T11:00:49Z

r? rust-lang/compiler
I'd like to take some break from reviewing.

estebank · 2023-01-02T17:23:49Z

@petrochenkov Consider removing yourself from the review rotation and readding yourself once you are ready to take them on, just to avoid spam on your end :)

Happy new year.

Ezrashaw · 2023-01-06T00:24:32Z

@estebank I have some code for the case where arguments must be added, but that code relies on this code to be merged first.

Also I think I have found a bug with this PR:

fn foo(x: &str) {}

fn main() {
    foo(1, 2, "hello");
}

emits this:

error[E0061]: this function takes 1 argument but 3 arguments were supplied
 --> <anon>:7:5
  |
7 |     foo(1, 2, "hello");
  |     ^^^ -  - argument of type `{integer}` unexpected
  |         |
  |         argument of type `{integer}` unexpected
  |
note: function defined here
 --> <anon>:2:4
  |
2 | fn foo(x: &str) {
  |    ^^^ -------
help: remove the extra arguments
  |
7 -     foo(1, 2, "hello");
7 +     foo(, "hello");
  |

Note the leading comma.

estebank · 2023-01-06T01:26:02Z

Yeah, you're right :-/

Would you have time to try and fix the bug in a subsequent PR?

Ezrashaw · 2023-01-06T02:18:10Z

Yeah, I can have a look at this. Honestly the whole error printing bit of that function needs a rewrite, it's all based around the whole-span idea. Ideally we can get this merged soon so that I can create my PRs for the additional suggestion and to fix that bug.

Ezrashaw · 2023-01-06T02:20:26Z

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

+        // Incorporate the argument changes in the removal suggestion.
+        let mut prev = -1;
+        for (expected_idx, provided_idx) in matched_inputs.iter_enumerated() {
+            if let Some(provided_idx) = provided_idx {
+                prev = provided_idx.index() as i64;
+            }
+            let idx = ProvidedIdx::from_usize((prev + 1) as usize);
+            if let None = provided_idx
+                && let Some((_, arg_span)) = provided_arg_tys.get(idx)
+            {
+                let (_, expected_ty) = formal_and_expected_inputs[expected_idx];
+                suggestions.push((*arg_span, ty_to_snippet(expected_ty, expected_idx)));
+            }
+        }


Can you explain what this does?

It's breaking my code :(

What do you mean by "breaking your code"? Do you have an example of a broken suggestion that we can put as a test?

estebank: I don't really understand what this is doing either.

It's been a while since I wrote this and now I don't recall exactly what this does, but this weird code was so that I could also incorporate the "change this argument for this other one" suggestions into the removal one, but it is quite messy :-/

The last commit now shows the effect of this block taken out on the tests.

Ezrashaw · 2023-01-09T06:14:33Z

@estebank What's happening with this PR? I intend to write some more changes but obviously they rely on this PR. Also could you look at my review comment, thanks!

estebank · 2023-01-12T02:47:12Z

@Ezrashaw this PR is awaiting review. Different reviewers have different schedules when they are available.

cjgillot · 2023-01-21T14:48:59Z

src/test/ui/argument-suggestions/extra_arguments.stderr

-   |               ~~~~~~
+LL -     1,
+LL +     1
+   |


This suggestion is missing a bit of context.

I would like to address this in a separate PR. This is a consequence of how the suggestion machinery works. I find it unreasonable to address that as part of an "unrelated" change.

cjgillot · 2023-01-21T14:49:19Z

src/test/ui/argument-suggestions/extra_arguments.stderr

-   |               ~~~~~~~
+LL -     1,
+LL +     1,
+   |


This one too.

cjgillot · 2023-01-21T14:56:43Z

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

@@ -1098,7 +1153,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                Some(format!("provide the argument{}", if plural { "s" } else { "" }))
            }
            SuggestionText::Remove(plural) => {
-                Some(format!("remove the extra argument{}", if plural { "s" } else { "" }))
+                err.multipart_suggestion_verbose(


If we are just suggesting a removal, is a verbose suggestion the best choice?
From the ui diff when arguments span multiple lines, the terse version may be better.

I tend to prefer the verbose output in general, because I feel showing the resulting code ends up being clearer, but I can change this to terse, it shouldn't be too much of an issue.

Changed. It's on its own commit so that you can review it.

cjgillot · 2023-01-21T15:03:04Z

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

+        // Incorporate the argument changes in the removal suggestion.
+        let mut prev = -1;
+        for (expected_idx, provided_idx) in matched_inputs.iter_enumerated() {
+            if let Some(provided_idx) = provided_idx {
+                prev = provided_idx.index() as i64;
+            }
+            let idx = ProvidedIdx::from_usize((prev + 1) as usize);
+            if let None = provided_idx
+                && let Some((_, arg_span)) = provided_arg_tys.get(idx)
+            {
+                let (_, expected_ty) = formal_and_expected_inputs[expected_idx];
+                suggestions.push((*arg_span, ty_to_snippet(expected_ty, expected_idx)));
+            }
+        }


What do you mean by "breaking your code"? Do you have an example of a broken suggestion that we can put as a test?

estebank: I don't really understand what this is doing either.

cjgillot · 2023-01-21T15:16:03Z

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

+            if let Some(provided_idx) = provided_idx {
+                prev = provided_idx.index() as i64;
+            }
+            let idx = ProvidedIdx::from_usize((prev + 1) as usize);
+            if let None = provided_idx
+                && let Some((_, arg_span)) = provided_arg_tys.get(idx)
+            {
+                let (_, expected_ty) = formal_and_expected_inputs[expected_idx];
+                suggestions.push((*arg_span, ty_to_snippet(expected_ty, expected_idx)));
+            }


Suggested change

if let Some(provided_idx) = provided_idx {

prev = provided_idx.index() as i64;

}

let idx = ProvidedIdx::from_usize((prev + 1) as usize);

if let None = provided_idx

&& let Some((_, arg_span)) = provided_arg_tys.get(idx)

{

let (_, expected_ty) = formal_and_expected_inputs[expected_idx];

suggestions.push((*arg_span, ty_to_snippet(expected_ty, expected_idx)));

}

if let Some(provided_idx) = provided_idx {

prev = provided_idx.index() as i64;

} else {

let idx = ProvidedIdx::from_usize((prev + 1) as usize);

if let Some((_, arg_span)) = provided_arg_tys.get(idx) {

let (_, expected_ty) = formal_and_expected_inputs[expected_idx];

suggestions.push((*arg_span, ty_to_snippet(expected_ty, expected_idx)));

}

}

?

cjgillot · 2023-01-21T15:17:24Z

src/test/ui/alloc-error/alloc-error-handler-bad-signature-3.stderr

-   |              ++
+LL - fn oom() -> ! {
+LL -     loop {}
+LL - }


This suggestion is weird too. Any idea why?

I think this is caused by the macro spans :-/

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

TaKO8Ki · 2023-02-14T05:23:26Z

Cloud you rebase the commits?

estebank · 2023-02-14T20:44:16Z

@TaKO8Ki I just rebased and pushed, but the changes were merging cleanly.

TaKO8Ki · 2023-02-16T07:38:08Z

@bors r+

bors · 2023-02-16T07:38:10Z

📌 Commit dff10d0 has been approved by TaKO8Ki

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#106347 (More accurate spans for arg removal suggestion) - rust-lang#108057 (Prevent some attributes from being merged with others on reexports) - rust-lang#108090 (`if $c:expr { Some($r:expr) } else { None }` =>> `$c.then(|| $r)`) - rust-lang#108092 (note issue for feature(packed_bundled_libs)) - rust-lang#108099 (use chars instead of strings where applicable) - rust-lang#108115 (Do not ICE on unmet trait alias bounds) - rust-lang#108125 (Add new people to the compiletest review rotation) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned petrochenkov Jan 1, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 1, 2023

estebank mentioned this pull request Jan 1, 2023

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

Open

This comment was marked as resolved.

Sign in to view

rustbot assigned TaKO8Ki and unassigned petrochenkov Jan 2, 2023

Ezrashaw reviewed Jan 7, 2023

View reviewed changes

cjgillot reviewed Jan 21, 2023

View reviewed changes

estebank force-pushed the removal-suggestion branch 2 times, most recently from 4d85fb2 to 134a305 Compare February 2, 2023 22:23

estebank added 7 commits February 14, 2023 20:22

More accurate spans for arg removal suggestion

f02d8ec

Fix fmt

fb61f5d

Avoid trailing commas

287cd59

rebase and review comments

5d63e10

Make removal suggestion not verbose

bd176ee

Show the effects of weird code commented out

755252b

Re-add replacement logic and add comment explaining it

dff10d0

estebank force-pushed the removal-suggestion branch from 134a305 to dff10d0 Compare February 14, 2023 20:44

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2023

matthiaskrgr mentioned this pull request Feb 16, 2023

Rollup of 7 pull requests #108127

Merged

bors merged commit f65c6e4 into rust-lang:master Feb 16, 2023

rustbot added this to the 1.69.0 milestone Feb 16, 2023

lukas-code mentioned this pull request Feb 19, 2023

ICE: (debug assertions) suggestion must not have overlapping part #108225

Closed

Ezrashaw mentioned this pull request Apr 24, 2023

ICE: debug assertions: E0061 suggests garbage code if struct name == fn name #110745

Open

estebank deleted the removal-suggestion branch November 9, 2023 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More accurate spans for arg removal suggestion #106347

More accurate spans for arg removal suggestion #106347

estebank commented Jan 1, 2023

rustbot commented Jan 1, 2023

This comment was marked as resolved.

petrochenkov commented Jan 2, 2023

estebank commented Jan 2, 2023

Ezrashaw commented Jan 6, 2023

estebank commented Jan 6, 2023

Ezrashaw commented Jan 6, 2023

Ezrashaw Jan 6, 2023

cjgillot Jan 21, 2023

estebank Feb 2, 2023

estebank Feb 2, 2023

Ezrashaw commented Jan 9, 2023 •

edited

Loading

estebank commented Jan 12, 2023

cjgillot Jan 21, 2023

estebank Feb 1, 2023

cjgillot Jan 21, 2023

cjgillot Jan 21, 2023

estebank Feb 1, 2023

estebank Feb 1, 2023

cjgillot Jan 21, 2023

cjgillot Jan 21, 2023

cjgillot Jan 21, 2023

estebank Feb 1, 2023

TaKO8Ki commented Feb 14, 2023

estebank commented Feb 14, 2023

TaKO8Ki commented Feb 16, 2023

bors commented Feb 16, 2023

More accurate spans for arg removal suggestion #106347

More accurate spans for arg removal suggestion #106347

Conversation

estebank commented Jan 1, 2023

rustbot commented Jan 1, 2023

This comment was marked as resolved.

petrochenkov commented Jan 2, 2023

estebank commented Jan 2, 2023

Ezrashaw commented Jan 6, 2023

estebank commented Jan 6, 2023

Ezrashaw commented Jan 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ezrashaw commented Jan 9, 2023 • edited Loading

estebank commented Jan 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TaKO8Ki commented Feb 14, 2023

estebank commented Feb 14, 2023

TaKO8Ki commented Feb 16, 2023

bors commented Feb 16, 2023

Ezrashaw commented Jan 9, 2023 •

edited

Loading