-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
wf-check hidden types at definition site #114933
Conversation
Some changes occurred to the core trait solver cc @rust-lang/initiative-trait-system-refactor |
@bors try |
⌛ Trying commit 83930b0 with merge b86c3b4b0ed7ac3cd1dc08a5d86c8f78b9e21603... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☀️ Try build successful - checks-actions |
@craterbot check |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
🎉 Experiment
|
@rust-timer build b86c3b4b0ed7ac3cd1dc08a5d86c8f78b9e21603 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (b86c3b4b0ed7ac3cd1dc08a5d86c8f78b9e21603): comparison URL. Overall result: ❌ regressions - 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 a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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: 632.38s -> 635.628s (0.51%) |
this regresses All of them have updates in the last month and at least the first two seem easily fixable. |
This PR fixes a soundness hole, where we were failing to check that hidden types are actually well formed wrt to lifetimes. From a not-well formed hidden type you can easily satisfy trait bounds that make no sense and thus allow treating any lifetime as another lifetime that lives longer. Considering that the implementation does nothing but add well-formedness predicates, which should always be sound and at worst a performance issue, I do not believe there is any possible risk with this PR, beyond causing breakage for benign code that just forgot some bounds somewhere. Crater showed 3 such cases, which we should all fix before landing this. @rfcbot fcp merge |
Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
@rfcbot concern we should open PRs against the crates that will break |
Can we get specific minimal examples of the crates that are breaking? Are they potential soundness issues? |
Closing in favor of #115008, will explain the differences and reasons on that PR when starting the FCP |
I believe this is the only way to fix both linked issues.
Fixes #114728
Fixes #114572
Ideally we would do WF-check after each subtyping relation, but that's cumbersome and most likely a perf hit. On the other extreme, we can do the WF-check in
InferCtxt::register_hidden_type
but that would break the tests intests/ui/type-alias-impl-trait/wf-nested.rs
.This PR, as a middle ground, takes an advantage of the fact that the trait solver have limited occasions where subtyping occurs:
PredicateKind::{Subtype,Coerce}
: irrelevant in MIR typeck.PredicateKind::AliasRelate
: fixed in this PR.A == B => A <: B, B<: A
: we can ignore them. Why?Given all of this we can rely on the fact that the trait solver will never infer an ill-formed type if the goal input types are well-formed.
To make sure that the trait solver input types are well-formed when invoked from MIR typeck:
prove_predicate
call in MIR typeck.AliasRelate
obligations. Done inregister_type_relate_obligation
r? @compiler-errors @lcnr
cc @oli-obk