Arbitrary self types v2: main compiler changes by adetaylor · Pull Request #132961 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation56 Commits6 Checks6 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This is the main PR in a series of PRs related to Arbitrary Self Types v2, tracked in #44874. Specifically this is step 7 of the plan described here, for RFC 3519.
Overall this PR:
- Switches from the
Dereftrait to the newReceivertrait when the unstablearbitrary_self_typesfeature is enabled (the simple bit) - Introduces new algorithms to spot "shadowing"; that is, the case where a newly-added method in an outer smart pointer might end up overriding a pre-existing method in the pointee (the complex bit). Most of this bit was explored in this earlier perf-testing PR.
- Lots of tests
This should not break compatibility for:
- Stable users, where it should have no effect
- Users of the existing
arbitrary_self_typesfeature (because we implementReceiverforT: Deref) unless those folks have added methods which may shadow methods in inner types, which we no longer want to allow
Subsequent PRs will add better diagnostics.
It's probably easiest to review this commit-by-commit.
r? @wesleywiser
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
@rustbot label +S-waiting-on-author -S-waiting-on-review
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
This comment has been minimized.
The failure here is interesting - it seems that our deshadowing algorithm does impact some existing code. IndexVec has its own into_iter() and so might its contents have (it implements Deref). It was not intentional that this deshadowing impacts existing code but I think I can see why the pattern here wasn't what I was expecting - I'll need to do a bit of thinking about how to avoid this case.
This comment has been minimized.
| // ``` |
|---|
| // In these circumstances, the logic is wrong, and we wouldn't spot |
| // the shadowing, because the autoderef-based maths wouldn't line up. |
| // This is a niche case and we can live without generating an error |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe this should be an unresolved question on the tracking issue. T-lang can decide if they're fine with this niche case before stabilization.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll wait until we're largely through the review process here, and after we've done a crater run, then accumulate a list of questions which have come up, including this one.
Other assumptions I've made which might merit wider discussion:
- We only care to apply the shadowing algorithm for inherent methods, not trait methods. (This applies both for the shadower and the shadowed method). This is the primary case that we cared about in RFC discussions, so I've gone with that for simplicity, but there are also arguments that we might want to error if (for example) trait methods are shadowed by a new inherent method, or even vice-versa.
- We don't want to try to spot shadowing problems involving raw pointers or the newfangled "reborrowed pin" (which didn't exist when we wrote the RFC)
Thanks for the early review @wesleywiser - I have a question for you as well - obviously, for now, the new functionality remains behind the arbitrary_self_types feature flag. Eventually we want to stabilize it, and it would be good to know now about any unforeseen problems there, before this PR is finalized and merged. Is there best practice for raising a PR and/or a crater run to test the world as if the feature were stabilized?
(NB this is still in draft - no need to review further till I put it up for proper review - there's still work to do)
If you would like the crater run done, what I would suggest is opening a second PR that has the changes you want to test un-feature-gated (ie, as if they had been stabilized) and then we can do a crater run on that PR and close it once the results come back.
If you would like the crater run done, what I would suggest is opening a second PR that has the changes you want to test un-feature-gated (ie, as if they had been stabilized) and then we can do a crater run on that PR and close it once the results come back.
Great - I'll do that. (Update: done in #133570)
@rustbot label -S-waiting-on-author +S-waiting-on-review
Status: This is awaiting some action (such as code changes or more information) from the author.
label
📌 Commit 718b4ca has been approved by compiler-errors,wesleywiser
It is now in the queue for this repository.
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
🔒 Merge conflict
This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.
How do I rebase?
Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:
git checkout arbitrary-self-types-the-big-bit(switch to your branch)git fetch upstream master(retrieve the latest master)git rebase upstream/master -p(rebase on top of it)- Follow the on-screen instruction to resolve conflicts (check
git statusif you got lost). git push self arbitrary-self-types-the-big-bit --force-with-lease(update this PR)
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.
Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.
Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.
Error message
warning: Cannot merge binary files: tests/ui/self/arbitrary-self-opaque.stderr (HEAD vs. heads/homu-tmp)
Auto-merging tests/ui/self/arbitrary-self-opaque.stderr
CONFLICT (content): Merge conflict in tests/ui/self/arbitrary-self-opaque.stderr
Auto-merging compiler/rustc_hir_analysis/src/check/wfcheck.rs
Auto-merging compiler/rustc_hir_analysis/messages.ftl
Automatic merge failed; fix conflicts and then commit the result.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
In this new version of Arbitrary Self Types, we no longer use the Deref trait exclusively when working out which self types are valid. Instead, we follow a chain of Receiver traits. This enables methods to be called on smart pointer types which fundamentally cannot support Deref (for instance because they are wrappers for pointers that don't follow Rust's aliasing rules).
This includes:
- Changes to tests appropriately
- New tests for:
- The basics of the feature
- Ensuring lifetime elision works properly
- Generic Receivers
- A copy of the method subst test enhanced with Receiver
This is really the heart of the 'arbitrary self types v2' feature, and is the most critical commit in the current PR.
Subsequent commits are focused on:
- Detecting "shadowing" problems, where a smart pointer type can hide methods in the pointee.
- Diagnostics and cleanup.
Naming: in this commit, the "Autoderef" type is modified so that it no longer solely focuses on the "Deref" trait, but can now consider the "Receiver" trait instead. Should it be renamed, to something like "TraitFollower"? This was considered, but rejected, because
- even in the Receiver case, it still considers built-in derefs
- the name Autoderef is short and snappy.
This commit makes no (intentional) functional change.
Previously, the picking process maintained two lists of extra information useful for diagnostics:
- any unstable candidates which might have been picked
- any unsatisfied predicates
Previously, these were dealt with quite differently - the former list was passed around as a function parameter; the latter lived in a RefCell in the ProbeCtxt.
With this change we increase consistency by keeping them together in a new PickDiagHints structure, passed as a parameter, with no need for interior mutability.
The lifecycle of each of these lists remains fairly complex, so it's explained with new comments in pick_core.
A further cleanup here would be to package the widely-used tuple (ty::Predicate<'tcx>, Option<ty::Predicate<'tcx>>, Option<ObligationCause<'tcx>>) into a named struct for UnsatisfiedPredicate. This seems worth doing but it turns out that this tuple is used in dozens of places, so if we're going to do this we should do it as a separate PR to avoid constant rebase trouble.
This is the first part of a series of commits which impact the "deshadowing detection" in the arbitrary self types v2 RFC.
This commit should not have any functional changes, but may impact performance. Subsequent commits add back the performance, and add error checking to this new code such that it has a functional effect.
Rust prioritizes method candidates in this order:
- By value;
- By reference;
- By mutable reference;
- By const ptr.
- By reborrowed pin.
Previously, if a suitable candidate was found in one of these earlier categories, Rust wouldn't even move onto probing the other categories.
As part of the arbitrary self types work, we're going to need to change that - even if we choose a method from one of the earlier categories, we will sometimes need to probe later categories to search for methods that we may be shadowing.
This commit adds those extra searches for shadowing, but it does not yet produce an error when such shadowing problems are found. That will come in a subsequent commit, by filling out the 'check_for_shadowing' method.
This commit contains a naive approach to detecting these shadowing problems, which shows what we've functionally looking to do. However, it's too slow. The performance of this approach was explored in this PR: rust-lang#127812 (comment) Subsequent commits will improve the speed of the search.
A previous commit added a search for certain types of "shadowing" situation where one method (in an outer smart pointer type, typically) might hide or shadow the method in the pointee.
Performance investigation showed that the naïve approach is too slow - this commit speeds it up, while being functionally the same.
This still does not actually cause the deshadowing check to emit any errors; that comes in a subsequent commit which is where all the tests live.
This builds on the previous commits by actually adding checks for cases where a new method shadows an older method.
There's some discussion on the RFC about whether generic receivers should be allowed, but in the end the conclusion was that they should be blocked (at least for some definition of 'generic'). This blocking landed in an earlier PR; this commit adds additional tests to ensure the interaction with the rest of the Arbitrary Self Types v2 feature is as expected. This test may be a little duplicative but it seems better to land it than not.
@bors r=compiler-errors,wesleywiser
📌 Commit 337af8a has been approved by compiler-errors,wesleywiser
It is now in the queue for this repository.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-author
Status: This is awaiting some action (such as code changes or more information) from the author.
labels
☀️ Test successful - checks-actions
Approved by: compiler-errors,wesleywiser
Pushing 915e7eb to master...
Finished benchmarking commit (915e7eb): comparison URL.
Overall result: ❌ regressions - no action needed
@rustbot label: -perf-regression
Instruction count
This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
| mean | range | count | |
|---|---|---|---|
| Regressions ❌ (primary) | - | - | 0 |
| Regressions ❌ (secondary) | 0.3% | [0.3%, 0.4%] | 7 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | - | - | 0 |
Max RSS (memory usage)
Results (secondary 4.6%)
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) | 4.6% | [4.6%, 4.6%] | 1 |
| Improvements ✅ (primary) | - | - | 0 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | - | - | 0 |
Cycles
Results (primary -3.2%)
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) | -3.2% | [-3.2%, -3.2%] | 1 |
| Improvements ✅ (secondary) | - | - | 0 |
| All ❌✅ (primary) | -3.2% | [-3.2%, -3.2%] | 1 |
Binary size
This benchmark run did not return any relevant results for this metric.
Bootstrap: 770.02s -> 769.736s (-0.04%)
Artifact size: 331.01 MiB -> 331.09 MiB (0.02%)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…mpiler-errors
Arbitrary self types v2: adjust diagnostic.
The recently landed PR rust-lang#132961 to adjust arbitrary self types was a bit overenthusiastic, advising folks to use the new Receiver trait even before it's been stabilized. Revert to the older wording of the lint in such cases.
Tracking issue rust-lang#44874
r? @wesleywiser
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request
…mpiler-errors
Arbitrary self types v2: adjust diagnostic.
The recently landed PR rust-lang#132961 to adjust arbitrary self types was a bit overenthusiastic, advising folks to use the new Receiver trait even before it's been stabilized. Revert to the older wording of the lint in such cases.
Tracking issue rust-lang#44874
r? @wesleywiser
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#134262 - adetaylor:revert-diagnostics, r=compiler-errors
Arbitrary self types v2: adjust diagnostic.
The recently landed PR rust-lang#132961 to adjust arbitrary self types was a bit overenthusiastic, advising folks to use the new Receiver trait even before it's been stabilized. Revert to the older wording of the lint in such cases.
Tracking issue rust-lang#44874
r? @wesleywiser
95 tasks
Labels
This PR was explicitly merged by bors.
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.