Arbitrary self types v2: probe for more methods.

[adetaylor]

This PR is one of the first steps towards arbitrary self types v2, RFC 3519 (#44874). Specifically, it is step 2 of the plan outlined here. That says:

1. Search for potentially conflicting method candidates per the deshadowing part of the RFC. This is the riskiest part - per
   > the production of errors requires us to interrogate more candidates to look for potential conflicts, so this could have a compile-time performance penalty which we should measure
   let's attempt to land this first and see if performance issues show up. If so, it's back to the drawing board.

So - this commit isn't useful in itself but we need to determine its performance and any unexpected compatibility problems. That's what this PR is for. We shouldn't necessarily merge it.

Here's what this commit does.

Rust prioritizes method candidates in this order:

1. By value;
2. By reference;
3. By mutable reference;
4. By const ptr.

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.

We're adding the extra searches in a preliminary commit because they are the risky bit. We want to find any functional or performance problems from these extra searches before we proceed with the actual work on arbitrary self types and specifically on extra deshadowing errors.

[rustbot]

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[adetaylor]

@wesleywiser Hello, you've been assigned as reviewer!

However, rather than working towards merging this, I'd love advice about how to get Crater running over this, and how best we can assess performance implications. Any suggestions? Otherwise I'll pester some of the usual suspects who have previously been involved in arbitrary self types.

[lqd]

@bors try @rust-timer queue

[bors]

⌛ Testing commit edc2276 with merge f313d8b...

[adetaylor]

(a bit of iteration here because I can't get the whole rustc test suite to work locally due to something like llvm/llvm-project#92603)

[jackh726]

Is a crater run useful here yet? We're not emitting an extra errors, right?

For sure, perf run here indicates the "minimum extra work" in the happy path though.

[adetaylor]

Is a crater run useful here yet? We're not emitting an extra errors, right?

I've found that the extra calls to (for instance) pick_autorefd_method sometimes trigger other rustc bugs/overflows/extra error emission. In fact, an earlier version of this commit was causing some failures in rustc's own test suite due to the extra probing which was occurring. I've resolved those specific problems by doing the most minimal possible amount of probing, but it wouldn't surprise me to find that there are unforeseen issues in real crates.

[lqd]

@rust-timer build f313d8b

[rust-timer]

Finished benchmarking commit (f313d8b): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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 a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.2%, 0.9%]	34
Regressions ❌ (secondary)	5.8%	[0.4%, 10.1%]	11
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.4%	[-0.5%, 0.9%]	35

Max RSS (memory usage)

Results (primary 3.1%)

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)	3.1%	[2.2%, 4.0%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.1%	[2.2%, 4.0%]	2

Cycles

Results (secondary 8.7%)

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)	8.7%	[7.0%, 10.0%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 700.14s -> 701.137s (0.14%)
Artifact size: 328.63 MiB -> 328.45 MiB (-0.06%)

[adetaylor]

I'm glad we did a perf run :) It looks like it's significant.

So, the next steps here are:

1. Redo the PR such that it does less extra stuff, if possible. This may be possible at the cost of making the method probing more complex. Specifically, options include:

   • Reduce probing search space We only want to probe for possibly-shadowed methods meeting certain criteria. Right now we go ahead and call pick_method and then evaluate if any discovered methods match those criteria. We may be able to reduce the work done if we pass some of these criteria into pick_method, consider_candidates etc., reducing the work they have to do.
   • Cease to fully resolve candidates. We can take things further by ceasing to return fully-resolved candidates, but instead the minimum information required for pick_all_method to determine if there is shadowing. I don't know if there are possibilities here.

2. If spotting these shadowing picks remains expensive, then we'll have to revisit that section of the RFC to determine if we can live without it, or amend it. However I'm fairly optimistic we won't need to do that.

I'm unlikely to be able to work on this in the next 3 weeks or so, so I'm closing this PR for now. I may reopen it with an extra commit or two when I get back to this.