Arbitrary self types v2: main compiler changes #132961
Conversation
rustbot
added
S-waiting-on-review
|
@rustbot label +S-waiting-on-author -S-waiting-on-review |
rustbot
added
S-waiting-on-author
| @@ -69,6 +73,14 @@ impl<'a, 'tcx> Iterator for Autoderef<'a, 'tcx> { | |||
| } | |||
|
|
|||
| // Otherwise, deref if type is derefable: | |||
| // NOTE: in the case of self.use_receiver_trait = true, you might think it would | |||
| // be better to skip this clause and use the Overloaded case only, since &T | |||
| // and &mut T implement Receiver. (They don't implement Deref). We can't do that | |||
There was a problem hiding this comment.
&T does implement Deref. Am I misunderstanding something here?
There was a problem hiding this comment.
Good spot, this comment is outdated, I'll tweak.
There was a problem hiding this comment.
I think we should probably just mention that this is an optimization that applies equally to Receiver or Deref, and the fact that we record those as builtin derefs has consequences on const and the emitted MIR.
This comment has been minimized.
This comment has been minimized.
|
The failure here is interesting - it seems that our deshadowing algorithm does impact some existing code. |
adetaylor
force-pushed
the
arbitrary-self-types-the-big-bit
branch
from
5478b3d
to
0e03674
Compare
This comment has been minimized.
This comment has been minimized.
adetaylor
force-pushed
the
arbitrary-self-types-the-big-bit
branch
from
0e03674
to
adea5bc
Compare
| // ``` | ||
| // 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 |
There was a problem hiding this comment.
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.
|
Thanks for the early review @wesleywiser - I have a question for you as well - obviously, for now, the new functionality remains behind the |
adetaylor
force-pushed
the
arbitrary-self-types-the-big-bit
branch
2 times, most recently
from
c4bf39f
to
c0e8581
Compare
|
(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) |
adetaylor
force-pushed
the
arbitrary-self-types-the-big-bit
branch
from
c0e8581
to
520f90b
Compare
|
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. |
adetaylor
force-pushed
the
arbitrary-self-types-the-big-bit
branch
from
520f90b
to
b7da5e1
Compare
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.
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: 1. By value; 2. By reference; 3. By mutable reference; 4. By const ptr. 5. 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.
The deshadowing algorithm in this PR is at risk of triggering in undesirable circumstances, where extension traits have made methods available. Add a couple of tests corresponding to cases where this has been encountered.
adetaylor
force-pushed
the
arbitrary-self-types-the-big-bit
branch
from
b7da5e1
to
6d61ab8
Compare
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:
Dereftrait to the newReceivertrait when the unstablearbitrary_self_typesfeature is enabled (the simple bit)This should not break compatibility for:
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 allowSubsequent PRs will add better diagnostics.
It's probably easiest to review this commit-by-commit.
r? @wesleywiser