forbid defining opaque types with regions in closures #134688
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
lcnr
changed the title
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Dec 23, 2024
forbid defining opaque types with regions in closures We currently only allow defining opaque types if their args are exclusively local to the closure. I would like to know whether we may change it. r? `@compiler-errors`
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
presumably wanted a crater run so @craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
Finished benchmarking commit (a0cd57e): comparison URL. Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary 3.1%, secondary -0.1%)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 -1.9%)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: 763.753s -> 763.731s (-0.00%) |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
removed
the
S-waiting-on-crater
craterbot
added
the
S-waiting-on-review
|
Could you please explain the reason behind this change? Is it a limitation of the new solver? If so, is it fundamental to new solver design and cannot be reasonably mitigated? |
fn generate_parse_a<'a>() -> impl FnOnce() + 'a {
|| drop(generate_parse_a())
}Old trait solver only normalizes opaque types in their defining scope when equating them with rigid types. The new solver (and imo any reasonable design) treats them the same as associated types and always normalizes them when structurally resolving. This means we have existing code which opaquely opaque types in their defining scope inside of closures. These uses end up defining the opaque in the new solver and currently fail to prove member constraints for them. I wanted to know whether people actually define opaques using the late-bound closure lifetimes as I had some ideas which would have always mapped opaque type args to free variables of the typeck root. I doubt that this will be part of the approach we'll actually be using, and regardless, the final approach will have a proper writeup of what is being done and why. Was really surprising to see you react to this PR without even getting pinged while being otherwise unreachable. How did that happen? |
|
jup, this (apparently unproblematic) breaking change will be necessary for #135445. Or at least it's not immediately obvious to me how you'd want to support it. |
|
Is it ordinary that the "error" crates in crater result is more than 100k?
Occasionally, I scan t-types PRs and issues. Hopefully, I'll have some time for rust starting in April |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We currently only allow defining opaque types if their args are exclusively local to the closure. I would like to know whether we may change it.
r? @compiler-errors