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

fix(generic_const_exprs): Fix predicate inheritance for children of opaque types #99801

Conversation

Neo-Zhixing
Copy link
Contributor

@Neo-Zhixing Neo-Zhixing commented Jul 27, 2022

Fixes #99705

We currently have a special case to perform predicate inheritance when the const item is in the generics. I think we're also going to need this for opaque return types. When evaluating the predicates applied to the associated item, it'll inherit from its parent, the opaque type, which will never have predicates applied. This PR bypass the opaque typed parent and inherit predicates directly from the function itself.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 27, 2022
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2022
@Neo-Zhixing
Copy link
Contributor Author

r? @BoxyUwU

@Neo-Zhixing
Copy link
Contributor Author

@rustbot label +A-const-generics +F-generic_const_exprs

@rustbot rustbot added A-const-generics Area: const generics (parameters and arguments) F-generic_const_exprs `#![feature(generic_const_exprs)]` labels Jul 27, 2022
@BoxyUwU
Copy link
Member

BoxyUwU commented Jul 28, 2022

I suspect this probably doesnt work right for -> impl Foo<Assoc = impl Bar = Baz<{ T::ASSOC }>>. I am not that familiar with opaque type stuff so going to r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned BoxyUwU Jul 28, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Jul 28, 2022

I think this will get resolved by @spastorino's work which removes this parent generics behaviour of return position impl trait. This PR is just more proof that doing that is important. It feels like a band-aid that I would prefer not to merge

@Neo-Zhixing
Copy link
Contributor Author

@oli-obk Sure, I'm OK with closing this for now and wait for the RPIT refactor to happen. Do you have a pointer to where that is happening & the timeline? Just want to get this bug properly documented here so that other people don't end up wasting time on this.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 28, 2022

I don't know where @spastorino is tracking this work. I'll let him post a link in case there's a hackmd.

We could link #99705 from the generic_const_exprs tracking issue so it doesn't get lost.

@Neo-Zhixing
Copy link
Contributor Author

Ok sounds good. Let's keep this PR open in the mean time so that those who need this now can use the branch as a custom toolchain.

@oli-obk oli-obk added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jul 29, 2022
@spastorino
Copy link
Member

For some reason I've missed this one. Anyway, my changes are close to be completed. Right now I'm fixing some bugs on my branch.

@spastorino
Copy link
Member

PR with RPIT changes is now up #101345, we still need to address some problems that crater have found.

@bors
Copy link
Contributor

bors commented Sep 27, 2022

☔ The latest upstream changes (presumably #102306) made this pull request unmergeable. Please resolve the merge conflicts.

@pnkfelix
Copy link
Member

discussed in T-compiler triage meeting.

reassigning ownership for design decisions here to T-types.

@rustbot label: -T-compiler +T-types

@rustbot rustbot added 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 Oct 27, 2022
@oli-obk oli-obk removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Oct 27, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Oct 31, 2022

I think we should "just do this PR" and remove the changes when we'll actually change the opaque type parenting logic.

So... after much work that didn't get merged after all, @Neo-Zhixing can you rebase this PR, we can then merge it.

@Neo-Zhixing
Copy link
Contributor Author

@oli-obk changes merged with master. Thanks!

@compiler-errors
Copy link
Member

@Neo-Zhixing we don't actually allow merge conflicts in the rust repo: https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy

Can you rebase this commit instead?

@Neo-Zhixing Neo-Zhixing force-pushed the fix/generic_const_exprs_parent_opaque_predicates branch from 25a0dd5 to ccd9e07 Compare November 1, 2022 00:02
@Neo-Zhixing
Copy link
Contributor Author

My apologies, I thought GitHub would be able to squash those commits for you.
Rebased. Thanks @compiler-errors

@oli-obk
Copy link
Contributor

oli-obk commented Nov 1, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Nov 1, 2022

📌 Commit ccd9e07281b43567de28ad9f6601c49464e44bd6 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 1, 2022
@BoxyUwU
Copy link
Member

BoxyUwU commented Nov 1, 2022

did we test if -> impl Foo<Assoc = impl Bar<{ T::ASSOC }>> is fixed by this PR?

#![feature(generic_const_exprs)]

trait Foo {
    type Assoc;
}

trait Bar<const N: usize> {}

impl Foo for () {
    type Assoc = ();
}
impl<const N: usize> Bar<N> for () {}

trait Trait {
    const ASSOC: usize;
}

fn foo<T: Trait>() -> impl Foo<Assoc = impl Bar<{ T::ASSOC }>> {}

@oli-obk
Copy link
Contributor

oli-obk commented Nov 1, 2022

@bors r- let's add the test, but I think it should work

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 1, 2022
@Neo-Zhixing
Copy link
Contributor Author

@rustbot label -S-waiting-on-author +S-waiting-on-review
@oli-obk @BoxyUwU Test added for the nested impl. Indeed it works! Thanks

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 1, 2022
@Neo-Zhixing Neo-Zhixing force-pushed the fix/generic_const_exprs_parent_opaque_predicates branch from a553575 to 744fa61 Compare November 1, 2022 22:41
@oli-obk
Copy link
Contributor

oli-obk commented Nov 2, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Nov 2, 2022

📌 Commit 744fa61 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 2, 2022
@BoxyUwU
Copy link
Member

BoxyUwU commented Nov 2, 2022

Is there no way to get nested opaque items in current rustc 🤔 that was what i was going for with -> impl Trait<Assoc = impl Other<{ T::ASSOC }>>. Judging by the fact that the code i posted compiles it kind of implies that is nested opaques so I am slightly confused as to how this PR fixes that ✨

@oli-obk
Copy link
Contributor

oli-obk commented Nov 2, 2022

Is there no way to get nested opaque items in current rustc

yes there is. Your example does indeed show that, but it's not an issue because the inner opaque item fetches the predicates of the outer one, which fetches it from the function.

@BoxyUwU
Copy link
Member

BoxyUwU commented Nov 2, 2022

I thought the bug was that opaque type items dont have all the predicates of the parent, so anon consts inheriting predicates from the opaque is incorrect since they should have "all" of them. Clearly i've misunderstood something here since if that was true the anon const having a parent of the outter opaque type would not be correct 😅

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 3, 2022
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#99801 (fix(generic_const_exprs): Fix predicate inheritance for children of opaque types)
 - rust-lang#103610 (Allow use of `-Clto=thin` with `-Ccodegen-units=1` in general)
 - rust-lang#103870 (Fix `inferred_kind` ICE)
 - rust-lang#103875 (Simplify astconv item def id handling)
 - rust-lang#103886 (rustdoc: Fix merge of attributes for reexports of local items)
 - rust-lang#103890 (rustdoc: remove unused mobile CSS `.rustdoc { padding-top: 0 }`)

Failed merges:

 - rust-lang#103884 (Add visit_fn_ret_ty to hir intravisit)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Nov 3, 2022

⌛ Testing commit 744fa61 with merge 96787c4...

@bors bors merged commit 214d6b6 into rust-lang:master Nov 3, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 3, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 24, 2024
…-ct, r=BoxyUwU

Resolve anon const's parent predicates to direct parent instead of opaque's parent

When an anon const is inside of an opaque, rust-lang#99801 added a hack to resolve the anon const's parent predicates *not* to the opaque's predicates, but to the opaque's *parent's* predicates. This is insufficient when considering nested opaques.

This means that the `predicates_of` an anon const might reference duplicated lifetimes (installed by `compute_bidirectional_outlives_predicates`) when computing known outlives in MIR borrowck, leading to these ICEs:
Fixes rust-lang#121574
Fixes rust-lang#118403

~~Instead, we should be using the `OpaqueTypeOrigin` to acquire the owner item (fn/type alias/etc) of the opaque, whose predicates we're fine to mention.~~

~~I think it's a bit sketchy that we're doing this at all, tbh; I think it *should* be fine for the anon const to inherit the predicates of the opaque it's located inside. However, that would also mean that we need to make sure the `generics_of` that anon const line up in the same way.~~

~~None of this is important to solve right now; I just want to fix these ICEs so we can land rust-lang#125468, which accidentally fixes these issues in a different and unrelated way.~~

edit: We don't need this special case anyways because we install the right parent item in `generics_of` anyways:
https://github.com/rust-lang/rust/blob/213ad10c8f0fc275648552366275dc4e07f97462/compiler/rustc_hir_analysis/src/collect/generics_of.rs#L150

r? `@BoxyUwU`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 24, 2024
Rollup merge of rust-lang#125501 - compiler-errors:opaque-opaque-anon-ct, r=BoxyUwU

Resolve anon const's parent predicates to direct parent instead of opaque's parent

When an anon const is inside of an opaque, rust-lang#99801 added a hack to resolve the anon const's parent predicates *not* to the opaque's predicates, but to the opaque's *parent's* predicates. This is insufficient when considering nested opaques.

This means that the `predicates_of` an anon const might reference duplicated lifetimes (installed by `compute_bidirectional_outlives_predicates`) when computing known outlives in MIR borrowck, leading to these ICEs:
Fixes rust-lang#121574
Fixes rust-lang#118403

~~Instead, we should be using the `OpaqueTypeOrigin` to acquire the owner item (fn/type alias/etc) of the opaque, whose predicates we're fine to mention.~~

~~I think it's a bit sketchy that we're doing this at all, tbh; I think it *should* be fine for the anon const to inherit the predicates of the opaque it's located inside. However, that would also mean that we need to make sure the `generics_of` that anon const line up in the same way.~~

~~None of this is important to solve right now; I just want to fix these ICEs so we can land rust-lang#125468, which accidentally fixes these issues in a different and unrelated way.~~

edit: We don't need this special case anyways because we install the right parent item in `generics_of` anyways:
https://github.com/rust-lang/rust/blob/213ad10c8f0fc275648552366275dc4e07f97462/compiler/rustc_hir_analysis/src/collect/generics_of.rs#L150

r? `@BoxyUwU`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-generics Area: const generics (parameters and arguments) F-generic_const_exprs `#![feature(generic_const_exprs)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No associated item found when the associated item was used as const generic parameter in return type
9 participants