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

Split tait and impl trait in assoc items logic #119766

Merged
merged 6 commits into from
Jan 24, 2024

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jan 9, 2024

And simplify the assoc item logic where applicable.

This separation shows that it is easier to reason about impl trait in assoc items compared with TAITs. See https://rust-lang.zulipchat.com/#narrow/stream/315482-t-compiler.2Fetc.2Fopaque-types/topic/impl.20trait.20in.20associated.20type for some discussion.

The current plan is to try to stabilize impl trait in associated items before TAIT, as they do not have any issues with their defining scopes (see #107645 for why this is not a trivial or uncontroversial topic).

@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 Jan 9, 2024
@oli-obk oli-obk added the F-impl_trait_in_assoc_type `#![feature(impl_trait_in_assoc_type)]` label Jan 9, 2024
@oli-obk oli-obk force-pushed the split_tait_and_atpit branch from 4c03162 to 9155570 Compare January 9, 2024 14:13
@compiler-errors
Copy link
Member

Add additional test:

#![feature(impl_trait_in_assoc_type)]

trait Trait {
    type Assoc;
    
    fn t() -> Struct;
}

struct Struct {
  field: <() as Trait>::Assoc,
}

impl Trait for () {
    type Assoc = impl Sized;
    
    fn t() -> Struct {
        Struct { field: () }
    }
}

ItemKind::OpaqueTy(OpaqueTy {
origin: hir::OpaqueTyOrigin::TyAlias { in_assoc_ty: true },
..
}) => opaque::find_opaque_ty_constraints_for_impl_trait_in_assoc_type(tcx, def_id),
Copy link
Member

Choose a reason for hiding this comment

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

One question -- why does this need to be separate? Shouldn't this work with the regular find_opaque_ty_constraints_for_tait, as long as typeck uses the right def-ids collected from ImplTraitInAssocTypeCollector?

Copy link
Member

Choose a reason for hiding this comment

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

Are we still checking the nested bodies of associated items? Pls check if this example (which constrains an ITIAT incorrectly inside of a closure body) still fails:

#![feature(impl_trait_in_assoc_type)]

trait Foo {
  type Assoc;
  fn bar() -> Self::Assoc;
}

impl Foo for () {
  type Assoc = impl Sized;
  fn bar() -> Self::Assoc {
    let closure = || -> Self::Assoc { let x: Self::Assoc = (); x };
    let _: i32 = closure();
    1i32
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't need to be separate per se, but the new impl is so straight forward... and if the TAIT impl changes, we don't need to worry about the ITAIT impl at all.

ItemKind::OpaqueTy(OpaqueTy {
origin: hir::OpaqueTyOrigin::TyAlias { in_assoc_ty: true },
..
}) => opaque::find_opaque_ty_constraints_for_impl_trait_in_assoc_type(tcx, def_id),
Copy link
Member

Choose a reason for hiding this comment

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

Are we still checking the nested bodies of associated items? Pls check if this example (which constrains an ITIAT incorrectly inside of a closure body) still fails:

#![feature(impl_trait_in_assoc_type)]

trait Foo {
  type Assoc;
  fn bar() -> Self::Assoc;
}

impl Foo for () {
  type Assoc = impl Sized;
  fn bar() -> Self::Assoc {
    let closure = || -> Self::Assoc { let x: Self::Assoc = (); x };
    let _: i32 = closure();
    1i32
  }
}

compiler/rustc_hir_analysis/src/collect/type_of/opaque.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 16, 2024

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.

Please also add this as a test:

#![feature(impl_trait_in_assoc_type)]

trait Foo {
    type Assoc<'a, 'b>;
    fn bar<'a: 'a, 'b: 'b>(_: &'a ()) -> Self::Assoc<'a, 'b>;
}

impl Foo for () {
    type Assoc<'a, 'b> = impl Sized;
    fn bar<'a: 'a, 'b: 'b>(x: &'a ()) -> Self::Assoc<'a, 'b> {
        let closure = |x: &'a ()| -> Self::Assoc<'b, 'a> {
            x
        };
        x
    }
}

fn main() {}

I don't know why it's failing actually, because it seems unrelated. But if it ever got fixed, then I'd want to see it not become unsound.

r=me afterwards

@compiler-errors
Copy link
Member

@rustbot author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 21, 2024
@traviscross
Copy link
Contributor

traviscross commented Jan 22, 2024

I don't know why it's failing actually, because it seems unrelated. But if it ever got fixed, then I'd want to see it not become unsound.

[Edit (see below):] Here's a different interesting issue found while trying to minimize this one.

#![feature(impl_trait_in_assoc_type)]

trait Foo {
    type Assoc<'a>;
    fn bar();
}

impl Foo for () {
    type Assoc<'a> = impl Sized;
    fn bar()
    where
        for<'a> Self::Assoc<'a>:,
    {
        let _ = |x: &()| -> Self::Assoc<'_> { x };
        //~^ ERROR hidden type for `<() as Foo>::Assoc<'_>` captures lifetime that does not appear in bounds
        //
        // If we ascribe the type to:
        //
        //     for<'b> fn(&'b ()) -> Self::Assoc<'b>
        //
        // ...then this works.
    }
}

fn main() {}

@compiler-errors
Copy link
Member

compiler-errors commented Jan 22, 2024

@traviscross: I don't believe that is a faithful reproduction of the error I mentioned. Your example has to do with the fact that you cannot have a definition of an opaque's hidden type capture any higher-ranked lifetimes.

edit: actually, I don't even think it's that. I just think that in that example, the opaque's lifetime isn't being constrained to a universal lifetime at all.

Notice that in my example that closure has a signature that only references lifetimes that are coming from the parent function.

@traviscross
Copy link
Contributor

traviscross commented Jan 22, 2024

We talked off-thread and agreed that in the example I gave it is surprising that ascribing the type there makes it work. But as @compiler-errors helpfully pointed out, it's likely a separate issue. In that light, here's a corrected faithful minimization stated in terms of ATPIT:

#![feature(impl_trait_in_assoc_type)]

trait Foo {
    type Assoc<'a>;
    fn bar<'a: 'a>();
}

impl Foo for () {
    type Assoc<'a> = impl Sized;
    fn bar<'a: 'a>()
    where
        Self::Assoc<'a>:,
    {
        let _ = |x: &'a ()| {
            let _: Self::Assoc<'a> = x;
            //~^ ERROR hidden type for `<() as Foo>::Assoc<'a>` captures lifetime that does not appear in bound
        };
    }
}

fn main() {}

Here is what I would suspect to be an RPIT minimization of the same issue:

use core::convert::identity;

fn test<'a: 'a>(n: bool) -> impl Sized + 'a {
    let true = n else { loop {} };
    let _ = || {
        let _ = identity::<&'a ()>(test(false));
        //~^ ERROR hidden type for `impl Sized + 'a` captures lifetime that does not appear in bounds
    };
    loop {}
}

fn main() {}

@oli-obk oli-obk force-pushed the split_tait_and_atpit branch from 2a7e53a to 5e5d135 Compare January 22, 2024 14:36
@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 22, 2024

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented Jan 22, 2024

📌 Commit 5e5d135 has been approved by compiler-errors

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 22, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 22, 2024
…mpiler-errors

Split tait and impl trait in assoc items logic

And simplify the assoc item logic where applicable.

This separation shows that it is easier to reason about impl trait in assoc items compared with TAITs. See https://rust-lang.zulipchat.com/#narrow/stream/315482-t-compiler.2Fetc.2Fopaque-types/topic/impl.20trait.20in.20associated.20type for some discussion.

The current plan is to try to stabilize impl trait in associated items before TAIT, as they do not have any issues with their defining scopes (see rust-lang#107645 for why this is not a trivial or uncontroversial topic).
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119433 (rc,sync: Do not create references to uninitialized values)
 - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions)
 - rust-lang#119766 (Split tait and impl trait in assoc items logic)
 - rust-lang#120160 (Manually implement derived `NonZero` traits.)
 - rust-lang#120177 (Remove duplicate dependencies for rustc)
 - rust-lang#120183 (Add `#[coverage(off)]` to closures introduced by `#[test]` and `#[bench]`)
 - rust-lang#120185 (coverage: Don't instrument `#[automatically_derived]` functions)
 - rust-lang#120201 (Bump some deps with syn 1.0 dependencies)
 - rust-lang#120246 (Re-add estebank to review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
fmease added a commit to fmease/rust that referenced this pull request Jan 23, 2024
…mpiler-errors

Split tait and impl trait in assoc items logic

And simplify the assoc item logic where applicable.

This separation shows that it is easier to reason about impl trait in assoc items compared with TAITs. See https://rust-lang.zulipchat.com/#narrow/stream/315482-t-compiler.2Fetc.2Fopaque-types/topic/impl.20trait.20in.20associated.20type for some discussion.

The current plan is to try to stabilize impl trait in associated items before TAIT, as they do not have any issues with their defining scopes (see rust-lang#107645 for why this is not a trivial or uncontroversial topic).
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 23, 2024
Rollup of 12 pull requests

Successful merges:

 - rust-lang#112806 (Small code improvements in `collect_intra_doc_links.rs`)
 - rust-lang#119460 (coverage: Never emit improperly-ordered coverage regions)
 - rust-lang#119766 (Split tait and impl trait in assoc items logic)
 - rust-lang#120062 (llvm: change data layout bug to an error and make it trigger more)
 - rust-lang#120099 (linker: Refactor library linking methods in `trait Linker`)
 - rust-lang#120139 (Do not normalize closure signature when building `FnOnce` shim)
 - rust-lang#120160 (Manually implement derived `NonZero` traits.)
 - rust-lang#120171 (Fix assume and assert in jump threading)
 - rust-lang#120183 (Add `#[coverage(off)]` to closures introduced by `#[test]` and `#[bench]`)
 - rust-lang#120195 (add several resolution test cases)
 - rust-lang#120259 (Split Diagnostics for Uncommon Codepoints: Add List to Display Characters Involved)
 - rust-lang#120261 (Provide structured suggestion to use trait objects in some cases of `if` arm type divergence)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 23, 2024
Rollup of 9 pull requests

Successful merges:

 - rust-lang#112806 (Small code improvements in `collect_intra_doc_links.rs`)
 - rust-lang#119766 (Split tait and impl trait in assoc items logic)
 - rust-lang#120139 (Do not normalize closure signature when building `FnOnce` shim)
 - rust-lang#120160 (Manually implement derived `NonZero` traits.)
 - rust-lang#120171 (Fix assume and assert in jump threading)
 - rust-lang#120183 (Add `#[coverage(off)]` to closures introduced by `#[test]` and `#[bench]`)
 - rust-lang#120195 (add several resolution test cases)
 - rust-lang#120259 (Split Diagnostics for Uncommon Codepoints: Add List to Display Characters Involved)
 - rust-lang#120261 (Provide structured suggestion to use trait objects in some cases of `if` arm type divergence)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5da220a into rust-lang:master Jan 24, 2024
11 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 24, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 24, 2024
Rollup merge of rust-lang#119766 - oli-obk:split_tait_and_atpit, r=compiler-errors

Split tait and impl trait in assoc items logic

And simplify the assoc item logic where applicable.

This separation shows that it is easier to reason about impl trait in assoc items compared with TAITs. See https://rust-lang.zulipchat.com/#narrow/stream/315482-t-compiler.2Fetc.2Fopaque-types/topic/impl.20trait.20in.20associated.20type for some discussion.

The current plan is to try to stabilize impl trait in associated items before TAIT, as they do not have any issues with their defining scopes (see rust-lang#107645 for why this is not a trivial or uncontroversial topic).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-impl_trait_in_assoc_type `#![feature(impl_trait_in_assoc_type)]` 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.

5 participants