cleanup TypeVerifier#134465
Conversation
| | ProjectionElem::Subslice { .. } | ||
| | ProjectionElem::Downcast(..) => {} | ||
| ProjectionElem::Field(field, fty) => { | ||
| let fty = self.typeck.normalize(fty, location); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
we wouldn't be normalizing during writeback bc we create this field projection ty during mir build, right?
There was a problem hiding this comment.
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)
}txstarts asOption<?x>- constructing handle infers
?xtoBox<dyn for<'c> Fn(&mut <?y as Fooey::Context<'c>)>(we keep the projection as it contains bound vars) - returning
Handle<?y>then constrains?ytoFooImpl - we don't normalize the type of
txagain. - without the assert, borrowck fails when equating the unnormalized with the normalized projection.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
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
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (231855a): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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 Instruction countThis 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.
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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 771.888s -> 769.352s (-0.33%) |
| | ProjectionElem::Subslice { .. } | ||
| | ProjectionElem::Downcast(..) => {} | ||
| ProjectionElem::Field(field, fty) => { | ||
| let fty = self.typeck.normalize(fty, location); |
There was a problem hiding this comment.
we wouldn't be normalizing during writeback bc we create this field projection ty during mir build, right?
| /// 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 |
|
@bors r+ |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (0eca4dd): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis 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.
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.
CyclesResults (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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 765.462s -> 763.42s (-0.27%) |
We should merge it with the
TypeCheckeras 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 theTypeCheckerto 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