Shallow resolve ty and const vars to their root vars, attempt 2

[jdonszelmann]

View all comments

r? @lcnr
cc: @ShoyuVanilla

This is attempt 2. There is breakage, but how much? It may be worth it. @lcnr and I discovered today that this PR may have major perf impact on the new trait solver (in attempt 1 this wasn't tested in perf runs). In addition, fixing #158441 makes some tests 12x slower on the old solver, and this PR could mitigate that slowdown. The new solver already fixed this bug, hence we expect the new solver to become much faster with this PR.

Hence, we plan to do a crater and perf run to see just how bad it is.

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[jdonszelmann]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8ac0915 (8ac0915e346f9a2e49ca3951b642c55bd7d1eaab)
Base parent: 40557f6 (40557f6225e337d68c8d4f086557ce54135f5dd9)

[rust-timer]

Finished benchmarking commit (8ac0915): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-0.3%	[-0.5%, -0.1%]	19
Improvements ✅ (secondary)	-3.4%	[-7.5%, -0.2%]	38
All ❌✅ (primary)	-0.3%	[-0.5%, -0.1%]	19

Max RSS (memory usage)

Results (primary -0.8%, secondary 0.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.3%	[2.0%, 2.8%]	3
Improvements ✅ (primary)	-0.8%	[-0.9%, -0.8%]	2
Improvements ✅ (secondary)	-4.1%	[-4.1%, -4.1%]	1
All ❌✅ (primary)	-0.8%	[-0.9%, -0.8%]	2

Cycles

Results (primary -2.3%, secondary -2.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.0%	[1.5%, 10.5%]	3
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-3.8%	[-6.1%, -2.3%]	20
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 482.935s -> 488.862s (1.23%)
Artifact size: 392.98 MiB -> 395.55 MiB (0.65%)

[lcnr]

@craterbot check

[craterbot]

👌 Experiment pr-158447 created and queued.
🤖 Automatically detected try build 8ac0915
🔍 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]

🚧 Experiment pr-158447 is now running

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

[craterbot]

🎉 Experiment pr-158447 is completed!
📊 2 regressed and 1 fixed (997455 total)
📊 4979 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary 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

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-158447/retry-regressed-list.txt ↩

[theemathas]

The two crater breakages are identical to #153816 and #153849. See #153869.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[jdonszelmann]

@bors try @rust-timer queue

[jdonszelmann]

(note: with the added changes, both crates now compile :3)

[rust-bors]

☀️ Try build successful (CI)
Build commit: aab3d83 (aab3d8364c6796c1cac0962708a81c3cde81ac09)
Base parent: c290daa (c290daa1e5274a52e54a0f49e31bd70192abc606)

[rust-timer]

Finished benchmarking commit (aab3d83): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	2
Improvements ✅ (primary)	-0.3%	[-0.5%, -0.2%]	12
Improvements ✅ (secondary)	-3.2%	[-7.4%, -0.2%]	41
All ❌✅ (primary)	-0.3%	[-0.5%, -0.2%]	12

Max RSS (memory usage)

Results (primary 0.0%, secondary 6.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	6.4%	[6.4%, 6.4%]	1
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.7%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.8%, 3.2%]	5

Cycles

Results (secondary -3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.0%	[3.0%, 3.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.0%	[-6.5%, -1.5%]	18
All ❌✅ (primary)	-	-	0

Binary size

Results (primary 0.0%, secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	23
Regressions ❌ (secondary)	0.0%	[0.0%, 0.1%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	23

Bootstrap: 488.694s -> 487.103s (-0.33%)
Artifact size: 389.00 MiB -> 389.06 MiB (0.01%)

[jdonszelmann]

Well, that's good across the board. The extra relate doesn't cause too much damage. @lcnr I tried to get it right, but I'm afraid the new type relation isn't super sophisticated and I'm afraid I'm forgetting some subtle cases I'm not aware of or something.

@rustbot review

[lcnr]

can move this map right after the fudge_inference_if_okay instead of after the unwrap_or_default. The fact that it comes after is slightly confusing to me, even if it doesn't matter too much.

View changes since the review

[lcnr]

should mention why we do this :>

View changes since the review

[lcnr]

cool stuff, wanna write an FCP proposal? :>

View changes since this review

[jdonszelmann]

Let's do it. This PR is a 2nd attempt at making eager resolving the default.

Context about resolving and root variables

Resolving a type variable (roughly) means that if a variable has been equated with a concrete type, we return the concrete type. We currently have two flavors of resolving: shallow_resolve and eager resolving (try_resolve_ty_var). They differ in what they do when a type variable has not been equated with a concrete type yet.

Type variables are part of a union find data structure, and each variable has a single "root" variable that represents the set of equated variables. Eager resolving makes sure that when resolving a type variable, the root variable is returned
shallow_resolve instead does a bit of logic to ensure that TyVar and Int/float vars play nice together, but otherwise doesn't look up the root of a type variable.

It turns out that resolving to the root is quite nice for caching reasons: if we more often get the exact same type variable show up, as opposed to various type variables that happen to be equated in the union find, we get more cache hits. This matters more for the old solver than the new, since the new solver already often resolves to the root variable.

Context about fudging

Fudging is a process (though might be better described as a hack) that makes code that looks like it should be type inferred correctly, actually type infer correctly. An example I've seen often that relies on fudging is the following:

fn main() {
    let y: Box<dyn Fn(&str) -> usize> = Box::new(|x| x.len());
}

To know what x.len() means, we have to use the expected type from the let binding. However, if we were to propagate this expected through the Box, we'd infer that Box::new()'s input parameter type is dyn Fn ... which doesn't implement Sized, despite that being a requirement on Box::new().

Fudging fixes this by, unifying the expected type with the actual return type, ignoring the Sized bound completely. Then we roll back most inference progress we made (because this all runs in a probe), and replace any type variable remaining in the new (fudged) expected type with fresh ones.

In essence, we do want to keep the fact that x: &str, but any constraints on type variables that aren't concrete types should be discarded. For more details, see fudge_inference_if_ok.

Note that even without fudging there was already a way to write the above code. You could simply give a type hint of x: &str on the right hand side for example.

This PR

This PR changes shallow_resolving to also return root type variables. In the near future, we can likely remove the difference between shallow and eager resolving altogether, having only one resolve function.

We previously landed and reverted this change because there were regressions.

Fudging relies on shallow resolving working the way it does when replacing type variables with fresh ones (see #153849 and #153816). An earlier attempt at mitigating these regressions was to use eager resolving everywhere except in fudging. We reverted this.

This PR's last commit tries a different approach to mitigate the regressions: Sometimes, fudging changes the expected type very minimally. No inference progress is made, though despite that we still replace all type variables with new fresh ones at the end of fudging. This loses information when the original unfudged expected type contained type variables with some existing constraints on them. So, we check this! If the only change between the unfudged and fudged expected type is changes in type variables, i.e. no structural changes, we keep the original unfudged expected type.

This fixes both crater failures, and likely makes more code compile in general. But it's not perfect. It's a band-aid that happens to address exactly these crater failures. We still switch to eager resolving in fudging, which means there is possibly still code out there that used to compile and doesn't anymore. Just nothing that's visible to crater.

FCP

This fcp proposes to accept this (likely extremely rare) breakage. Eager resolving breaking fudging is already quite rare, and with the band aid of this PR, seems very unlikely to exist at all in real codebases.

Notably, this gains us quite a bit of performance, and consistency in the code base, allowing us to remove the shallow resolving API.

@rfcbot fcp merge types

[rust-rfcbot]

@jdonszelmann has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @jackh726
• @lcnr
• @oli-obk
• @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[lcnr]

A bit more detail about the regressions caused by resolving to root vars during fudging:

pub struct Server<T>(*mut T);
impl<T> Server<T> {
    fn new(_: T) -> Self
    where
        // Must be higher-ranked
        T: Fn(&mut i32),
    {
        todo!()
    }
}

fn main() {
    // Must have a type annotation
    let _: Server<_> = Server::new(|_| ());
}

here we equate the return type Server<?t> with the expectation Server<?exp>. This unifies ?t with ?exp. If we now resolve the type of the function argument to its root var, we resolve the argument to ?exp. While ?exp is equal to ?t in the fudging scope, they won't be equal anymore after leaving the probe.

We need the ?expected_closure_ty: Fn(&mut i32) where-bound to correctly infer the closure signature. We only have this where-bound on ?t, but not on ?exp. This means that resolving ?t to ?exp inside of the fudging scope (where they are equal and it shouldn't matter) does matter because outside of the fudging scope, these two infer vars are equal.

I think fudging is cursed enough for me to not really care too much about any remaining theoretical breakage here. I think the approach by @jdonszelmann in the last commit is good enough and after chatting about this issue for a while, we haven't been able to come up with a more principled solution here. Fudging is definitely something we do in rustc

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rust-bors]

☔ The latest upstream changes (presumably #160238) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.