-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
Shallow resolve ty vars to their root var #147193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
cool just weird diagnostics junk |
|
@bors try |
This comment has been minimized.
This comment has been minimized.
Shallow resolve ty vars to their root var
|
@rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (dd7e4e9): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -3.3%, secondary -2.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.7%, secondary -3.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 471.179s -> 471.5s (0.07%) |
| // dynamic borrow errors on `self.inner`. | ||
| let known = self.inner.borrow_mut().type_variables().probe(v).known(); | ||
| known.map_or(ty, |t| self.shallow_resolve(t)) | ||
| known.map_or(Ty::new_var(self.tcx, self.root_var(v)), |t| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use map_or_else to only look up the root var if it's actually going to be used
misc coercion cleanups and handle safety correctly r? lcnr ### "remove normalize call" Fixes #132765 If the normalization fails we would sometimes get a `TypeError` containing inference variables created inside of the probe used by coercion. These would then get leaked out causing ICEs in diagnostics logic ### "leak check and lub for closure<->closure coerce-lubs of same defids" Fixes rust-lang/trait-system-refactor-initiative#233 ```rust fn peculiar() -> impl Fn(u8) -> u8 { return |x| x + 1 } ``` the `|x| x + 1` expr has a type of `Closure(?31t)` which we wind up inferring the RPIT to. The `CoerceMany` `ret_coercion` for the whole `peculiar` typeck has an expected type of `RPIT` (unnormalized). When we type check the `return |x| x + 1` expr we go from the never type to `Closure(?31t)` which then participates in the `ret_coercion` giving us a `coerce-lub(RPIT, Closure(?31t))`. Normalizing `RPIT` gives us some `Closure(?50t)` where `?31t` and `?50t` have been unified with `?31t` as the root var. `resolve_vars_if_possible` doesn't resolve infer vars to their roots so these wind up with different structural identities so the fast path doesn't apply and we fall back to coercing to a `fn` ptr. cc #147193 which also fixes this New solver probably just gets more inference variables here because canonicalization + generally different approach to normalization of opaques. Idk :3 ### FCP worthy stuffy there are some other FCP worthy things but they're in my FCP comment which also contains some analysis of the breaking nature of the previously listed changes in this PR: #148602 (comment)
misc coercion cleanups and handle safety correctly r? lcnr ### "remove normalize call" Fixes rust-lang/rust#132765 If the normalization fails we would sometimes get a `TypeError` containing inference variables created inside of the probe used by coercion. These would then get leaked out causing ICEs in diagnostics logic ### "leak check and lub for closure<->closure coerce-lubs of same defids" Fixes rust-lang/trait-system-refactor-initiative#233 ```rust fn peculiar() -> impl Fn(u8) -> u8 { return |x| x + 1 } ``` the `|x| x + 1` expr has a type of `Closure(?31t)` which we wind up inferring the RPIT to. The `CoerceMany` `ret_coercion` for the whole `peculiar` typeck has an expected type of `RPIT` (unnormalized). When we type check the `return |x| x + 1` expr we go from the never type to `Closure(?31t)` which then participates in the `ret_coercion` giving us a `coerce-lub(RPIT, Closure(?31t))`. Normalizing `RPIT` gives us some `Closure(?50t)` where `?31t` and `?50t` have been unified with `?31t` as the root var. `resolve_vars_if_possible` doesn't resolve infer vars to their roots so these wind up with different structural identities so the fast path doesn't apply and we fall back to coercing to a `fn` ptr. cc rust-lang/rust#147193 which also fixes this New solver probably just gets more inference variables here because canonicalization + generally different approach to normalization of opaques. Idk :3 ### FCP worthy stuffy there are some other FCP worthy things but they're in my FCP comment which also contains some analysis of the breaking nature of the previously listed changes in this PR: rust-lang/rust#148602 (comment)
misc coercion cleanups and handle safety correctly r? lcnr ### "remove normalize call" Fixes rust-lang/rust#132765 If the normalization fails we would sometimes get a `TypeError` containing inference variables created inside of the probe used by coercion. These would then get leaked out causing ICEs in diagnostics logic ### "leak check and lub for closure<->closure coerce-lubs of same defids" Fixes rust-lang/trait-system-refactor-initiative#233 ```rust fn peculiar() -> impl Fn(u8) -> u8 { return |x| x + 1 } ``` the `|x| x + 1` expr has a type of `Closure(?31t)` which we wind up inferring the RPIT to. The `CoerceMany` `ret_coercion` for the whole `peculiar` typeck has an expected type of `RPIT` (unnormalized). When we type check the `return |x| x + 1` expr we go from the never type to `Closure(?31t)` which then participates in the `ret_coercion` giving us a `coerce-lub(RPIT, Closure(?31t))`. Normalizing `RPIT` gives us some `Closure(?50t)` where `?31t` and `?50t` have been unified with `?31t` as the root var. `resolve_vars_if_possible` doesn't resolve infer vars to their roots so these wind up with different structural identities so the fast path doesn't apply and we fall back to coercing to a `fn` ptr. cc rust-lang/rust#147193 which also fixes this New solver probably just gets more inference variables here because canonicalization + generally different approach to normalization of opaques. Idk :3 ### FCP worthy stuffy there are some other FCP worthy things but they're in my FCP comment which also contains some analysis of the breaking nature of the previously listed changes in this PR: rust-lang/rust#148602 (comment)
r? lcnr
just want to see CI