Only emit one error per unsized binding, instead of one per usage #113183

estebank · 2023-06-30T03:15:23Z

rustbot · 2023-06-30T03:15:28Z

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

compiler-errors

I think this is too fragile of a heuristic to be worth landing, and it introduces potential future headaches when queries interleave and for the parallel compiler. I'm not convinced this PR is worth landing 😓

clarifying edit: Specifically the dependency on self.tcx.sess.has_errors().is_some(), which is global state that I think we want to eventually remove, or at least track a lot more delicately.

compiler-errors · 2023-06-30T03:33:33Z

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

+                            && let Some(Node::Pat(pat)) = self.tcx.hir().find(*hir_id)
+                            && let Some(preds) = self.reported_trait_errors.borrow().get(&pat.span)
+                            && preds.contains(&obligation.predicate)
+                            && self.tcx.sess.has_errors().is_some()


I think we want to move forward with depending on less global state, rather than more? UI test instability due to global diagnostic state is one of the blockers for the parallel compiler, I think?

Note that a few lines above (689, hidden by the patch view) we already have a has_errors check. When we come up with an alternative for that one, this one can also go away. In this case this is to ensure that we don't unnecessarily silence obligations that happen to be in reported_trait_errors without actually having been emitted (I found a couple of examples in the test suite).

@compiler-errors let me know if my rationale above is sufficient to land the PR as is, otherwise we can close this PR.

Is tcx.sess.has_errors().is_some() required when we also check self.reported_trait_errors.borrow().get(&pat.span)?

The following tests ICE without that check

[ui] tests/ui/derives/derives-span-Clone-enum-struct-variant.rs [ui] tests/ui/derives/derives-span-Clone-enum.rs [ui] tests/ui/derives/derives-span-Hash-enum-struct-variant.rs [ui] tests/ui/derives/derives-span-PartialOrd-enum-struct-variant.rs [ui] tests/ui/derives/derives-span-PartialOrd-enum.rs [ui] tests/ui/derives/derives-span-Hash-enum.rs

I think because report_fulfillment_errors inserts all the error predicates found in the global reported_trait_errors, but it later has suppression logic that avoids emitting some diagnostics. I'm sure we could change that logic instead, but will make the PR larger.

Edit: actually the change ended up being much smaller (and the slight perf loss due to tiny amount of extra work only happens in the error path).

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

BoxyUwU · 2023-06-30T06:01:58Z

r? @compiler-errors

compiler-errors · 2023-07-10T16:42:07Z

I am still a bit skeptical whether this is worthwhile given the limitations (only fn calls) and introducing more dependency on global state, but I'd rather not unilaterally make that call.

r? diagnostics

estebank · 2023-07-18T14:04:11Z

given the limitations (only fn calls)

I focused on dealing with the reported case, but would hope to extend this to more ObligationCauseCode cases, within reason.

apiraino · 2023-08-24T09:49:18Z

r? diagnostics

davidtwco · 2023-08-24T10:37:31Z

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

+                            arg_hir_id,
+                            ..
+                        } = obligation.cause.code()
+                            && let Some(Node::Expr(arg)) = self.tcx.hir().find(*arg_hir_id)


I think I'd like to see the stuff inside the if let here put into a function, just to keep report_selection_error a bit simpler.

@estebank does this comment needs to be addressed?

Apart from this I think this PR can be r+'ed by @davidtwco (as per comment)?

This was already done

davidtwco

I think this is a useful improvement, particularly if extended to other reasonable obligation causes as @estebank has said is his intention.

I agree with the concerns about has_errors, but given that the function already uses has_errors only a few lines above the diff here, I don't think this addition makes removing has_errors or changing it materially more difficult - however the usage above is replaced, this usage can replaced the same way.

I've left a comment/reply or two, but I think I'm happy to r=me this after those are resolved.

apiraino · 2023-10-05T11:07:21Z

@estebank by reading the last comment, I think the review switch was left unchanged, when I think this is actually waiting on you.

@rustbot author

Fix rust-lang#56607.

estebank · 2023-10-10T18:14:16Z

I can confirm that handling other ObligationCauseCodes isn't necessary because Sized requirements on bindings is the only case where this particular issue crops up. I found a case where a type is defined as dyn Trait, which gets a predicate for dyn Trait + 'static in the definition, but on the use the predicate is for dyn Trait + '_, so the deduplication doesn't happen, but I feel like adding logic to check for it might be too convoluted (will try it out anyways).

estebank · 2023-10-26T21:02:45Z

@bors r=davidtwco

bors · 2023-10-26T21:02:48Z

📌 Commit 568b316 has been approved by davidtwco

It is now in the queue for this repository.

bors · 2023-10-27T00:06:16Z

⌛ Testing commit 568b316 with merge aa91057...

bors · 2023-10-27T02:01:31Z

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing aa91057 to master...

rust-timer · 2023-10-27T04:41:38Z

Finished benchmarking commit (aa91057): 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)

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

Cycles

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)	1.5%	[1.5%, 1.5%]	1
Regressions ❌ (secondary)	2.4%	[2.3%, 2.4%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.5%	[1.5%, 1.5%]	1

Binary size

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

Bootstrap: 635.024s -> 636.976s (0.31%)
Artifact size: 304.50 MiB -> 304.49 MiB (-0.00%)

rustbot assigned BoxyUwU Jun 30, 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 Jun 30, 2023

compiler-errors reviewed Jun 30, 2023

View reviewed changes

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

rustbot assigned compiler-errors and unassigned BoxyUwU Jun 30, 2023

rustbot assigned TaKO8Ki and unassigned compiler-errors Jul 10, 2023

rustbot assigned davidtwco and unassigned TaKO8Ki Aug 24, 2023

davidtwco reviewed Aug 24, 2023

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 Oct 5, 2023

This comment was marked as resolved.

Sign in to view

estebank added 4 commits October 9, 2023 22:51

Add test for rust-lang#56607

3d86b8a

Only emit one error per unsized binding, instead of one per usage

2f79681

Fix rust-lang#56607.

Remove need for has_errors() check

124d6d8

Move predicate error early check to its own method

568b316

estebank force-pushed the redundant-sized-errors branch from a6579e6 to 568b316 Compare October 9, 2023 23:18

estebank 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 Oct 10, 2023

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 Oct 26, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 27, 2023

bors merged commit aa91057 into rust-lang:master Oct 27, 2023
12 checks passed

rustbot added this to the 1.75.0 milestone Oct 27, 2023

bors mentioned this pull request Oct 27, 2023

Provide context when ? can't be called because of Result<_, E> #116496

Merged

github-actions bot mentioned this pull request Oct 31, 2023

Automated pull from upstream master ferrocene/ferrocene#74

Merged

estebank deleted the redundant-sized-errors branch November 9, 2023 05:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only emit one error per unsized binding, instead of one per usage #113183

Only emit one error per unsized binding, instead of one per usage #113183

estebank commented Jun 30, 2023

rustbot commented Jun 30, 2023

compiler-errors left a comment •

edited

Loading

compiler-errors Jun 30, 2023 •

edited

Loading

estebank Jun 30, 2023 •

edited

Loading

estebank Jul 6, 2023

davidtwco Aug 24, 2023

estebank Oct 9, 2023 •

edited

Loading

BoxyUwU commented Jun 30, 2023

compiler-errors commented Jul 10, 2023

estebank commented Jul 18, 2023

apiraino commented Aug 24, 2023

davidtwco Aug 24, 2023

apiraino Oct 26, 2023

estebank Oct 26, 2023

davidtwco left a comment

apiraino commented Oct 5, 2023

This comment was marked as resolved.

estebank commented Oct 10, 2023

estebank commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 27, 2023

bors commented Oct 27, 2023

rust-timer commented Oct 27, 2023

Only emit one error per unsized binding, instead of one per usage #113183

Only emit one error per unsized binding, instead of one per usage #113183

Conversation

estebank commented Jun 30, 2023

rustbot commented Jun 30, 2023

compiler-errors left a comment • edited Loading

Choose a reason for hiding this comment

compiler-errors Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

estebank Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estebank Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

BoxyUwU commented Jun 30, 2023

compiler-errors commented Jul 10, 2023

estebank commented Jul 18, 2023

apiraino commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidtwco left a comment

Choose a reason for hiding this comment

apiraino commented Oct 5, 2023

This comment was marked as resolved.

estebank commented Oct 10, 2023

estebank commented Oct 26, 2023

bors commented Oct 26, 2023

bors commented Oct 27, 2023

bors commented Oct 27, 2023

rust-timer commented Oct 27, 2023

Overall result: no relevant changes - no action needed

compiler-errors left a comment •

edited

Loading

compiler-errors Jun 30, 2023 •

edited

Loading

estebank Jun 30, 2023 •

edited

Loading

estebank Oct 9, 2023 •

edited

Loading