refactor and cleanup the leak check, add it to new solver #111881

lcnr · 2023-05-23T17:41:35Z

ended up being a bit more involved than I wanted but is hopefully still easy enough to review as a single PR, can split it into separate ones otherwise.

this can be reviewed commit by commit:
a473d55cdb9284aa2b01282d1b529a2a4d26547b 31a686646534ca006d906ec757ece4e771d6f973 949039c107852a5e36361c08b62821a0613656f5 242917bf5170d9a723c6c8e23e9d9d0c2fa8dc9d ed2b25a7aa28be3184be9e3022c2796a30eaad87 are all pretty straightforward.

03dd83b4c3f4ff27558f5c8ab859bd9f83db1d04 makes it easier to refactor coherence in a later commit, see the commit description, cc @oli-obk

4fe311d807a77b6270f384e41689bf5d58f46aec I don't quite remember what we wanted to test here, this definitely doesn't test that the occurs check doesn't cause incorrect errors in coherence, also cc @oli-obk here. I may end up writing a new test for this myself later.

5c200d88a91b75bd0875b973150655bd581ef97a is the main refactor of the leak check, changing it to take the outer_universe instead of getting it from a snapshot. Using a snapshot requires us to be in a probe which we aren't in the new solver, it also just feels dirty as snapshots don't really have anything to do with universes.

with all of this cfc230d54188d9c7ed867a9a0d1f51be77b485f9 is now kind of trivial.

r? @nikomatsakis

rustbot · 2023-05-23T17:41:43Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

lcnr · 2023-05-23T18:45:07Z

@bors try @rust-timer queue

bors · 2023-05-23T18:45:17Z

⌛ Trying commit ed2b25a7aa28be3184be9e3022c2796a30eaad87 with merge b409a3a7f3fb1da87acf288912de6839c6678e29...

bors · 2023-05-23T20:56:27Z

☀️ Try build successful - checks-actions
Build commit: b409a3a7f3fb1da87acf288912de6839c6678e29 (b409a3a7f3fb1da87acf288912de6839c6678e29)

rust-timer · 2023-05-23T22:13:48Z

Finished benchmarking commit (b409a3a7f3fb1da87acf288912de6839c6678e29): comparison URL.

Overall result: ✅ improvements - no 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.

@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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-1.3%	[-1.8%, -0.9%]	7
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	1

Max RSS (memory usage)

Results

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)	4.7%	[4.7%, 4.7%]	1
Regressions ❌ (secondary)	2.2%	[2.1%, 2.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	4.7%	[4.7%, 4.7%]	1

Cycles

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

Binary size

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

Bootstrap: 646.927s -> 647.248s (0.05%)

jackh726 · 2023-05-25T03:04:01Z

If you want to split them out, r=me on the first 4 commits

nikomatsakis · 2023-05-26T13:38:50Z

r=me on the PR

lcnr · 2023-05-29T14:02:04Z

@bors r=nikomatsakis,jackh726

bors · 2023-05-29T14:02:06Z

📌 Commit f0b4c63e7d4d5887a35913fa086630f218d49ddf has been approved by nikomatsakis,jackh726

It is now in the queue for this repository.

bors · 2023-05-30T01:28:46Z

🔒 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 leak-check (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 status if you got lost).
git push self leak-check --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

Removing tests/ui/higher-rank-trait-bounds/issue-95230.rs
Removing tests/ui/coherence/coherence-inherited-subtyping.re.stderr
Auto-merging src/tools/tidy/src/ui_tests.rs
CONFLICT (content): Merge conflict in src/tools/tidy/src/ui_tests.rs
Auto-merging compiler/rustc_hir_typeck/src/coercion.rs
Automatic merge failed; fix conflicts and then commit the result.

we can otherwise assign a hidden type to the opaque which causes ICE if we don't use `take_opaque_types` during coherence. This is annoying so I didn't bother. Added a test showing the behavior this prevents.

clean up coherence to not rely on probes anymore

lcnr · 2023-05-30T11:15:03Z

@bors r=nikomatsakis,jackh726

bors · 2023-05-30T11:15:04Z

📌 Commit 0b81f99 has been approved by nikomatsakis,jackh726

It is now in the queue for this repository.

bors · 2023-05-30T18:48:15Z

⌛ Testing commit 0b81f99 with merge f0411ff...

bors · 2023-05-30T21:26:06Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis,jackh726
Pushing f0411ff to master...

rust-timer · 2023-05-30T23:54:19Z

Finished benchmarking commit (f0411ff): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.3%	[-1.7%, -0.9%]	6
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

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

Cycles

Results

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.6%	[2.1%, 3.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 644.295s -> 642.846s (-0.22%)

rustbot assigned nikomatsakis May 23, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels May 23, 2023