Always point at index span on index obligation failure #117856

estebank · 2023-11-13T00:52:50Z

Use more targetted span for index obligation failures by rewriting the obligation cause span.

CC #66023

Use more targetted span for index obligation failures by rewriting the obligation cause span. CC rust-lang#66023

rustbot · 2023-11-13T00:52:59Z

r? @wesleywiser

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

compiler-errors · 2023-11-13T01:10:17Z

compiler/rustc_hir_typeck/src/expr.rs

@@ -2877,7 +2877,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                    // two-phase not needed because index_ty is never mutable
                    self.demand_coerce(idx, idx_t, index_ty, None, AllowTwoPhase::No);
                    self.select_obligations_where_possible(|errors| {
-                        self.point_at_index_if_possible(errors, idx.span)
+                        self.point_at_index(errors, idx.span)


This makes me pretty uncomfortable. We may be selecting obligations that are only connected to the index expression by long chains of inference, i.e. that are only able to be selected due to coercion that happens above. This seems both non-local and order-dependent.

I would prefer if you just made the heuristic better in point_at_index_if_possible -- which is already a hack, but at least currently it's localized to SliceIndex trait predicates -- or find out where that Borrow obligation comes from below and give it a better ObligationCauseCode so that it may be noted more precisely.

One check I could make is to pass in the whole expression span in and only modify those that are matched. I initially did that, but then it didn't make any difference to any test in the suite, so I reduced to what there is in the pr.

compiler-errors · 2023-11-13T01:16:41Z

Why not just check that the root obligation (or at least when we peel the derives from the obligation cause) of the error comes from an Index trait?

estebank · 2023-11-13T01:53:24Z

Why not just check that the root obligation (or at least when we peel the derives from the obligation cause) of the error comes from an Index trait?

Because the root obligation is MiscObligation, and figuring out where the relevant one is created is a chore 😅

Every now and then I add a u16 sentinel to them so that I can actually track them down, but it didn't seem worth the effort here.

compiler-errors · 2023-11-13T01:58:30Z

Because the root obligation is MiscObligation, and figuring out where the relevant one is created is a chore 😅

What do you mean? I just meant checking the trait def id of the root obligation for each fulfillment error you're modifying in select_where_possible. I think that checking that trait obligation's def id is the Index lang item + checking that the error span contains the span of the index expression would be the best approach here. There shouldn't be any need to look at obligation cause with this approach.

. I initially did that, but then it didn't make any difference to any test in the suite, so I reduced to what there is in the pr.

Given that no other test in the suite was affected here, I think it's probably most likely that we lack good testing. That's one of the contributing factors for my uncomfort towards how large of a hammer this PR is. In that case, I think it's best to make this modification more defensively rather than have to deal with users complaining about poor spans later on.

compiler-errors · 2023-11-13T17:06:57Z

compiler/rustc_hir_typeck/src/expr.rs

+                {
+                    seen_preds.insert(error.obligation.predicate.kind().skip_binder());
+                }
+                (root, pred) if seen_preds.contains(&pred) | seen_preds.contains(&root) => {}


Is there a test exercising this branch?

Yes, without this the new test ends up with 2 E0277, one pointing at the index (with depth=0) and one to the whole expr (with depth=1). The later doesn't have an Index root_obligation, but the same trait predicate, which is why I did this (which felt less hacky than maybe relying on Spans or something like that).

compiler/rustc_hir_typeck/src/expr.rs

compiler-errors · 2023-11-13T19:03:27Z

r=me

estebank · 2023-11-13T20:34:38Z

@bors r=compiler-errors

bors · 2023-11-13T20:34:49Z

📌 Commit 5061c09 has been approved by compiler-errors

It is now in the queue for this repository.

…rrors Always point at index span on index obligation failure Use more targetted span for index obligation failures by rewriting the obligation cause span. CC rust-lang#66023

bors · 2023-11-14T06:20:29Z

⌛ Testing commit 5061c09 with merge 173b6e6...

bors · 2023-11-14T08:18:18Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 173b6e6 to master...

rust-timer · 2023-11-14T09:49:42Z

Finished benchmarking commit (173b6e6): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.6%	[2.1%, 3.0%]	2
Regressions ❌ (secondary)	3.1%	[3.1%, 3.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-5.2%, -2.4%]	2
All ❌✅ (primary)	2.6%	[2.1%, 3.0%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 674.024s -> 672.988s (-0.15%)
Artifact size: 311.11 MiB -> 311.12 MiB (0.00%)

Always point at index span on index obligation failure

69634f2

Use more targetted span for index obligation failures by rewriting the obligation cause span. CC rust-lang#66023

rustbot assigned wesleywiser Nov 13, 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 Nov 13, 2023

compiler-errors requested changes Nov 13, 2023

View reviewed changes

compiler-errors reviewed Nov 13, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

estebank force-pushed the issue-66023 branch from 08fb7b1 to e90fe80 Compare November 13, 2023 19:00

compiler-errors reviewed Nov 13, 2023

View reviewed changes

compiler/rustc_hir_typeck/src/expr.rs Outdated Show resolved Hide resolved

review comments: more targeted span setting approach

5061c09

estebank force-pushed the issue-66023 branch from e90fe80 to 5061c09 Compare November 13, 2023 19:57

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 Nov 13, 2023

matthiaskrgr mentioned this pull request Nov 14, 2023

Rollup of 2 pull requests #117901

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 14, 2023

bors merged commit 173b6e6 into rust-lang:master Nov 14, 2023

rustbot added this to the 1.76.0 milestone Nov 14, 2023

Always point at index span on index obligation failure #117856

Always point at index span on index obligation failure #117856

Uh oh!

Conversation

estebank commented Nov 13, 2023

Uh oh!

rustbot commented Nov 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compiler-errors commented Nov 13, 2023

Uh oh!

estebank commented Nov 13, 2023

Uh oh!

compiler-errors commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Uh oh!

compiler-errors commented Nov 13, 2023

Uh oh!

estebank commented Nov 13, 2023

Uh oh!

bors commented Nov 13, 2023

Uh oh!

bors commented Nov 14, 2023

Uh oh!

bors commented Nov 14, 2023

Uh oh!

rust-timer commented Nov 14, 2023

Overall result: no relevant changes - no action needed

Uh oh!

Uh oh!

compiler-errors commented Nov 13, 2023 •

edited

Loading