Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Introduce `Unnormalized` wrapper
This comment has been minimized.
This comment has been minimized.
|
@bors try cancel |
|
Try build cancelled. Cancelled workflows: |
309b0de to
a616210
Compare
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Introduce `Unnormalized` wrapper
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (10d0ef4): comparison URL. Overall result: no relevant changes - no action neededBenchmarking 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 Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (primary -2.4%, secondary -6.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 492.281s -> 489.321s (-0.60%) |
3b64c2b to
5407a88
Compare
|
r? @lcnr |
There was a problem hiding this comment.
Things I'd like to see in this PR maybe?
TypeErrCtxt::normalize_fn_sigshould takeUnnormalized. that should simplify the diffexplicit_item_self_boundsis an iterator overUnnormalized<(Whatever, Span)>, it should also have the span outside of it
Things for future PRs
- I dislike
.map(|(c, s)| (c.skip_norm_wip(), s))as a pattern, we shouldskip_norm_wiponce we iterate over the collection - all of the
t == cx.tcx.normalize_erasing_regions(cx.typing_env(), t)asserts should be someassert_fully_normalizedand don't useUnnormalized::new_wip - a bunch of these normalize calls in codegen are unnecessary. we should change them to use
assert_fully_normalizedinstead - a big one is
struct_tail_rawwhose normalize should take anUnnormalized - all calls to
skip_norm_wip().def_idshould be replaced with adef_id()function onUnnormalized<Whateverhasadefid> - we should add
fn Unnormalized::<T>::map<U>(self, f: impl FnOnce(T) -> U) -> Unnormalized<U> - unsure what to do in
type_ofandpredicates_ofas these queries very intentionally ferry unnormalized types around. I guess we just have a.skip_normalization()for places where it's intentional 🤔 - also slightly unsure about elaborate, I think it should take
Unnormalizedas it also just returns unnormalized things? field.tyshould returnUnnormalizednormalize_with_depth_toshould takeUnnormalized🤔coherence::ImplHeadershould its things beUnnormalizedI think?DropckConstraintshould haveUnnormalizedfields? 🤔- the
DeeplyNormalizeQueryTypeOpshould contain anUnnormalized<T>as input
|
reviewed all changes, and would like to merge this soon 😁 makes it easier to work off it |
|
Do we need to make the |
|
fine to do it later 👍 |
Other than that, I should've resolved all review comments. |
This comment has been minimized.
This comment has been minimized.
177d0f9 to
a9214f5
Compare
This comment has been minimized.
This comment has been minimized.
| let mut hybrid_preds: Vec<_> = | ||
| impl_predicates.instantiate_identity(tcx).predicates.into_iter().collect(); |
There was a problem hiding this comment.
? what is this doing
There was a problem hiding this comment.
Ah, sorry, I reordered the code a bit. This collect isn't needed anymore.
| let impl_args = self.fresh_args_for_item(base_expr.span, impl_def_id); | ||
| let impl_trait_ref = | ||
| self.tcx.impl_trait_ref(impl_def_id).instantiate(self.tcx, impl_args); | ||
| let impl_trait_ref = self | ||
| .tcx | ||
| .impl_trait_ref(impl_def_id) | ||
| .instantiate(self.tcx, impl_args) | ||
| .skip_norm_wip(); | ||
| let cause = self.misc(base_expr.span); | ||
|
|
||
| // Match the impl self type against the base ty. If this fails, | ||
| // we just skip this impl, since it's not particularly useful. | ||
| let impl_trait_ref = ocx.normalize(&cause, self.param_env, impl_trait_ref); | ||
| let impl_trait_ref = | ||
| ocx.normalize(&cause, self.param_env, Unnormalized::new_wip(impl_trait_ref)); |
There was a problem hiding this comment.
that one is just skip_Norm_wip into new_wip
| let impl_args = self.fresh_args_for_item(base_expr.span, impl_def_id); | ||
| let impl_trait_ref = | ||
| self.tcx.impl_trait_ref(impl_def_id).instantiate(self.tcx, impl_args); | ||
| let impl_trait_ref = self | ||
| .tcx | ||
| .impl_trait_ref(impl_def_id) | ||
| .instantiate(self.tcx, impl_args) | ||
| .skip_norm_wip(); | ||
| let cause = self.misc(base_expr.span); | ||
|
|
||
| // Match the impl self type against the base ty. If this fails, | ||
| // we just skip this impl, since it's not particularly useful. | ||
| let impl_trait_ref = ocx.normalize(&cause, self.param_env, impl_trait_ref); | ||
| let impl_trait_ref = | ||
| ocx.normalize(&cause, self.param_env, Unnormalized::new_wip(impl_trait_ref)); |
| /// A wrapper for value that needs normalization. | ||
| /// FIXME: This is very WIP. The plan is to replace the `skip_norm_wip` spread | ||
| /// throughout the codebase with proper normalization. This is the first step toward | ||
| /// switching to eager normalization with the next solver. See the normalization refactor plan | ||
| /// [here](https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/Eager.20normalization.2C.20ahoy.21/with/582996293) | ||
| /// We're in a weird intermediate state due to the change is big to land in a single PR. | ||
| /// A lot of active work is going on which is tracked in #155345. | ||
| /// While this is in progress, if you encounter this type, just use `Unnormalized::new_wip` | ||
| /// and `Unnormalized::skip_norm_wip` as needed. | ||
| /// | ||
| /// The interner type parameter exists to constraint generic for certain impl, e.g., | ||
| /// `Unnormalized<I, I::Clause>`. |
There was a problem hiding this comment.
| /// A wrapper for value that needs normalization. | |
| /// FIXME: This is very WIP. The plan is to replace the `skip_norm_wip` spread | |
| /// throughout the codebase with proper normalization. This is the first step toward | |
| /// switching to eager normalization with the next solver. See the normalization refactor plan | |
| /// [here](https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/Eager.20normalization.2C.20ahoy.21/with/582996293) | |
| /// We're in a weird intermediate state due to the change is big to land in a single PR. | |
| /// A lot of active work is going on which is tracked in #155345. | |
| /// While this is in progress, if you encounter this type, just use `Unnormalized::new_wip` | |
| /// and `Unnormalized::skip_norm_wip` as needed. | |
| /// | |
| /// The interner type parameter exists to constraint generic for certain impl, e.g., | |
| /// `Unnormalized<I, I::Clause>`. | |
| /// A wrapper for values that need normalization. | |
| /// | |
| /// FIXME(#155345): This is very WIP. The plan is to replace the `skip_norm_wip` | |
| /// spread throughout the codebase with proper normalization. This is the first | |
| /// step toward switching to eager normalization with the next solver. See the | |
| /// normalization refactor plan [here]. | |
| /// | |
| /// We're in a weird intermediate state as the change is too big to land in a | |
| /// single PR. While this work is in progress, just use `Unnormalized::new_wip` | |
| /// and `Unnormalized::skip_norm_wip` as needed. | |
| /// | |
| /// The interner type parameter exists to constraint generic for certain impl, | |
| /// e.g., `Unnormalized<I, I::Clause>`. | |
| /// | |
| /// [here]: https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/Eager.20normalization.2C.20ahoy.21/with/582996293 |
There was a problem hiding this comment.
.explicit_super_predicates_of(def_id)
.iter_identity_copied()
.map(Unnormalized::skip_norm_wip)the skipping above is needed since iter_identitidy_copied returns iterator over Unnoramlized<(..., span)> while the next method expects pair tuple.
| let ct = lower_const_arg_for_rustdoc(cx.tcx, const_arg, cx.tcx.types.usize); | ||
| let typing_env = ty::TypingEnv::post_analysis(cx.tcx, *def_id); | ||
| let ct = cx.tcx.normalize_erasing_regions(typing_env, ct); | ||
| let ct = cx.tcx.normalize_erasing_regions(typing_env, Unnormalized::new(ct)); |
There was a problem hiding this comment.
| let ct = cx.tcx.normalize_erasing_regions(typing_env, Unnormalized::new(ct)); | |
| let ct = cx.tcx.normalize_erasing_regions(typing_env, Unnormalized::new_wip(ct)); |
so for the other uses in this file
| @@ -381,7 +382,7 @@ impl<'tcx> NonCopyConst<'tcx> { | |||
| ) -> bool { | |||
| // Make sure to instantiate all types coming from `typeck` with `gen_args`. | |||
| let ty = EarlyBinder::bind(typeck.expr_ty(e)).instantiate(tcx, gen_args); | |||
| let ty = tcx.try_normalize_erasing_regions(typing_env, ty).unwrap_or(ty); | |||
| let ty = tcx.try_normalize_erasing_regions(typing_env, ty).unwrap_or(ty.skip_normalization()); | |||
There was a problem hiding this comment.
| let ty = tcx.try_normalize_erasing_regions(typing_env, ty).unwrap_or(ty.skip_normalization()); | |
| let ty = tcx.try_normalize_erasing_regions(typing_env, ty).unwrap_or(ty.skip_norm_wip()); |
|
@bors rollup=never |
Should I just squash or isolate the API change in one commit for this big PR? Also unsure why I can't reply to single inline review. If you missed it, |
This comment has been minimized.
This comment has been minimized.
whatever u prefer, as long as the "address review" commits are gone :3 |
|
r=me |
c894a60 to
a73a9c7
Compare
|
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. |
|
@bors r=lcnr |
|
☔ The latest upstream changes (presumably #155404) made this pull request unmergeable. Please resolve the merge conflicts. This pull request was unapproved. |
View all comments
This is the first step of the eager normalization series.
This PR introduce an
Unnormalizedwrapper and make most normalization routines consume it. The purpose is to make normalization explicit.This PR contains no behavior change.
API changes are in the first two commit.
There're some normalization routines left untouched:
normalizein the type checker of borrowck: better do it together withfield.ty()returningUnnormalized.normalize_with_depth: only used inside the old solver. Can be done later.query_normalize: rarely used.The compiler errors are mostly fixed via
ast-grep, with exceptions handled manually.