Canonicalize free regions from inputs as placeholders in root univ#155487
Canonicalize free regions from inputs as placeholders in root univ#155487ShoyuVanilla wants to merge 1 commit intorust-lang:mainfrom
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.
Canonicalize free regions from inputs as placeholders in root univ
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e39d9d1): comparison URL. Overall result: ✅ improvements - 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 countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 2.8%, secondary 3.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -3.2%, secondary 0.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 493.313s -> 494.404s (0.22%) |
3b462e7 to
bfdd538
Compare
|
r? lcnr |
| = note: closure with signature `for<'a> fn(&'a &'2 str) -> bool` must implement `FnOnce<(&&'1 str,)>`, for any lifetime `'1`... | ||
| = note: ...but it actually implements `FnOnce<(&&'2 str,)>`, for some specific lifetime `'2` | ||
| = note: closure with signature `for<'a> fn(&'a &'0 str) -> bool` must implement `FnOnce<(&&'1 str,)>`, for any two lifetimes `'0` and `'1`... | ||
| = note: ...but it actually implements `FnOnce<(&&str,)>` |
There was a problem hiding this comment.
We have lots of those ...but it actually implements X for some specific lifetime '?x -> ...but it actually implements X for some specific lifetime diagnostic drifts due to the canonicalized region being placeholders rather than region vars.
I think this is somewhat regressive. Would it make sense to fix them somehow?
|
I'm not sure I should do something with diagnostics but marking this as ready 😅 |
| if !(placeholder.universe() == ty::UniverseIndex::ROOT | ||
| && placeholder.bound.kind == ty::BoundRegionKind::Anon) | ||
| && max_input_universe.can_name(placeholder.universe()) |
There was a problem hiding this comment.
shouldn't just placeholder.universe() != ty::UniverseIndex::ROOT be enough here. We should never add new placeholders to an existing universe and max_input_universe is guaranteed to be able to the root universe, as can_name is equivalent to max_input_universe >= placeholder.universe()
There was a problem hiding this comment.
Wouldn't there still be the cases that adding new placeholders to a new_universe > max_input_universe?
There was a problem hiding this comment.
ah, yeah, it's the other way around
- either the placeholder is in the root universe
- or from a universe which can't be named by
max_input_universe
There was a problem hiding this comment.
2 thoughts here:
It's somewhat scary that this requires literally no changes when we instantiate query responses. I guess what's happening is that this is (another) load bearing use of query_response_instantiation_guess (old solver) and the hack in compute_query_response_instantiation_values (next solver). We should definitely at least update the comments here.
Related to the above, it feels scary to me to change the old solver canonicalization here. I think your change also makes the different CanonicalizeMode be slightly outdated or what not 🤔 could you just limit this to the new solver for now?
CanonicalizeUserTypeAnnotations is interesting to me as that one is more a canonicalize_response where the var_values are just the early/late bound params in the parent function
|
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 try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Canonicalize free regions from inputs as placeholders in root univ
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (ea40b77): comparison URL. Overall result: ✅ improvements - 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 countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary -6.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis perf run didn't have relevant results for this metric. Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 487.764s -> 486.922s (-0.17%) |
Context: The box named coroutine witness Send lifetime requirements now considered by leakcheck this roadmap
Fixes (only for the next-solver) #106569
Prerequisite of #155749