Don't add redundant help for object safety violations #117200

rmehri01 · 2023-10-26T04:07:56Z

r? WaffleLapkin

rustbot · 2023-10-26T04:07:59Z

Could not assign reviewer from: WaffleLapkin.
User(s) WaffleLapkin are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

rustbot · 2023-10-26T04:08:04Z

r? @spastorino

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

WaffleLapkin · 2023-10-26T09:04:55Z

tests/ui/feature-gates/feature-gate-object_safe_for_dispatch.stderr

@@ -62,7 +62,6 @@ LL | trait NonObjectSafe4 {
   |       -------------- this trait cannot be made into an object...
 LL |     fn foo(&self, s: &Self);
   |                      ^^^^^ ...because method `foo` references the `Self` type in this parameter
-   = help: consider moving `foo` to another trait


I don't think this is a change we want. Note that this mentions a different foo than the previous messages.

Ah okay, sorry about that, I pushed a different solution that doesn't change the other tests

spastorino · 2023-10-27T13:31:36Z

@WaffleLapkin are you taking care of this review? can you assign it to yourselve?.
My only thought from skimming it quickly is ... why are we reporting twice in the first place?, do we have an idea why we insert 2 times in this list?.

WaffleLapkin · 2023-10-27T13:35:13Z

@spastorino I can take the review.

The cause is that there can be multiple problems with the same item, we want to highlight all of them, but the proposed solution is the same for all problems — move the item to another trait.

spastorino · 2023-10-27T14:27:55Z

The cause is that there can be multiple problems with the same item, we want to highlight all of them, but the proposed solution is the same for all problems — move the item to another trait.

Makes sense, I guess I'd hide from consumers the existence of that vector that tracks already suggested "solutions".

bors · 2023-10-30T21:43:32Z

☔ The latest upstream changes (presumably #117405) made this pull request unmergeable. Please resolve the merge conflicts.

WaffleLapkin

This looks fine.

Please add a comment, fix conflicts and use @rustbot review to ask me for review again =)

WaffleLapkin · 2023-11-26T16:08:49Z

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

@@ -103,9 +103,11 @@ pub fn report_object_safety_error<'tcx>(
    if trait_span.is_some() {
        let mut reported_violations: Vec<_> = reported_violations.into_iter().collect();
        reported_violations.sort();
+
+        let mut reported_names = vec![];


Could you add a little comment explaining why this is needed like "allows us to skip suggesting moving the same item to another trait multiple times" or something?

Done! Do you think there is a better way of doing this to hide the vector, maybe by taking in the list of violations up front for example or is it okay as is?

We could create an ObjectSafetyViolationSolution enum, which would be returned from solution, then deduplicated, then added to the error. Something like

let mut potential_solutions: Vec<_> = reported_violations.into_iter().map(...::solution).collect(); potential_solutions.sort(); potential_solutions.dedup(); potential_solutions.for_each(|solution| solution.add_to(&mut err));

Do you want to try doing this?

Ah I see, that sounds better, I'll give it a try!

rmehri01 · 2023-11-26T16:40:58Z

@rustbot review

WaffleLapkin · 2023-11-26T17:19:57Z

@rustbot author (#117200 (comment))

WaffleLapkin · 2023-11-26T18:03:40Z

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

-            // Only provide the help if its a local trait, otherwise it's not actionable.
-            violation.solution(&mut err);
+
+        // Only provide the help if its a local trait, otherwise it's not actionable.


Please move this to the if

rmehri01 · 2023-11-26T18:51:15Z

@rustbot review

WaffleLapkin

Thanks, I like this fix :)

WaffleLapkin · 2023-11-27T18:03:23Z

@bors r+

bors · 2023-11-27T18:03:26Z

📌 Commit ee96a7a has been approved by WaffleLapkin

It is now in the queue for this repository.

rmehri01 · 2023-11-27T18:04:13Z

Thanks for the help, I like this one much more too! 😄

bors · 2023-11-27T19:37:38Z

⌛ Testing commit ee96a7a with merge 6eb9524...

bors · 2023-11-27T21:51:43Z

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing 6eb9524 to master...

rust-timer · 2023-11-28T07:19:04Z

Finished benchmarking commit (6eb9524): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.1%	[-1.1%, -1.1%]	1
All ❌✅ (primary)	-	-	0

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)	-	-	0
Regressions ❌ (secondary)	3.1%	[2.1%, 4.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

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.379s -> 675.422s (0.15%)
Artifact size: 313.34 MiB -> 313.36 MiB (0.01%)

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

This comment has been minimized.

Sign in to view

rmehri01 force-pushed the repeated_help branch from e0e3503 to 76e0c85 Compare October 26, 2023 05:09

WaffleLapkin reviewed Oct 26, 2023

View reviewed changes

rmehri01 force-pushed the repeated_help branch from 76e0c85 to a29fb71 Compare October 26, 2023 15:21

WaffleLapkin assigned WaffleLapkin and unassigned spastorino Oct 27, 2023

WaffleLapkin requested changes Nov 26, 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 Nov 26, 2023

rmehri01 force-pushed the repeated_help branch from a29fb71 to e7b873b Compare November 26, 2023 16:37

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

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

rmehri01 force-pushed the repeated_help branch from e7b873b to e726058 Compare November 26, 2023 17:52

don't add redundant help for object safety violations

af6b84a

rmehri01 force-pushed the repeated_help branch from e726058 to af6b84a Compare November 26, 2023 17:54

WaffleLapkin reviewed Nov 26, 2023

View reviewed changes

move comment about local trait

ee96a7a

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

WaffleLapkin approved these changes Nov 27, 2023

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

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

bors merged commit 6eb9524 into rust-lang:master Nov 27, 2023

rustbot added this to the 1.76.0 milestone Nov 27, 2023

rmehri01 deleted the repeated_help branch November 27, 2023 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't add redundant help for object safety violations #117200

Don't add redundant help for object safety violations #117200

rmehri01 commented Oct 26, 2023

rustbot commented Oct 26, 2023

rustbot commented Oct 26, 2023

This comment has been minimized.

WaffleLapkin Oct 26, 2023

rmehri01 Oct 26, 2023

spastorino commented Oct 27, 2023

WaffleLapkin commented Oct 27, 2023

spastorino commented Oct 27, 2023 •

edited

Loading

bors commented Oct 30, 2023

WaffleLapkin left a comment •

edited

Loading

WaffleLapkin Nov 26, 2023

rmehri01 Nov 26, 2023

WaffleLapkin Nov 26, 2023

rmehri01 Nov 26, 2023

rmehri01 commented Nov 26, 2023

WaffleLapkin commented Nov 26, 2023

WaffleLapkin Nov 26, 2023

rmehri01 commented Nov 26, 2023

WaffleLapkin left a comment

WaffleLapkin commented Nov 27, 2023

bors commented Nov 27, 2023

rmehri01 commented Nov 27, 2023

bors commented Nov 27, 2023

bors commented Nov 27, 2023

rust-timer commented Nov 28, 2023

Don't add redundant help for object safety violations #117200

Don't add redundant help for object safety violations #117200

Conversation

rmehri01 commented Oct 26, 2023

rustbot commented Oct 26, 2023

rustbot commented Oct 26, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spastorino commented Oct 27, 2023

WaffleLapkin commented Oct 27, 2023

spastorino commented Oct 27, 2023 • edited Loading

bors commented Oct 30, 2023

WaffleLapkin left a comment • 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

rmehri01 commented Nov 26, 2023

WaffleLapkin commented Nov 26, 2023

Choose a reason for hiding this comment

rmehri01 commented Nov 26, 2023

WaffleLapkin left a comment

Choose a reason for hiding this comment

WaffleLapkin commented Nov 27, 2023

bors commented Nov 27, 2023

rmehri01 commented Nov 27, 2023

bors commented Nov 27, 2023

bors commented Nov 27, 2023

rust-timer commented Nov 28, 2023

Overall result: ✅ improvements - no action needed

spastorino commented Oct 27, 2023 •

edited

Loading

WaffleLapkin left a comment •

edited

Loading