Rework "point at arg" suggestions to be more accurate #100654

compiler-errors · 2022-08-16T23:55:31Z

Introduce a new set of ObligationCauseCodes which have additional bookeeping for what expression caused the obligation, and which predicate caused the obligation. This allows us to look at the unsubstituted signature to find out which parameter or generic type argument caused an obligaton to fail.

This means that (in most cases) we significantly improve the likelihood of pointing out the right argument that causes a fulfillment error. Also, since this logic isn't happening in just the select_where_possible_and_mutate_fulfillment() calls in the argument checking code, but instead during all trait selection in FnCtxt, we are also able to point out the correct argument even if inference means that we don't know whether an obligation has failed until well after a call expression has been checked.

r? @ghost

src/test/ui/binop/issue-77910-1.stderr

src/test/ui/closures/closure-move-sync.stderr

src/test/ui/issues/issue-60218.stderr

src/test/ui/associated-types/associated-types-eq-3.stderr

compiler-errors · 2022-08-17T16:21:59Z

I think this is ready for review. I am happy to talk over zulip or meet with anyone to discuss the main changes in this PR if interested.

I am also happy to explain any cases where diagnostics have changed, to help weigh the benefits over the shortcomings over this PR. In general, though, I think this is a major improvement over the old logic.

r? diagnostics

estebank

I need to finish looking at checks.rs, but I'm comfortable landing as is so far.

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/mismatched_static_lifetime.rs

compiler/rustc_middle/src/ty/generics.rs

estebank · 2022-08-18T09:04:49Z

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

-                ) =
-                    (self.tcx.sess.source_map().span_to_snippet(span), obligation.cause.code())
+                } else if let Ok(snippet) = &self.tcx.sess.source_map().span_to_snippet(span)
+                    && let ObligationCauseCode::BindingObligation(def_id, _) | ObligationCauseCode::ExprBindingObligation(def_id, ..)


Seeing the proliferation of these kind of checks, we might want to start adding methods to the ObligationCauseCode to deduplicate them.

estebank · 2022-08-18T09:10:48Z

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

+        let Some(arg_ty) = typeck_results.node_type_opt(*arg_hir_id)
+            else { return false };


IIRC there's a separate more appropriate method along the lines of resolved_node_type_opt. I believe this method gives you the type before inference. It's still useful, but not maximally so. (Unless I'm wrong and I'm misremembering things.)

do you mean expr_ty_adjusted?

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

estebank · 2022-08-18T09:48:22Z

src/test/ui/no-send-res-ports.stderr

+   |  _____|_____________within this `[closure@$DIR/no-send-res-ports.rs:25:19: 25:25]`
+   | |     |
+   | |     required by a bound introduced by this call
+LL | |
+LL | |         let y = x;
+LL | |         println!("{:?}", y);
+LL | |     });
+   | |_____^ `Rc<()>` cannot be sent between threads safely


Maybe we should special case closures like we do for the within this to customize the output here.

estebank · 2022-08-18T09:51:31Z

src/test/ui/suggestions/issue-84973-blacklist.stderr

   |
 LL |     f_unpin(static || { yield; });
-   |     ^^^^^^^ the trait `Unpin` is not implemented for `[static generator@$DIR/issue-84973-blacklist.rs:17:13: 17:22]`
+   |     ------- ^^^^^^^^^^^^^^^^^^^^ the trait `Unpin` is not implemented for `[static generator@$DIR/issue-84973-blacklist.rs:17:13: 17:22]`


I just realized that in cases where the closure would be the only label, and it's pointing at the closure itself, we can customize the output and refer to "this closure" instead.

This seems a bit difficult since I'm walking over the unsubstituted predicate here. I'm gonna leave that as a follow-up, since it's not technically a regression I think.

Yeah, yeah, this was more of a general observation, not part of the review.

estebank · 2022-08-18T09:52:24Z

src/test/ui/traits/bound/assoc-fn-bound-root-obligation.stderr

-   |       ------------ ^^^^^ expected an `FnMut<(char,)>` closure, found `u8`
-   |       |
-   |       required by a bound introduced by this call
+   |       ^^^^^^^^^^^^ expected an `FnMut<(char,)>` closure, found `u8`


Do you know why we might have this regression? (We should also modify the main message to talk about Pattern, like we did with the help.

Also not part of the review.

src/test/ui/traits/issue-71136.stderr

estebank · 2022-08-18T10:02:11Z

compiler/rustc_typeck/src/check/expr.rs

                        span,
-                        traits::SizedArgumentType(None),
+                        traits::SizedArgumentType(arg_span),


I think here is where we could keep around the info about the Sized argument requirement coming from a call.

estebank · 2022-08-18T10:04:44Z

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

+        match hir.get(hir_id) {
+            hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Path(qpath), hir_id, .. }) => {


Should we move all of this to its own method?

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

estebank · 2022-08-18T13:54:07Z

r=me

compiler-errors · 2022-08-18T14:23:23Z

I'm gonna fix CI, then land this and work on a couple of easy follow-ups like those weird closure overlapping spans.

compiler-errors · 2022-08-18T19:42:11Z

Rebased, will wait until CI is green.

compiler/rustc_typeck/src/check/method/mod.rs

compiler-errors · 2022-08-18T20:37:24Z

@bors r=estebank rollup=never

(kinda bitrotty probably)

bors · 2022-08-18T20:37:26Z

📌 Commit cb7a2ec40879726ec47c777e128e0c0de5f4e6dd has been approved by estebank

It is now in the queue for this repository.

…macro notes

compiler-errors · 2022-08-21T03:52:59Z

Actually, I think I overestimated the difficulty of rebasing this. The fix was to just add an extra match for ExprItemObligation seen in the last commit. Sorry for the ping, I'm actually totally fine here.

@bors r=estebank

bors · 2022-08-21T03:53:01Z

📌 Commit 4045a268443bed3a3688340e8550d1c7b03cf78d has been approved by estebank

It is now in the queue for this repository.

compiler-errors · 2022-08-21T04:08:07Z

Tidy is the bane of my existence.

compiler-errors · 2022-08-21T04:53:31Z

@bors r=estebank

bors · 2022-08-21T04:53:33Z

📌 Commit d577eb0 has been approved by estebank

It is now in the queue for this repository.

bors · 2022-08-21T21:52:59Z

⌛ Testing commit d577eb0 with merge 0b71ffc...

bors · 2022-08-22T00:33:56Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 0b71ffc to master...

rust-timer · 2022-08-22T01:48:59Z

Finished benchmarking commit (0b71ffc): comparison url.

Instruction count

Primary benchmarks: ✅ relevant improvement found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.9%	-0.9%	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.9%	-0.9%	1

Max RSS (memory usage)

Results

Primary benchmarks: ✅ relevant improvement found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.8%	1.8%	2
Improvements ✅ (primary)	-2.3%	-2.3%	1
Improvements ✅ (secondary)	-3.8%	-3.8%	1
All ❌✅ (primary)	-2.3%	-2.3%	1

Cycles

Results

Primary benchmarks: ✅ relevant improvement found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-6.0%	-6.0%	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-6.0%	-6.0%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 16, 2022

compiler-errors commented Aug 16, 2022

View reviewed changes

src/test/ui/binop/issue-77910-1.stderr Outdated Show resolved Hide resolved

compiler-errors commented Aug 16, 2022

View reviewed changes

src/test/ui/closures/closure-move-sync.stderr Outdated Show resolved Hide resolved

compiler-errors commented Aug 17, 2022

View reviewed changes

src/test/ui/issues/issue-60218.stderr Show resolved Hide resolved

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the rework-point-at-arg branch from d573569 to c8d7081 Compare August 17, 2022 04:58

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the rework-point-at-arg branch from a94562b to ab844c5 Compare August 17, 2022 06:49

compiler-errors commented Aug 17, 2022

View reviewed changes

src/test/ui/associated-types/associated-types-eq-3.stderr Show resolved Hide resolved

rust-highfive assigned estebank Aug 17, 2022

compiler-errors marked this pull request as ready for review August 17, 2022 16:22

compiler-errors added the A-diagnostics Area: Messages for errors, warnings, and lints label Aug 17, 2022

estebank approved these changes Aug 18, 2022

View reviewed changes

estebank reviewed Aug 18, 2022

View reviewed changes

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs Outdated Show resolved Hide resolved

estebank reviewed Aug 18, 2022

View reviewed changes

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs Outdated Show resolved Hide resolved

estebank reviewed Aug 18, 2022

View reviewed changes

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs Outdated Show resolved Hide resolved

compiler-errors force-pushed the rework-point-at-arg branch from b25a1bc to f74e258 Compare August 18, 2022 13:38

estebank reviewed Aug 18, 2022

View reviewed changes

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the rework-point-at-arg branch from f74e258 to cb7a2ec Compare August 18, 2022 19:41

klensy reviewed Aug 18, 2022

View reviewed changes

compiler/rustc_typeck/src/check/method/mod.rs Show resolved Hide resolved

compiler-errors added 5 commits August 21, 2022 02:35

Comment a bit, use find_ancestor_in_same_ctxt to suppress some weird …

6848ba2

…macro notes

Targeted fixes addressing erroneous suggestions

8917894

More docs

2a16a12

Adjust messages, address some nits

d2f54b1

Make check for overlapping closure span more accurate

5212ac9

compiler-errors force-pushed the rework-point-at-arg branch from 6d6e9f5 to 4045a26 Compare August 21, 2022 03:52

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 21, 2022

This comment has been minimized.

Sign in to view

Bless tests after rust-lang#100769

d577eb0

compiler-errors force-pushed the rework-point-at-arg branch from 4045a26 to d577eb0 Compare August 21, 2022 04:08

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 22, 2022

bors merged commit 0b71ffc into rust-lang:master Aug 22, 2022

rustbot added this to the 1.65.0 milestone Aug 22, 2022

This was referenced Aug 22, 2022

DO NOT MERGE (yet): Enable -Zdrop-tracking by default #97334

Closed

Fix two const_trait_impl issues #100336

Merged

safe transmute: use Assume struct to provide analysis options #100726

Merged

compiler-errors mentioned this pull request Aug 29, 2022

E0525 should point at where a closure trait obligation was introduced #101157

Closed

yshui mentioned this pull request Sep 9, 2022

ICE: no type for node #101623

Closed

compiler-errors deleted the rework-point-at-arg branch August 11, 2023 20:15

matthiaskrgr mentioned this pull request Jan 7, 2024

ICE: effects, gce: difference in caller_arg caller_abi layout #119701

Open

GrigorenkoPV mentioned this pull request Mar 25, 2024

Incorrect argument index highlighting an error for type mismatch on static trait function calls #122938

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework "point at arg" suggestions to be more accurate #100654

Rework "point at arg" suggestions to be more accurate #100654

compiler-errors commented Aug 16, 2022 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

compiler-errors commented Aug 17, 2022 •

edited

Loading

estebank left a comment

estebank Aug 18, 2022

estebank Aug 18, 2022

compiler-errors Aug 18, 2022 •

edited

Loading

estebank Aug 18, 2022

estebank Aug 18, 2022

estebank Aug 18, 2022

compiler-errors Aug 18, 2022

estebank Aug 18, 2022

estebank Aug 18, 2022

estebank Aug 18, 2022

estebank Aug 18, 2022

estebank Aug 18, 2022

estebank commented Aug 18, 2022

This comment has been minimized.

compiler-errors commented Aug 18, 2022

compiler-errors commented Aug 18, 2022

compiler-errors commented Aug 18, 2022

bors commented Aug 18, 2022

compiler-errors commented Aug 21, 2022

bors commented Aug 21, 2022

This comment has been minimized.

compiler-errors commented Aug 21, 2022

compiler-errors commented Aug 21, 2022

bors commented Aug 21, 2022

bors commented Aug 21, 2022

bors commented Aug 22, 2022

rust-timer commented Aug 22, 2022

		let Some(arg_ty) = typeck_results.node_type_opt(*arg_hir_id)
		else { return false };

		match hir.get(hir_id) {
		hir::Node::Expr(hir::Expr { kind: hir::ExprKind::Path(qpath), hir_id, .. }) => {

Rework "point at arg" suggestions to be more accurate #100654

Rework "point at arg" suggestions to be more accurate #100654

Conversation

compiler-errors commented Aug 16, 2022 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

compiler-errors commented Aug 17, 2022 • edited Loading

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Aug 18, 2022 • edited Loading

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

estebank commented Aug 18, 2022

This comment has been minimized.

compiler-errors commented Aug 18, 2022

compiler-errors commented Aug 18, 2022

compiler-errors commented Aug 18, 2022

bors commented Aug 18, 2022

compiler-errors commented Aug 21, 2022

bors commented Aug 21, 2022

This comment has been minimized.

compiler-errors commented Aug 21, 2022

compiler-errors commented Aug 21, 2022

bors commented Aug 21, 2022

bors commented Aug 21, 2022

bors commented Aug 22, 2022

rust-timer commented Aug 22, 2022

Footnotes

compiler-errors commented Aug 16, 2022 •

edited

Loading

compiler-errors commented Aug 17, 2022 •

edited

Loading

compiler-errors Aug 18, 2022 •

edited

Loading