Arbitrary self types v2: simulated stabilization, do not merge #133570

adetaylor · 2024-11-28T12:46:47Z

This PR is essentially the same as #132961, but also flips the feature to being stabilized. We shouldn't merge this yet, but we want to do a crater run on this PR to check for unexpected surprises.

The difference relative to #132961 can be seen in this comparison.

Please do not review this - please add review comments on #132961, instead.

r? @wesleywiser

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

Part-implement new diagnostics specified in the RFC, and leave TODOs for the rest.

Do not merge any time soon - just to see what blows up in test suites when the old non-arbitrary-self-types paths are removed.

All the test changes necessary for stabilization here.

wesleywiser · 2024-12-03T00:02:13Z

@bors try

bors · 2024-12-03T00:03:25Z

⌛ Trying commit 9647820 with merge 2277c63...

…_for_real, r=<try> Arbitrary self types v2: simulated stabilization, do not merge This PR is essentially the same as rust-lang#132961, but also flips the feature to being stabilized. We shouldn't merge this yet, but we want to do a crater run on this PR to check for unexpected surprises. The difference relative to rust-lang#132961 can be seen in [this comparison](adetaylor/rust@arbitrary-self-types-the-big-bit...adetaylor:rust:receiver_trait_with_target_for_real). Please *do not* review this - please add review comments on rust-lang#132961, instead. r? `@wesleywiser`

bors · 2024-12-03T01:48:09Z

☀️ Try build successful - checks-actions
Build commit: 2277c63 (2277c63fa464d237b17b68d3b87f9fbb89e58a3e)

wesleywiser · 2024-12-03T02:38:42Z

@craterbot check

craterbot · 2024-12-03T02:38:54Z

👌 Experiment pr-133570 created and queued.
🤖 Automatically detected try build 2277c63
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

compiler-errors · 2024-12-03T06:31:23Z

@rust-timer build 2277c63

rust-timer · 2024-12-03T07:48:42Z

Finished benchmarking commit (2277c63): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +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.3%	[0.1%, 0.6%]	35
Regressions ❌ (secondary)	0.3%	[0.1%, 1.1%]	21
Improvements ✅ (primary)	-0.4%	[-0.6%, -0.2%]	22
Improvements ✅ (secondary)	-0.8%	[-1.5%, -0.1%]	12
All ❌✅ (primary)	0.1%	[-0.6%, 0.6%]	57

Max RSS (memory usage)

Results (primary -2.6%, secondary -2.5%)

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)	2.3%	[2.3%, 2.3%]	2
Improvements ✅ (primary)	-2.6%	[-3.2%, -1.4%]	3
Improvements ✅ (secondary)	-5.7%	[-7.1%, -3.8%]	3
All ❌✅ (primary)	-2.6%	[-3.2%, -1.4%]	3

Cycles

Results (primary 0.9%, secondary -0.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.9%	[0.8%, 1.1%]	2
Regressions ❌ (secondary)	2.2%	[2.1%, 2.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.8%	[-2.1%, -1.6%]	3
All ❌✅ (primary)	0.9%	[0.8%, 1.1%]	2

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 767.322s -> 767.19s (-0.02%)
Artifact size: 332.21 MiB -> 332.20 MiB (-0.00%)

craterbot · 2024-12-05T05:42:43Z

🚧 Experiment pr-133570 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-12-07T18:55:29Z

🎉 Experiment pr-133570 is completed!
📊 14 regressed and 2 fixed (548085 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

adetaylor · 2024-12-08T09:37:36Z

Crater results triaged here - #132961 (comment) - a clean run. Closing this PR.

compiler-errors · 2024-12-08T17:45:06Z

Since this is in no rush to be stabilized (namely, perf investigation still needs to happen and it naturally needs time to bake), we probably should run another stabilization crater run since the large number of "unknown" results is a bit concerning -- that's being investigated in https://rust-lang.zulipchat.com/#narrow/channel/242791-t-infra/topic/crater.20returning.20unknown (doesn't have to do w/ this PR)

compiler-errors · 2024-12-10T22:17:21Z

Let's rerun crater just for funsies.

@craterbot check

craterbot · 2024-12-10T22:17:33Z

👌 Experiment pr-133570-1 created and queued.
🤖 Automatically detected try build 2277c63
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-12-15T07:33:35Z

🚧 Experiment pr-133570-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-12-16T03:56:35Z

🎉 Experiment pr-133570-1 is completed!
📊 139 regressed and 48 fixed (552519 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

adetaylor · 2024-12-16T07:31:45Z

Triaging the new crater run from here, and relative to the prior crater run here:

We no longer have 179819 crates in "unknown" (thanks to this fix?)
The number of crates in "error" state has risen from 5635 to 149100. These were all in "error" in both the before and after commit so it's unlikely to be due to this PR. Looking at a few of those we have a variety of errors - failed to parse the version requirement 0.11 for dependencyparking_lot`` is quite common but there are others too.
Looking at those which have actually regressed in this PR there are 139:
- They're almost all no space left on device, and a few bus errors from linking with ld which may well be due to the same problem
- opusic-c gives a ninja error which doesn't seem to be related
- We have one real problem: fmhall.chess-program gives an ICE with Encountered anon const with inference variable args but no error reported - this looks to be from relevant code so is -presumed to be a real problem with arbitrary self types v2-, I'll look later. Update: it wasn't apparently related, see below

adetaylor · 2024-12-16T12:39:09Z

Looking into the above "real problem", it in fact reproduces with which the compiler version (c0e0d8f) immediately before the main arbitrary self types v2 PR (#132961) so I'm confident it's not related. I think the reason crater reported it as a new regression is that in prior runs, this test had been OOMing.

rustbot assigned wesleywiser Nov 28, 2024

adetaylor force-pushed the receiver_trait_with_target_for_real branch from 0325163 to b9ca9aa Compare November 28, 2024 13:05

adetaylor mentioned this pull request Nov 28, 2024

Arbitrary self types v2: main compiler changes #132961

Merged