Skip to content

Conversation

cjgillot
Copy link
Contributor

Blocked on #103448
Specific diff https://github.com/cjgillot/rust/compare/no-late-fn..variance-rpit

This PR attempts to stop duplicating generic parameters between functions and opaque types.

fn foo<'a, 'b, T>() -> impl Sized + 'a {}

// OLD
fn foo<'a, 'b, T>() -> foo::<'static, 'static, T>::opaque::<'a> {}
opaque foo::<T>::opaque<'a>: Sized + 'a;

// OLD post #103448
fn foo<'a, 'b, T>() -> foo::<'static, 'static, T>::opaque::<'a> {}
opaque foo::<'_a, '_b, T>::opaque<'a>: Sized + 'a;

// NEW
fn foo<'a, 'b, T>() -> foo::<'a, 'b, T>::opaque {}
opaque foo::<'a, 'b, T>::opaque: Sized + 'a;

This allows to support Self in impl-trait, since we do not replace lifetimes by 'static any more.

The implementation relies on 2 tweaking rules for opaques in 2 places:

  • we only relate substs that correspond to captured lifetimes during TypeRelation;
  • we only list captured lifetimes in choice region computation.

For simplicity, I encoded the "capturedness" of lifetimes as a variance, Bivariant vs Invariant for unused vs captured lifetimes. The variances_of query used to ICE for opaques.

Writing the PR description made me realize that this strategy can be used without #103448 to support Self in RPIT. I'll probably make such a PR during the week.

r? types

@cjgillot cjgillot added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Oct 23, 2022
@rustbot rustbot added 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 Oct 23, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 23, 2022

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

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

compiler-errors commented Oct 23, 2022

This allows to support Self in impl-trait, since we do not replace lifetimes by 'static any more.

Can you elaborate on this? I don't have context of what impl Trait currently has issues with.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Oct 24, 2022

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

@rustbot
Copy link
Collaborator

rustbot commented Oct 26, 2022

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
Attempting with retry: make prepare
---
* highest error code: E0790
Found 506 error codes
Found 0 error(s) in error codes
Done!
tidy error: /checkout/compiler/rustc_ast_lowering/src/lib.rs:1467: unexplained "```ignore" doctest; try one:

* make the test actually pass, by adding necessary imports and declarations, or
* use "```text", if the code is not Rust code, or
* use "```compile_fail,Ennnn", if the code is expected to fail at compile time, or
* use "```should_panic", if the code is expected to fail at run time, or
* use "```no_run", if the code should type-check but not necessary linkable/runnable, or
* explain it like "```ignore (cannot-test-this-because-xxxx)", if the annotation cannot be avoided.


tidy error: /checkout/compiler/rustc_ast_lowering/src/lib.rs:1474: unexplained "```ignore" doctest; try one:

* make the test actually pass, by adding necessary imports and declarations, or
* use "```text", if the code is not Rust code, or
* use "```compile_fail,Ennnn", if the code is expected to fail at compile time, or
* use "```should_panic", if the code is expected to fail at run time, or
* use "```no_run", if the code should type-check but not necessary linkable/runnable, or
* explain it like "```ignore (cannot-test-this-because-xxxx)", if the annotation cannot be avoided.

* 387 features
some tidy checks failed
Build completed unsuccessfully in 0:00:11

@bors
Copy link
Collaborator

bors commented Oct 27, 2022

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

@compiler-errors compiler-errors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 21, 2022
Support using `Self` or projections inside an RPIT/async fn

I reuse the same idea as rust-lang#103449 to use variances to encode whether a lifetime parameter is captured by impl-trait.

The current implementation of async and RPIT replace all lifetimes from the parent generics by `'static`.  This PR changes the scheme
```rust
impl<'a> Foo<'a> {
    fn foo<'b, T>() -> impl Into<Self> + 'b { ... }
}

opaque Foo::<'_a>::foo::<'_b, T>::opaque<'b>: Into<Foo<'_a>> + 'b;
impl<'a> Foo<'a> {
    // OLD
    fn foo<'b, T>() -> Foo::<'static>::foo::<'static, T>::opaque::<'b> { ... }
                             ^^^^^^^ the `Self` becomes `Foo<'static>`

    // NEW
    fn foo<'b, T>() -> Foo::<'a>::foo::<'b, T>::opaque::<'b> { ... }
                             ^^ the `Self` stays `Foo<'a>`
}
```

There is the same issue with projections. In the example, substitute `Self` by `<T as Trait<'b>>::Assoc` in the sugared version, and `Foo<'_a>` by `<T as Trait<'_b>>::Assoc` in the desugared one.

This allows to support `Self` in impl-trait, since we do not replace lifetimes by `'static` any more.  The same trick allows to use projections like `T::Assoc` where `Self` is allowed.  The feature is gated behind a `impl_trait_projections` feature gate.

The implementation relies on 2 tweaking rules for opaques in 2 places:
- we only relate substs that correspond to captured lifetimes during TypeRelation;
- we only list captured lifetimes in choice region computation.

For simplicity, I encoded the "capturedness" of lifetimes as a variance, `Bivariant` vs `Invariant` for unused vs captured lifetimes. The `variances_of` query used to ICE for opaques.

Impl-trait that do not reference `Self` or projections will have their variances as:
- `o` (invariant) for each parent type or const;
- `*` (bivariant) for each parent lifetime --> will not participate in borrowck;
- `o` (invariant) for each own lifetime.

Impl-trait that does reference `Self` and/or projections will have some parent lifetimes marked as `o` (as the example above), and participate in type relation and borrowck.  In the example above, `variances_of(opaque) = ['_a: o, '_b: *, T: o, 'b: o]`.

r? types
cc `@compiler-errors` , as you asked about the issue with `Self` and projections.
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Nov 27, 2022
Support using `Self` or projections inside an RPIT/async fn

I reuse the same idea as rust-lang/rust#103449 to use variances to encode whether a lifetime parameter is captured by impl-trait.

The current implementation of async and RPIT replace all lifetimes from the parent generics by `'static`.  This PR changes the scheme
```rust
impl<'a> Foo<'a> {
    fn foo<'b, T>() -> impl Into<Self> + 'b { ... }
}

opaque Foo::<'_a>::foo::<'_b, T>::opaque<'b>: Into<Foo<'_a>> + 'b;
impl<'a> Foo<'a> {
    // OLD
    fn foo<'b, T>() -> Foo::<'static>::foo::<'static, T>::opaque::<'b> { ... }
                             ^^^^^^^ the `Self` becomes `Foo<'static>`

    // NEW
    fn foo<'b, T>() -> Foo::<'a>::foo::<'b, T>::opaque::<'b> { ... }
                             ^^ the `Self` stays `Foo<'a>`
}
```

There is the same issue with projections. In the example, substitute `Self` by `<T as Trait<'b>>::Assoc` in the sugared version, and `Foo<'_a>` by `<T as Trait<'_b>>::Assoc` in the desugared one.

This allows to support `Self` in impl-trait, since we do not replace lifetimes by `'static` any more.  The same trick allows to use projections like `T::Assoc` where `Self` is allowed.  The feature is gated behind a `impl_trait_projections` feature gate.

The implementation relies on 2 tweaking rules for opaques in 2 places:
- we only relate substs that correspond to captured lifetimes during TypeRelation;
- we only list captured lifetimes in choice region computation.

For simplicity, I encoded the "capturedness" of lifetimes as a variance, `Bivariant` vs `Invariant` for unused vs captured lifetimes. The `variances_of` query used to ICE for opaques.

Impl-trait that do not reference `Self` or projections will have their variances as:
- `o` (invariant) for each parent type or const;
- `*` (bivariant) for each parent lifetime --> will not participate in borrowck;
- `o` (invariant) for each own lifetime.

Impl-trait that does reference `Self` and/or projections will have some parent lifetimes marked as `o` (as the example above), and participate in type relation and borrowck.  In the example above, `variances_of(opaque) = ['_a: o, '_b: *, T: o, 'b: o]`.

r? types
cc `@compiler-errors` , as you asked about the issue with `Self` and projections.
@cjgillot cjgillot closed this Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants