Don't add redundant help for object safety violations by rmehri01 · Pull Request #117200 · rust-lang/rust

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

   |       -------------- 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

        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

WaffleLapkin reviewed Oct 26, 2023

View reviewed changes

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

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

don't add redundant help for object safety violations

af6b84a

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

Uh oh!

Conversation

rmehri01 commented Oct 26, 2023

Uh oh!

rustbot commented Oct 26, 2023

Uh oh!

rustbot commented Oct 26, 2023

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spastorino commented Oct 27, 2023

Uh oh!

WaffleLapkin commented Oct 27, 2023

Uh oh!

spastorino commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Oct 30, 2023

Uh oh!

WaffleLapkin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmehri01 commented Nov 26, 2023

Uh oh!

WaffleLapkin commented Nov 26, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmehri01 commented Nov 26, 2023

Uh oh!

WaffleLapkin left a comment

Choose a reason for hiding this comment

Uh oh!

WaffleLapkin commented Nov 27, 2023

Uh oh!

bors commented Nov 27, 2023

Uh oh!

rmehri01 commented Nov 27, 2023

Uh oh!

bors commented Nov 27, 2023

Uh oh!

bors commented Nov 27, 2023

Uh oh!

rust-timer commented Nov 28, 2023

Overall result: ✅ improvements - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

spastorino commented Oct 27, 2023 •

edited

Loading

WaffleLapkin left a comment •

edited

Loading