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

Resolve region bounds from components of type projection #121602

Conversation

dingxiangfei2009
Copy link
Contributor

Fix #121601

This will stay in draft so as to resolve potential issue in design and approach, possible conflict with existing work on the type checker.

@rustbot
Copy link
Collaborator

rustbot commented Feb 25, 2024

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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. labels Feb 25, 2024
@rust-log-analyzer

This comment has been minimized.

@dingxiangfei2009
Copy link
Contributor Author

☝️ The existing build failure is a perfect example that I would like quote in a question about the well-formedness clauses which I am wondering about. I am formulating my question to be posted later in Zulip.

@compiler-errors
Copy link
Member

This should be assigned to T-types when it exits draft mode

@dingxiangfei2009 dingxiangfei2009 force-pushed the resolve-region-bounds-from-projection branch from 3e320bc to d6fdc9d Compare February 26, 2024 17:30
@dingxiangfei2009 dingxiangfei2009 marked this pull request as ready for review February 26, 2024 17:31
for &predicate in self.projection_predicates {
let Some(ty::ProjectionPredicate { term, projection_ty }) = predicate.no_bound_vars()
else {
continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

... so if we are here, we have, for now, T == for<'a, 'b, ...> <... as ...>::....

I guess it is safe to say that T: 'static?

Copy link
Member

Choose a reason for hiding this comment

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

I guess it is safe to say that T: 'static?

Why is that true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I retract. It is not necessarily.

@rust-log-analyzer

This comment has been minimized.

@dingxiangfei2009 dingxiangfei2009 force-pushed the resolve-region-bounds-from-projection branch from d6fdc9d to 0f8777e Compare February 26, 2024 21:23
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, but I'd personally lean towards closing this in favor of first having a discussion about whether we even want to support this at all.

Essentially this PR is a attempt to extend RFC 1214's projection rules (https://github.com/rust-lang/rfcs/blob/master/text/1214-projections-lifetimes-and-wf.md#outlives-for-projections) with a new rule:

TypeOutlivesViaBackwardsProjection:
  ∀i. R ⊢ Pi: 'a
  <P0 as Trait<P1..Pn>>::Assoc normalizes-to T
  --------------------------------------------------
  R ⊢ Ty: 'a

In order to fix code that looks like:

trait Foo: Bar {
    fn hello<'a>(&'a self) {}
}

trait Bar {
    type Assoc;
}

// impl<T> Foo for T where T: Bar // WORKS
impl<T, A> Foo for T where T: Bar<Assoc = A> // BROKEN
{
    fn hello<'a>(&'a self) {
        outlives::<'a, Self::Assoc>();
    }
}

fn outlives<'a, T: 'a>() {}

This is an attempt to fix a subset of a more general problem with the type system, where normalization only proceeds in one direction, even when considering an unnormalized type may allow us to prove goals that we cannot on the normalized type.

I don't believe this is unsound, as long as we're enforcing the same things that make RFC 1214's OutlivesProjectionComponents sound (i.e. projection must not give us unconstrained lifetimes).

However, I'm still generally concerned that this doesn't interact well with higher-ranked projection bounds, and is still quite limited in the exact matching it can do (consider, e.g., a chain of two projection bounds), and I don't know if it's worth the extra maintenance burden to fix this specific case, since I assume it's hit only very rarely and can be worked around.


Side-note: It reminds me of another limitation where we do not consider alias bounds when we are able to project to a rigid type, even if that rigid type doesn't have the same assumptions:

trait Foo {
    type Bar: Qux;
}

trait Qux {
    fn hello();
}

fn doesnt_work<T: Foo<Bar = U>, U>() {
    T::Bar::hello(); // <~ `T::Bar` becomes `U`, for which we know nothing.
}

fn works<T: Foo>() {
    T::Bar::hello();
}

for &predicate in self.projection_predicates {
let Some(ty::ProjectionPredicate { term, projection_ty }) = predicate.no_bound_vars()
else {
continue;
Copy link
Member

Choose a reason for hiding this comment

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

I guess it is safe to say that T: 'static?

Why is that true?

@estebank
Copy link
Contributor

r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned estebank Feb 26, 2024
@oli-obk oli-obk 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 Feb 27, 2024
@compiler-errors
Copy link
Member

As I noted above, I'd like to close this since I don't believe the fix warrants the additional complications. If another T-types member would like to shepherd this work instead, feel free to cancel the FCP and take it over.

@rfcbot fcp close

@rfcbot
Copy link

rfcbot commented Feb 28, 2024

Team member @compiler-errors has proposed to close 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-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Feb 28, 2024
@compiler-errors compiler-errors added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 28, 2024
@jackh726
Copy link
Member

I think a good place to prototype this kind of change would be a-mir-formality. I'm not sure if we're quite there yet, but that's where the types team wants to get to when thinking about changes that fundamentally interact with type and borrow checking.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 29, 2024
@rfcbot
Copy link

rfcbot commented Feb 29, 2024

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

@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 Mar 10, 2024
@rfcbot
Copy link

rfcbot commented Mar 10, 2024

The final comment period, with a disposition to close, 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.

@oli-obk oli-obk closed this Mar 10, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
9 participants