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

Opaque types' generic params do not imply anything about their hidden type's lifetimes #98933

Merged
merged 3 commits into from
Sep 8, 2022

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jul 5, 2022

fixes #97104

cc @aliemjay

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

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 5, 2022
Copy link
Member

@aliemjay aliemjay left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

I'm thinking if we should consider forcing the hidden type to constrain all generic params of the type alias in TAIT. If so, we won't have such weird differences between opaque types Adts. I'll discuss it further in a zulip stream https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/generic.20TAIT.20problems/near/286519266

src/test/ui/type-alias-impl-trait/constrain_inputs.stderr Outdated Show resolved Hide resolved
@cjgillot
Copy link
Contributor

cjgillot commented Jul 5, 2022

r? rust-lang/types

@rust-highfive rust-highfive assigned lcnr and unassigned cjgillot Jul 5, 2022
@lcnr
Copy link
Contributor

lcnr commented Jul 6, 2022

don't fully understand whether this is the correct fix, not sure whether #97104 has a meaningful relation to impl trait 😅 the ub here doesn't rely on impl trait at all.

trait Foo<ARG: 'static>: 'static {
    type Assoc: AsRef<str>;
}

fn hr_shit<T: ?Sized, ARG>(x: T::Assoc) -> Box<dyn AsRef<str> + 'static>
where
    T: Foo<ARG>
{
    Box::new(x)
}

fn extend_lt<'a>(x: &'a str) -> Box<dyn AsRef<str> + 'static> {
    type DynTrait = dyn for<'a> Foo<&'a str, Assoc = &'a str>;
    hr_shit::<DynTrait, _>(x)
} 

fn main() {
    let extended = extend_lt(&String::from("hello"));
    println!("{}", extended.as_ref().as_ref());
}

I think we already have an issue for this?

edit: I think that's #44454

@bors
Copy link
Contributor

bors commented Jul 6, 2022

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

@aliemjay
Copy link
Member

aliemjay commented Jul 29, 2022

@lcnr I believe the your reduced example is a separate problem, completely orthogonal to the one in hand.

The type dyn for<'a> Callable<Opaque<&'a ()>, Output = &'a str> must to be rejected because the lifetime 'a doesn't necessarily constrain the TAIT and thus can't appear in the associated type as dictated by RFC 1214, it's exactly the same reason we reject projection types in dyn for<'a> Callable<<&'a str as IntoIterator>::Item, Output = &'a str>.

Meanwhile, the type from your example dyn for<'a> Foo<&'a str, Assoc = &'a str>, I believe, is a valid one and should not be rejected per-se, because it requires checking the WF of the TraitRef before each instantiation of 'a. If anything, we should error in hr_shit::<DynTrait, _>(x)

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

seems good, so after my nits are dealt with, r=me

src/test/ui/type-alias-impl-trait/constrain_inputs.rs Outdated Show resolved Hide resolved
|
LL | type MalformedTy = dyn for<'a> Callable<Gal<&'a ()>, Output = &'a str>;
| ^^^^^^^^^^^^^^^^

Copy link
Contributor

Choose a reason for hiding this comment

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

why is the note "lifetimes appearing in an associated type are not considered constrained" not emitted here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're not checking for higher kinded lifetimes in the diagnostic, we're just checking for names. It would be a larger refactoring to handle the difference between lifetimes on the type alias and lifetimes in a binder.

@oli-obk oli-obk force-pushed the opaque_type_late_bound_lifetimes branch from 08441cb to 64d11fc Compare September 8, 2022 13:49
@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 8, 2022

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Sep 8, 2022

📌 Commit 64d11fc has been approved by lcnr

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 Sep 8, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 8, 2022
…times, r=lcnr

Opaque types' generic params do not imply anything about their hidden type's lifetimes

fixes rust-lang#97104

cc `@aliemjay`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Sep 8, 2022
…times, r=lcnr

Opaque types' generic params do not imply anything about their hidden type's lifetimes

fixes rust-lang#97104

cc ``@aliemjay``
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 8, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#98933 (Opaque types' generic params do not imply anything about their hidden type's lifetimes)
 - rust-lang#101041 (translations(rustc_session): migrates rustc_session to use SessionDiagnostic - Pt. 2)
 - rust-lang#101424 (Adjust and slightly generalize operator error suggestion)
 - rust-lang#101496 (Allow lower_lifetime_binder receive a closure)
 - rust-lang#101501 (Allow lint passes to be bound by `TyCtxt`)
 - rust-lang#101515 (Recover from typo where == is used in place of =)
 - rust-lang#101545 (Remove unnecessary `PartialOrd` and `Ord`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d392838 into rust-lang:master Sep 8, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 8, 2022
flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 6, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#98933 (Opaque types' generic params do not imply anything about their hidden type's lifetimes)
 - rust-lang#101041 (translations(rustc_session): migrates rustc_session to use SessionDiagnostic - Pt. 2)
 - rust-lang#101424 (Adjust and slightly generalize operator error suggestion)
 - rust-lang#101496 (Allow lower_lifetime_binder receive a closure)
 - rust-lang#101501 (Allow lint passes to be bound by `TyCtxt`)
 - rust-lang#101515 (Recover from typo where == is used in place of =)
 - rust-lang#101545 (Remove unnecessary `PartialOrd` and `Ord`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@oli-obk oli-obk deleted the opaque_type_late_bound_lifetimes branch November 9, 2022 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

late-bound lifetimes on type-alias-impl-trait can be unsound
7 participants