cleanup `TypeVerifier` #134465

lcnr · 2024-12-18T13:12:16Z

We should merge it with the TypeChecker as we no longer bail in cases where it encounters an error since #111863.

It's quite inconsistent whether a check lives in the verifier or the TypeChecker, so this feels like a quite impactful cleanup. I expect that for this we may want to change the TypeChecker to also be a MIR visitor 🤔 this is non-trivial so I didn't fully do it in this PR.

Best reviewed commit by commit.

r? @compiler-errors feel free to reassign however

lcnr · 2024-12-18T13:22:18Z

compiler/rustc_borrowck/src/type_check/mod.rs

+            | ProjectionElem::Subslice { .. }
+            | ProjectionElem::Downcast(..) => {}
+            ProjectionElem::Field(field, fty) => {
+                let fty = self.typeck.normalize(fty, location);


you'd think this normalize call is unnecessary, but no, it's not :3

cc #93141 :< I dislike that we end up normalizing here again instead of during writeback.

we wouldn't be normalizing during writeback bc we create this field projection ty during mir build, right?

From what I can tell we do use types from the HIR here, so the issue in the following test is:

pub trait Fooey: Sized { type Context<'c> where Self: 'c; } pub struct Handle<E: Fooey>(Option<Box<dyn for<'c> Fn(&mut E::Context<'c>)>>); fn tuple<T>() -> (Option<T>,) { (Option::None,) } pub struct FooImpl {} impl Fooey for FooImpl { type Context<'c> = &'c (); } fn fail1() -> Handle<FooImpl> { let (tx,) = tuple(); Handle(tx) }

tx starts as Option<?x>

constructing handle infers ?x to Box<dyn for<'c> Fn(&mut <?y as Fooey::Context<'c>)> (we keep the projection as it contains bound vars)

returning Handle<?y> then constrains ?y to FooImpl

we don't normalize the type of tx again.

without the assert, borrowck fails when equating the unnormalized with the normalized projection.

lcnr · 2024-12-18T13:33:03Z

@bors try @rust-timer queue

bors · 2024-12-18T13:34:15Z

⌛ Trying commit 5bced14 with merge 231855a...

cleanup `TypeVerifier` We should merge it with the `TypeChecker` as we no longer bail in cases where it encounters an error since rust-lang#111863. It's quite inconsistent whether a check lives in the verifier or the `TypeChecker`, so this feels like a quite impactful cleanup. I expect that for this we may want to change the `TypeChecker` to also be a MIR visitor 🤔 this is non-trivial so I didn't fully do it in this PR. Best reviewed commit by commit. r? `@compiler-errors` feel free to reassign however

bors · 2024-12-18T15:17:21Z

☀️ Try build successful - checks-actions
Build commit: 231855a (231855a63277f08e2be544002ae9583be4e222a1)

rust-timer · 2024-12-18T16:35:12Z

Finished benchmarking commit (231855a): 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 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
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	8
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.3%]	14
All ❌✅ (primary)	-0.3%	[-0.4%, -0.2%]	8

Max RSS (memory usage)

Results (secondary -1.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
Regressions ❌ (secondary)	3.1%	[3.1%, 3.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-4.4%, -2.3%]	2
All ❌✅ (primary)	-	-	0

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: 771.888s -> 769.352s (-0.33%)
Artifact size: 330.28 MiB -> 330.19 MiB (-0.03%)

compiler-errors · 2024-12-21T17:34:35Z

compiler/rustc_borrowck/src/type_check/mod.rs

+            | ProjectionElem::Subslice { .. }
+            | ProjectionElem::Downcast(..) => {}
+            ProjectionElem::Field(field, fty) => {
+                let fty = self.typeck.normalize(fty, location);


we wouldn't be normalizing during writeback bc we create this field projection ty during mir build, right?

compiler-errors · 2024-12-21T17:35:19Z

compiler/rustc_middle/src/mir/tcx.rs

-    /// not carry a `Ty` for `T`.)
+    /// `place_ty.field_ty(tcx, f)` computes the type of a given field.
+    ///
+    /// Most clients of `PlaceTy` can instead just extract the relevant type


"clients" 💀

compiler-errors · 2024-12-21T17:35:58Z

@bors r+

bors · 2024-12-21T17:36:01Z

📌 Commit 5bced14 has been approved by compiler-errors

It is now in the queue for this repository.

bors · 2024-12-23T04:15:44Z

⌛ Testing commit 5bced14 with merge 0eca4dd...

bors · 2024-12-23T06:54:23Z

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 0eca4dd to master...

rust-timer · 2024-12-23T08:12:00Z

Finished benchmarking commit (0eca4dd): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.2%]	6
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.1%]	16
All ❌✅ (primary)	-0.3%	[-0.4%, -0.2%]	6

Max RSS (memory usage)

Results (primary -0.8%, secondary 3.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.8%	[0.8%, 0.8%]	1
Regressions ❌ (secondary)	3.5%	[3.5%, 3.5%]	1
Improvements ✅ (primary)	-1.6%	[-2.2%, -1.0%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.8%	[-2.2%, 0.8%]	3

Cycles

Results (secondary 3.3%)

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)	3.3%	[2.9%, 3.7%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 765.462s -> 763.42s (-0.27%)
Artifact size: 330.55 MiB -> 330.55 MiB (-0.00%)

lcnr added 2 commits December 18, 2024 13:03

get_ambient_variance to inherent method

5f90b15

rm TypeChecker::sanitize_type

c54e815

rustbot assigned compiler-errors Dec 18, 2024

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. labels Dec 18, 2024

TypeVerifier: stop computing types for later use

809f55d

lcnr force-pushed the type-verifier branch from 1ddc900 to 71000f6 Compare December 18, 2024 13:13

lcnr added 2 commits December 18, 2024 14:15

merge PlaceTy field_ty computation

51cd03a

we aren't actually sanitizing anything anymore

5bced14

lcnr force-pushed the type-verifier branch from 71000f6 to 5bced14 Compare December 18, 2024 13:15

lcnr commented Dec 18, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 18, 2024

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 18, 2024

compiler-errors approved these changes Dec 21, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 21, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 23, 2024

bors merged commit 0eca4dd into rust-lang:master Dec 23, 2024
7 checks passed

rustbot added this to the 1.85.0 milestone Dec 23, 2024

bors mentioned this pull request Dec 23, 2024

best_blame_constraint: Blame better constraints when the region graph has cycles from invariance or 'static #133858

Open

lcnr deleted the type-verifier branch December 23, 2024 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup `TypeVerifier` #134465

cleanup `TypeVerifier` #134465

lcnr commented Dec 18, 2024 •

edited

Loading

lcnr Dec 18, 2024

compiler-errors Dec 21, 2024

lcnr Dec 23, 2024

lcnr commented Dec 18, 2024

This comment has been minimized.

bors commented Dec 18, 2024

bors commented Dec 18, 2024

This comment has been minimized.

rust-timer commented Dec 18, 2024

compiler-errors Dec 21, 2024

compiler-errors Dec 21, 2024

compiler-errors commented Dec 21, 2024

bors commented Dec 21, 2024

bors commented Dec 23, 2024

bors commented Dec 23, 2024

rust-timer commented Dec 23, 2024

cleanup TypeVerifier #134465

cleanup TypeVerifier #134465

Conversation

lcnr commented Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr commented Dec 18, 2024

This comment has been minimized.

bors commented Dec 18, 2024

bors commented Dec 18, 2024

This comment has been minimized.

rust-timer commented Dec 18, 2024

Overall result: ✅ improvements - no action needed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Dec 21, 2024

bors commented Dec 21, 2024

bors commented Dec 23, 2024

bors commented Dec 23, 2024

rust-timer commented Dec 23, 2024

Overall result: ✅ improvements - no action needed

cleanup `TypeVerifier` #134465

cleanup `TypeVerifier` #134465

lcnr commented Dec 18, 2024 •

edited

Loading