Report generic mismatches when calling bodyless trait functions #136497

Jarcho · 2025-02-03T17:01:17Z

Don't know if there's an open issue for this. Just happened to notice this when working in that area.

The awkward extra spans added to the diagnostics of some tests (e.g. trait-with-missing-associated-type-restriction) is consistent with what happens for normal functions. Should probably be removed since that span doesn't seem to note anything useful.

First and third commit are both cleanups removing some unnecessary work. Second commit has the actual fix.

fixes #135124

rustbot · 2025-02-03T17:01:25Z

r? @petrochenkov

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

compiler-errors · 2025-02-03T17:20:33Z

LGTM. I'll approve when it's green.

lqd · 2025-02-03T17:26:02Z

tests/ui/fn/param-mismatch-trait-fn.stderr

+   |        ^^^^^^^^^ -  -     - this parameter needs to match the `[X; 1]` type of ``
+   |                  |  |
+   |                  |  `` needs to match the `[X; 1]` type of this parameter
+   |                  `` and `` both reference this parameter `T`


btw, are all these empty `` names expected?

Whoops, yeah we should definitely be returning None if the param names are empty.

Jarcho · 2025-02-03T19:32:48Z

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

@@ -2484,7 +2485,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                        }
                        generics_with_unmatched_params.push(generic_param);
                    } else {
-                        spans.push_span_label(param.span, "");
+                        spans.push_span_label(param.span(), "");


This is the line adding all the extra spans to the diagnostic. It looks like it's just a catch-all for parameters that aren't otherwise matched.

Given that the final commit already affects a pile of tests should I just go ahead and remove it?

compiler-errors · 2025-02-04T00:20:26Z

tests/ui/argument-suggestions/extra_arguments.stderr

@@ -97,7 +97,7 @@ note: function defined here
  --> $DIR/extra_arguments.rs:3:4
   |
 LL | fn two_arg_same(_a: i32, _b: i32) {}
-   |    ^^^^^^^^^^^^ -------  -------


Where did this span go? This seems wrong lol.

The code marking those spans just marks the span of all arguments that aren't generics used in multiple positions. It's also not applied to anything without a body (e.g. foreign functions).

The message as a whole is:

rust/tests/ui/argument-suggestions/extra_arguments.stderr

Lines 90 to 105 in f027438

error[E0061]: this function takes 2 arguments but 3 arguments were supplied

--> $DIR/extra_arguments.rs:26:3

|

LL | two_arg_same(1, 1, 1);

| ^^^^^^^^^^^^ - unexpected argument #3 of type `{integer}`

|

note: function defined here

--> $DIR/extra_arguments.rs:3:4

|

LL | fn two_arg_same(_a: i32, _b: i32) {}

| ^^^^^^^^^^^^ ------- -------

help: remove the extra argument

|

LL - two_arg_same(1, 1, 1);

LL + two_arg_same(1, 1);

|

Why are the arguments even being given spans here? The message is noting where the definition is and it's a pile of spans.

I can remove the third commit. It wasn't really the main point anyways and is the cause of the hundred files worth of changes.

Labeling parameters that correspond to mismatched arguments at a call site is something that has existed since #92364, way before #121595 refined this logic to have better error messages for generics, so these spans were never intended to only highlight generic args. I think these spans are particularly useful in general even if it can be excessive in some UI tests.

The choice of example (argument-suggestions/extra_arguments.rs) is kinda an outlier; the fact that "too many arguments" type errors underline all of the argument spans is possibly excessive, but this also regresses cases where highlighting the spans of the arguments is useful, like when arguments are permuted or there are missing arguments.

I think it's kinda unwarranted to be changing this behavior anyways with basically no explanation or justification, so yes, I'd probably prefer if we'd it.

Going back looks like df93364 actually broke the span marking here. It used to mark only relevant parameters before the generic arg matching code was added.

Given that fixing this is going to be a whole bunch of extra work I'll just revert the change for now.

If you want to author a fix, I'm happy to review that too in a separate PR. But I think removing all non-generic spans is kinda orthogonal and unnecessary here.

Reverted now.

compiler-errors · 2025-02-04T00:21:45Z

Should probably be removed since that span doesn't seem to note anything useful.

This PR seems to remove a lot more than just "useless spans", now 🤔 .

Since this PR has now ballooned to way more than what it was doing when I first approved it, I'm gonna pass it back to you to both refine the implementation and also explain what you're changing here in the PR description and why.

@rustbot author

compiler-errors · 2025-02-04T00:22:43Z

compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

-    /// unavailable (eg is an instrinsic).
+    /// type of that parameter.
+    ///
+    /// Returns `None` if the function has no parameters taking generic type, or the function is


This doesn't really make sense...? I think it's meaningful to return spans for functions if they don't have generics... like, people can still make argument mismatch errors if the function is not generic 🤔

This is specifically for, and only used for, generic mismatches. It doesn't really make sense to gather the generic parameters outside that context.

We still definitely want to label mismatched parameters even if those parameters are not associated with generic parameters, though.

Reverted as part of the other issue.

Jarcho · 2025-02-04T01:14:16Z

Why is tidy complaining about the test name? There are other tests with exactly the same naming scheme right next to it.

compiler-errors · 2025-02-04T01:15:50Z

Why is tidy complaining about the test name? There are other tests with exactly the same naming scheme right next to it.

They're grandfathered in, and we no longer permit adding new issue-### test files. Pls give it a more descriptive name.

Jarcho · 2025-02-04T01:18:34Z

Ah. The crashtest message should probably be changed then since it said just to move the file.

Jarcho · 2025-02-04T01:25:53Z

Do you want the ICE fix moved into it's own PR? It's basically a one line fix.

compiler-errors · 2025-02-04T01:27:31Z

The crashtest suite actually says to give it a meaningful name:

crashtest no longer crashes/triggers ICE, horray! Please give it a meaningful name, add a doc-comment to the start of the test explaining why it exists and move it to tests/ui or wherever you see fit. Adding 'Fixes #' to your PR description ensures that the corresponding ticket is auto-closed upon merge.

Jarcho · 2025-02-04T01:30:34Z

Apparently I just cant read then.

Should be ready now assuming CI works. @rustbot ready

compiler-errors · 2025-02-04T04:31:22Z

@bors r+ rollup

bors · 2025-02-04T04:31:25Z

📌 Commit 8b1c28f has been approved by compiler-errors

It is now in the queue for this repository.

Report generic mismatches when calling bodyless trait functions Don't know if there's an open issue for this. Just happened to notice this when working in that area. The awkward extra spans added to the diagnostics of some tests (e.g. `trait-with-missing-associated-type-restriction`) is consistent with what happens for normal functions. Should probably be removed since that span doesn't seem to note anything useful. First and third commit are both cleanups removing some unnecessary work. Second commit has the actual fix. fixes rust-lang#135124

Rollup of 9 pull requests Successful merges: - rust-lang#128045 (#[contracts::requires(...)] + #[contracts::ensures(...)]) - rust-lang#136263 (rustdoc: clean up a bunch of ts-expected-error declarations in main) - rust-lang#136375 (cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 1)) - rust-lang#136392 (bootstrap: add wrapper macros for `feature = "tracing"`-gated `tracing` macros) - rust-lang#136405 (rustdoc-book: Clean up section on `--output-format`) - rust-lang#136497 (Report generic mismatches when calling bodyless trait functions) - rust-lang#136502 (Mark `std::fmt::from_fn` as `#[must_use]`) - rust-lang#136509 (Add tests for nested macro_rules edition behavior) - rust-lang#136526 (mir_build: Rename `thir::cx::Cx` to `ThirBuildCx` and remove `UserAnnotatedTyHelpers`) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#136242 (Remove `LateContext::match_def_path()`) - rust-lang#136274 (Check Sizedness of return type in WF) - rust-lang#136284 (Allow using named consts in pattern types) - rust-lang#136477 (Fix a couple NLL TLS spans ) - rust-lang#136497 (Report generic mismatches when calling bodyless trait functions) - rust-lang#136520 (Remove unnecessary layout assertions for object-safe receivers) - rust-lang#136526 (mir_build: Rename `thir::cx::Cx` to `ThirBuildCx` and remove `UserAnnotatedTyHelpers`) Failed merges: - rust-lang#136304 (Reject negative literals for unsigned or char types in pattern ranges and literals) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#136497 - Jarcho:fn_ctxt, r=compiler-errors Report generic mismatches when calling bodyless trait functions Don't know if there's an open issue for this. Just happened to notice this when working in that area. The awkward extra spans added to the diagnostics of some tests (e.g. `trait-with-missing-associated-type-restriction`) is consistent with what happens for normal functions. Should probably be removed since that span doesn't seem to note anything useful. First and third commit are both cleanups removing some unnecessary work. Second commit has the actual fix. fixes rust-lang#135124

Only highlight unmatchable parameters at the definition site Followup to rust-lang#136497 This generally results more focused messages in the same vein as rust-lang#99635 (see `test/ui/argument-suggestions/complex.rs`). There are still some cases (e.g. `test/ui/argument-suggestions/permuted_arguments.rs`) where it might be worth highlighting the arguments. This is mitigated by the fact that a suggestion with a suggested rearrangement is given. r? `@compiler-errors`

Rollup merge of rust-lang#136583 - Jarcho:fn_ctxt2, r=compiler-errors Only highlight unmatchable parameters at the definition site Followup to rust-lang#136497 This generally results more focused messages in the same vein as rust-lang#99635 (see `test/ui/argument-suggestions/complex.rs`). There are still some cases (e.g. `test/ui/argument-suggestions/permuted_arguments.rs`) where it might be worth highlighting the arguments. This is mitigated by the fact that a suggestion with a suggested rearrangement is given. r? `@compiler-errors`

Remove allocations in FnCtxt checks.

b909c36

rustbot assigned petrochenkov Feb 3, 2025

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 Feb 3, 2025

This comment has been minimized.

Sign in to view

Jarcho force-pushed the fn_ctxt branch from a384184 to 8cb7482 Compare February 3, 2025 17:14

compiler-errors approved these changes Feb 3, 2025

View reviewed changes

compiler-errors assigned compiler-errors and unassigned petrochenkov Feb 3, 2025

lqd reviewed Feb 3, 2025

View reviewed changes

Check for generic parameter mismatches on trait functions.

6378fbc

Jarcho force-pushed the fn_ctxt branch 2 times, most recently from 13ee415 to 1a886cf Compare February 3, 2025 19:27

rustbot added the WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) label Feb 3, 2025

Jarcho commented Feb 3, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

Jarcho force-pushed the fn_ctxt branch from 1a886cf to 4ed6585 Compare February 3, 2025 20:14

This comment has been minimized.

Sign in to view

Jarcho force-pushed the fn_ctxt branch from 4ed6585 to 776ad90 Compare February 3, 2025 22:58

This comment has been minimized.

Sign in to view

Jarcho force-pushed the fn_ctxt branch from 776ad90 to 51ebe10 Compare February 3, 2025 23:37

compiler-errors reviewed Feb 4, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 4, 2025

compiler-errors reviewed Feb 4, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

Jarcho force-pushed the fn_ctxt branch from 3f4af33 to 7551b1d Compare February 4, 2025 00:30

This comment has been minimized.

Sign in to view

Jarcho force-pushed the fn_ctxt branch from 7551b1d to 354f76c Compare February 4, 2025 01:27

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 4, 2025

This comment has been minimized.

Sign in to view

Fix ICE when function argument mismatches.

8b1c28f

Jarcho force-pushed the fn_ctxt branch from 354f76c to 8b1c28f Compare February 4, 2025 01:55

compiler-errors approved these changes Feb 4, 2025

View reviewed changes

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 4, 2025

jhpratt mentioned this pull request Feb 4, 2025

Rollup of 9 pull requests #136531

Closed

matthiaskrgr mentioned this pull request Feb 4, 2025

Rollup of 7 pull requests #136549

Merged

bors merged commit 29e1ddd into rust-lang:master Feb 4, 2025
6 checks passed

rustbot added this to the 1.86.0 milestone Feb 4, 2025

Jarcho mentioned this pull request Feb 5, 2025

Only highlight unmatchable parameters at the definition site #136583

Merged

matthiaskrgr mentioned this pull request Feb 9, 2025

ICE: demand: index out of bounds #135474

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report generic mismatches when calling bodyless trait functions #136497

Report generic mismatches when calling bodyless trait functions #136497

Jarcho commented Feb 3, 2025 •

edited

Loading

rustbot commented Feb 3, 2025

This comment has been minimized.

compiler-errors commented Feb 3, 2025

lqd Feb 3, 2025

compiler-errors Feb 3, 2025

Jarcho Feb 3, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

compiler-errors Feb 4, 2025

Jarcho Feb 4, 2025 •

edited

Loading

compiler-errors Feb 4, 2025

Jarcho Feb 4, 2025 •

edited

Loading

compiler-errors Feb 4, 2025

Jarcho Feb 4, 2025

compiler-errors commented Feb 4, 2025

compiler-errors Feb 4, 2025

Jarcho Feb 4, 2025

compiler-errors Feb 4, 2025

Jarcho Feb 4, 2025

This comment has been minimized.

This comment has been minimized.

Jarcho commented Feb 4, 2025

compiler-errors commented Feb 4, 2025

Jarcho commented Feb 4, 2025

Jarcho commented Feb 4, 2025

compiler-errors commented Feb 4, 2025 •

edited

Loading

Jarcho commented Feb 4, 2025

This comment has been minimized.

compiler-errors commented Feb 4, 2025

bors commented Feb 4, 2025

	error[E0061]: this function takes 2 arguments but 3 arguments were supplied
	--> $DIR/extra_arguments.rs:26:3
	\|
	LL \| two_arg_same(1, 1, 1);
	\| ^^^^^^^^^^^^ - unexpected argument #3 of type `{integer}`
	\|
	note: function defined here
	--> $DIR/extra_arguments.rs:3:4
	\|
	LL \| fn two_arg_same(_a: i32, _b: i32) {}
	\| ^^^^^^^^^^^^ ------- -------
	help: remove the extra argument
	\|
	LL - two_arg_same(1, 1, 1);
	LL + two_arg_same(1, 1);
	\|

Report generic mismatches when calling bodyless trait functions #136497

Report generic mismatches when calling bodyless trait functions #136497

Conversation

Jarcho commented Feb 3, 2025 • edited Loading

rustbot commented Feb 3, 2025

This comment has been minimized.

compiler-errors commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Jarcho Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jarcho Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Feb 4, 2025

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

This comment has been minimized.

This comment has been minimized.

Jarcho commented Feb 4, 2025

compiler-errors commented Feb 4, 2025

Jarcho commented Feb 4, 2025

Jarcho commented Feb 4, 2025

compiler-errors commented Feb 4, 2025 • edited Loading

Jarcho commented Feb 4, 2025

This comment has been minimized.

compiler-errors commented Feb 4, 2025

bors commented Feb 4, 2025

Jarcho commented Feb 3, 2025 •

edited

Loading

Jarcho Feb 4, 2025 •

edited

Loading

Jarcho Feb 4, 2025 •

edited

Loading

compiler-errors commented Feb 4, 2025 •

edited

Loading