Skip to content
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

stricter hidden type wf-check #115008

Merged
merged 5 commits into from Mar 6, 2024
Merged

Conversation

aliemjay
Copy link
Member

@aliemjay aliemjay commented Aug 19, 2023

Combines #114740 and #114933 for a shared crater run, fixes #114728

#114740 should handle all the cases we unintentionally miss in #114933 and I expect #114933 to fix most of the crater regressions of #114740.

However this is still a regression:

trait Id<X> { type Ty; }
impl<X> Id<X> for () { type Ty = X; }
fn test<T>() -> impl Id<&'static T, Ty = impl Sized> {}

We should probably ignore the wf-check nested RPIT, similar to nested TAIT.

r? @ghost
cc @compiler-errors

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Aug 19, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 19, 2023

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

@aliemjay aliemjay added A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 19, 2023
@aliemjay
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Aug 19, 2023

⌛ Trying commit ca0634b with merge e7920004d5fc5c25e6a81a373abcc9e5161ff603...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 19, 2023

☀️ Try build successful - checks-actions
Build commit: e7920004d5fc5c25e6a81a373abcc9e5161ff603 (e7920004d5fc5c25e6a81a373abcc9e5161ff603)

@aliemjay
Copy link
Member Author

(p=1 because it's only a small list)
@craterbot check p=1 crates=https://crater-reports.s3.amazonaws.com/pr-114740/retry-regressed-list.txt

@craterbot
Copy link
Collaborator

👌 Experiment pr-115008 created and queued.
🤖 Automatically detected try build e7920004d5fc5c25e6a81a373abcc9e5161ff603
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Aug 19, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-115008 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-115008 is completed!
📊 6 regressed and 0 fixed (298 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 20, 2023
oli-obk added a commit to oli-obk/rs-matter that referenced this pull request Sep 21, 2023
The soundness fix in rust-lang/rust#115008 will cause axum-starter to break, even though it is not unsound. The missing bound is very hard to abuse, but still a soundness hole in our type system.

It will likely take 12 weeks before a stable compiler with the soundness fix is shipped.
oli-obk added a commit to oli-obk/schemat that referenced this pull request Sep 21, 2023
The soundness fix in rust-lang/rust#115008 will cause axum-starter to break, even though it is not unsound. The missing bound is very hard to abuse, but still a soundness hole in our type system.

It will likely take 12 weeks before a stable compiler with the soundness fix is shipped.
oli-obk added a commit to oli-obk/CardMaster that referenced this pull request Sep 21, 2023
The soundness fix in rust-lang/rust#115008 will cause `discord` to break, even though it is not unsound. The missing bound is very hard to abuse, but still a soundness hole in our type system.

It will likely take 12 weeks before a stable compiler with the soundness fix is shipped.
raviqqe pushed a commit to raviqqe/schemat that referenced this pull request Sep 21, 2023
The soundness fix in rust-lang/rust#115008 will cause schemat to break,
even though it is not unsound. The missing bound is very hard to abuse,
but still a soundness hole in our type system.

It will likely take 12 weeks before a stable compiler with the soundness
fix is shipped.
Astavie pushed a commit to Astavie/CardMaster that referenced this pull request Sep 22, 2023
The soundness fix in rust-lang/rust#115008 will cause `discord` to break, even though it is not unsound. The missing bound is very hard to abuse, but still a soundness hole in our type system.

It will likely take 12 weeks before a stable compiler with the soundness fix is shipped.
@oli-obk
Copy link
Contributor

oli-obk commented Oct 11, 2023

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.

The implementation adds well-formedness predicates, which should always be sound and at worst a performance issue (#114933 does the same thing). In addition, during wf check of RPIT and async fn return types we do region checking of these opaques within their owning function, catching the remaining soundness bugs (which is what #114740 does).

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 all have PRs opened, with only one not having been noticed by the crate owners yet.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Oct 11, 2023

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 11, 2023
Comment on lines 4 to 5
//[pass] check-fail
// WARN new-solver BUG.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please instead check this to known-bug or FIXME(-Ztrait-solver=next)

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Oct 30, 2023
@rfcbot
Copy link

rfcbot commented Oct 30, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 30, 2023
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 9, 2023
@rfcbot
Copy link

rfcbot commented Nov 9, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Nov 9, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 29, 2024
stricter hidden type wf-check [based on rust-lang#115008]

Original work by `@aliemjay` in rust-lang#115008. A huge thanks to them for originally figuring out this approach ❤️

Fixes rust-lang#114728
Fixes rust-lang#114572

Instead of adding the `WellFormed` obligations when relating opaque types, I add always emit such an obligation when defining the hidden type.

This causes nested opaque types which aren't wf to error, see the comment below for the described impact. I believe this change to be desirable as it significantly reduces complexity by removing special-cases.

It also caused an issue with RPITIT: in defaulted trait methods, we add a `Projection(synthetic_assoc, rpit_of_trait_method)` clause to the `param_env`. This clause is not added to the `ParamEnv` of the nested coroutines. This caused a normalization failure in `fn check_coroutine_obligations` with the new solver. I fixed that by using the env of the typeck root instead.

r? `@oli-obk`
@bors bors merged commit 09bc67b into rust-lang:master Mar 6, 2024
23 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 6, 2024
@lcnr
Copy link
Contributor

lcnr commented Mar 7, 2024

I force pushed to aliemjays repo as well by accident? 😅 alright, have to be more careful next time 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-impl-trait Area: impl Trait. Universally / existentially quantified anonymous types with static dispatch. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-types Relevant to the types team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPIT hidden types can be ill-formed
9 participants