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

Require impl Trait in associated types to appear in method signatures #110454

Merged
merged 6 commits into from
May 13, 2023

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Apr 17, 2023

This implements the limited version of TAIT that was proposed in #107645 (comment)

Similar to impl Trait in return types, impl Trait in associated types may only be used within the impl block which it is a part of. To make everything simpler and forward compatible to getting desugared to a plain type alias impl trait in the future, we're requiring that any associated functions or constants that want to register hidden types must be using the associated type in their signature (type of the constant or argument/return type of the associated method. Where bounds mentioning the associated type are ignored).

We have preexisting tests checking that this works transitively across multiple associated types in situations like

impl Foo for Bar {
    type A = impl Trait;
    type B = impl Iterator<Item = Self::A>;
    fn foo() -> Self::B { ...... }
}

@rustbot
Copy link
Collaborator

rustbot commented Apr 17, 2023

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. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Apr 17, 2023
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.

Sorry for the delay. Here's some initial implementation thoughts.

compiler/rustc_hir_analysis/src/check/check.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/opaque_types.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/opaque_types.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/opaque_types.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/opaque_types.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/opaque_types.rs Outdated Show resolved Hide resolved
@compiler-errors compiler-errors 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 Apr 19, 2023
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.

Do we have a test for this case?:

trait Trait {
    type Item;
    type Iter: IntoIterator<Item = Self::Item>;
    fn iter() -> Self::Iter;
}
impl Trait for () {
    type Item = impl Sized;
    type Iter = impl IntoIterator<Item = Self::Item>;
    fn iter() -> Self::Iter { None::<()> }
}

I think it's pretty reasonable to allow fn iter to constrain impl Sized, but we should probably have an explicit t-lang decision here?

I also suggest adding a couple tests:

// check-fail
// revisions: compare_ty compare_method
#![feature(impl_trait_in_assoc_type)]

#[cfg(compare_ty)]
mod compare_ty {
    trait Trait {
        type Ty: IntoIterator<Item = ()>;
    }
    impl Trait for () {
        type Ty = Option<impl Sized>;
    }
}

#[cfg(compare_method)]
mod compare_method {
    trait Trait {
        type Ty;
        fn method() -> Self::Ty;
    }
    impl Trait for () {
        type Ty = impl Sized;
        fn method() -> () {}
    }
}

) -> Option<impl TypeVisitable<TyCtxt<'tcx>>> {
let sig = match tcx.def_kind(def_id) {
DefKind::AssocFn => Ok(tcx.fn_sig(def_id).skip_binder()),
DefKind::AssocConst | DefKind::AssocTy => Err(tcx.type_of(def_id).skip_binder()),
Copy link
Member

Choose a reason for hiding this comment

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

why DefKid::AssocTy?

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 added a comment for that. The reason for that is that we do wf checks of opaque types within their parent. So we need to treat their parent as having them in the signature, as we'd otherwise fail that wf check. The wfcheck is fishy anyway and we're working on removing it, so that will go away once we remove the wfcheck

compiler/rustc_infer/src/infer/opaque_types.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 25, 2023

I still need to look into @aliemjay's review, but I was able to get rid of many of the problems we had with the approach by replacing it entirely. It now mirrors ImplTraitInTraitFinder and I plan to see if there is some deduplication that can be done between the two.

@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk force-pushed the limited_impl_trait_in_assoc_type branch from 5de24be to 97b3038 Compare April 25, 2023 13:40
@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 25, 2023

I think it's pretty reasonable to allow fn iter to constrain impl Sized, but we should probably have an explicit t-lang decision here?

Yes, we do have https://github.com/rust-lang/rust/blob/master/tests/ui/generic-associated-types/issue-89008.rs

I don't think this feature makes sense without automatically recursing into other associated types from the same impl block.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 4, 2023

@rustbot ready

@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 May 4, 2023
@bors
Copy link
Contributor

bors commented May 6, 2023

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

compiler/rustc_ty_utils/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_middle/src/query/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/opaque_types.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/opaque_types.rs Outdated Show resolved Hide resolved
compiler/rustc_ty_utils/src/opaque_types.rs Outdated Show resolved Hide resolved
compiler/rustc_ty_utils/src/opaque_types.rs Outdated Show resolved Hide resolved
@oli-obk oli-obk force-pushed the limited_impl_trait_in_assoc_type branch from 30120bb to f7b5568 Compare May 8, 2023 13:37
@oli-obk
Copy link
Contributor Author

oli-obk commented May 8, 2023

@rustbot ready

@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels May 11, 2023
@rustbot

This comment was marked as spam.

@oli-obk oli-obk force-pushed the limited_impl_trait_in_assoc_type branch from c82a7a1 to 5b9ad1e Compare May 11, 2023 15:50
@oli-obk
Copy link
Contributor Author

oli-obk commented May 11, 2023

@rustbot ready

sorry everyone, no clue what happened here

@BoxyUwU
Copy link
Member

BoxyUwU commented May 11, 2023

It's okay its your first time making a PR here, it happens :)

@oli-obk
Copy link
Contributor Author

oli-obk commented May 12, 2023

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented May 12, 2023

📌 Commit 5cd56f11622c1311c3b54d734d5089e3200b3f7c 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 May 12, 2023
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented May 12, 2023

@bors r-

@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 May 12, 2023
@oli-obk oli-obk force-pushed the limited_impl_trait_in_assoc_type branch from 5cd56f1 to 4e92f76 Compare May 12, 2023 10:28
@oli-obk
Copy link
Contributor Author

oli-obk commented May 12, 2023

@bors r=compiler-errors

@bors
Copy link
Contributor

bors commented May 12, 2023

📌 Commit 4e92f76 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 May 12, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 13, 2023
Rollup of 6 pull requests

Successful merges:

 - rust-lang#110454 (Require impl Trait in associated types to appear in method signatures)
 - rust-lang#111096 (Add support for `cfg(overflow_checks)`)
 - rust-lang#111451 (Note user-facing types of coercion failure)
 - rust-lang#111469 (Fix data race in llvm source code coverage)
 - rust-lang#111494 (Encode `VariantIdx` so we can decode ADT variants in the right order)
 - rust-lang#111499 (asm: loongarch64: Drop efiapi)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 6cb1358 into rust-lang:master May 13, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 13, 2023
@oli-obk oli-obk deleted the limited_impl_trait_in_assoc_type branch May 13, 2023 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants