Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added shadowed hint for overlapping associated types #117661

Merged

Conversation

TheLazyDutchman
Copy link
Contributor

Previously, when you tried to set an associated type that is shadowed by an associated type in a subtrait, like this:

trait A {
    type X;
}

trait B: A {
    type X; // note: this is legal
}

impl<Y> Clone for Box<dyn B<X=Y, X=Y>> {
    fn clone(&self) -> Self {
        todo!()
    }
}

you got a confusing error message, that says nothing about the shadowing:

error[E0719]: the value of the associated type `X` (from trait `B`) is already specified
 --> test.rs:9:34
  |
9 | impl<Y> Clone for Box<dyn B<X=Y, X=Y>> {
  |                             ---  ^^^ re-bound here
  |                             |
  |                             `X` bound here first

error[E0191]: the value of the associated type `X` (from trait `A`) must be specified
 --> test.rs:9:27
  |
2 |     type X;
  |     ------ `X` defined here
...
9 | impl<Y> Clone for Box<dyn B<X=Y, X=Y>> {
  |                           ^^^^^^^^^^^ help: specify the associated type: `B<X=Y, X=Y, X = Type>`

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0191, E0719.
For more information about an error, try `rustc --explain E0191`.

Now instead, the error shows that the associated type is shadowed, and suggests renaming as a potential fix.

error[E0719]: the value of the associated type `X` in trait `B` is already specified
 --> test.rs:9:34
  |
9 | impl<Y> Clone for Box<dyn B<X=Y, X=Y>> {
  |                             ---  ^^^ re-bound here
  |                             |
  |                             `X` bound here first

error[E0191]: the value of the associated type `X` in `A` must be specified
 --> test.rs:9:27
  |
2 |     type X;
  |     ------ `A::X` defined here
...
6 |     type X; // note: this is legal
  |     ------ `A::X` shadowed here
...
9 | impl<Y> Clone for Box<dyn B<X=Y, X=Y>> {
  |                           ^^^^^^^^^^^ associated type `X` must be specified
  |
help: consider renaming this associated type
 --> test.rs:2:5
  |
2 |     type X;
  |     ^^^^^^
help: consider renaming this associated type
 --> test.rs:6:5
  |
6 |     type X; // note: this is legal
  |     ^^^^^^

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0191, E0719.
For more information about an error, try rustc --explain E0191.

The rename help message is only emitted when the trait is local. This is true both for the supertrait as for the subtrait.

There might be cases where you can use the fully qualified path (for instance, in a where clause), but this PR currently does not deal with that.

fixes #100109

(continues from #117642, because I didn't know renaming the branch would close the PR)

@rustbot
Copy link
Collaborator

rustbot commented Nov 7, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot 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 7, 2023
@rust-log-analyzer

This comment has been minimized.

@TheLazyDutchman TheLazyDutchman force-pushed the point_out_shadowed_associated_types branch from 4a71828 to 45f2eac Compare November 7, 2023 11:00
@rust-log-analyzer

This comment has been minimized.

@TheLazyDutchman TheLazyDutchman force-pushed the point_out_shadowed_associated_types branch from 45f2eac to f106bfd Compare November 7, 2023 11:37
@rust-log-analyzer

This comment has been minimized.

@TheLazyDutchman TheLazyDutchman force-pushed the point_out_shadowed_associated_types branch from f106bfd to 6d8c642 Compare November 7, 2023 11:56
@petrochenkov petrochenkov 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 23, 2023
@TheLazyDutchman
Copy link
Contributor Author

After rebasing to try to resolve the conflict I get the following error when running x test tidy: tidy error: could not find exception package supports-hyperlinks`. I do not know why this happened, or how to fix this.

@petrochenkov
Copy link
Contributor

Could you push the rebased version to github?
If CI fails then it's an issue with the rebased PR, if not then it's some issue with the local setup and fresh rebuild may help.

@TheLazyDutchman
Copy link
Contributor Author

It gives the error too when it runs tidy when pushing, so it happened probably in the rebase

@petrochenkov
Copy link
Contributor

It gives the error too when it runs tidy when pushing

That's a git hook installed by x.py setup, try removing .git/hooks/pre-push in the rust repo to push without running x.py.
I suspect that you committed some submodule changes, and that now prevents x.py from setting that submodule to an up-to-date version.

@rustbot rustbot added A-meta Area: Issues about the rust-lang/rust repository. A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic O-ios Operating system: iOS O-linux Operating system: Linux O-macos Operating system: macOS O-solid Operating System: SOLID O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-watchos Operating System: watchOS labels Nov 25, 2023
@TheLazyDutchman TheLazyDutchman force-pushed the point_out_shadowed_associated_types branch from c347492 to 1404dbd Compare December 5, 2023 15:04
@TheLazyDutchman
Copy link
Contributor Author

@rustbot review

@petrochenkov petrochenkov 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 Dec 5, 2023
@TheLazyDutchman
Copy link
Contributor Author

@rustbot review

@rustbot 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 Dec 6, 2023
@petrochenkov
Copy link
Contributor

r=me after removing the second commit (but keeping #117661 (comment)).
@rustbot author

@rustbot 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 Dec 6, 2023
Shadowing the associated type of a supertrait is allowed.
This however makes it impossible to set the associated type
of the supertrait in a dyn object.

This PR makes the error message for that case clearer, like
adding a note that shadowing is happening, as well as suggesting
renaming of one of the associated types.

r=petrochenckov
@TheLazyDutchman TheLazyDutchman force-pushed the point_out_shadowed_associated_types branch from 1476d5c to 3234418 Compare December 6, 2023 10:06
@TheLazyDutchman
Copy link
Contributor Author

@rustbot review

@rustbot 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 Dec 6, 2023
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Dec 6, 2023

📌 Commit 3234418 has been approved by petrochenkov

It is now in the queue for this repository.

@bors 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 Dec 6, 2023
@bors
Copy link
Contributor

bors commented Dec 6, 2023

⌛ Testing commit 3234418 with merge dd6126e...

@bors
Copy link
Contributor

bors commented Dec 6, 2023

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing dd6126e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 6, 2023
@bors bors merged commit dd6126e into rust-lang:master Dec 6, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 6, 2023
@TheLazyDutchman TheLazyDutchman deleted the point_out_shadowed_associated_types branch December 6, 2023 14:17
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (dd6126e): 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)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.6% [-3.6%, -3.6%] 1
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: 675.083s -> 674.182s (-0.13%)
Artifact size: 314.18 MiB -> 314.16 MiB (-0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing suggestion when E0719 and E0191 occur at the same time.
8 participants