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

nested RPIT and HRTB: unclear semantics and future incompatibility #96194

Closed
aliemjay opened this issue Apr 18, 2022 · 15 comments
Closed

nested RPIT and HRTB: unclear semantics and future incompatibility #96194

aliemjay opened this issue Apr 18, 2022 · 15 comments
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@aliemjay
Copy link
Member

aliemjay commented Apr 18, 2022

(Original post in internals forum)

Given the following code (stable sin 1.30!):

fn f() -> impl for<'a> Tr<'a, Assoc = impl Copy> {}

It is undocumented whether the inner existential type, impl Copy, is allowed to access 'a or not. In other words, it is equivalent to which of the following?

type Assoc = impl Copy;
fn f() -> impl for<'a> Tr<'a, Assoc = Assoc> {}

or,

type Assoc<'a> = impl Copy;
fn f() -> impl for<'a> Tr<'a, Assoc = Assoc<'a>> {}

On stable

The behavior in current stable is consistent with the first as shown by:

  • If Assoc uses the lifetime 'a, the compiler complains about Implementation is not general enough as expected.
  • The following test on the return type passes
fn assert_unique_assoc<Assoc>(_: impl for<'a> Tr<'a, Assoc = Assoc>) {}
fn main() { assert_unique_assoc(f()); }

It looks like this was unintentionally stabilized and changing that now would break the above test (unless we leak the fact that impl Copy captures 'a ??).

On beta

#94081 makes #88236 pass but that seems to be inconsistent with the behavior in stable:

  • If Assoc uses 'a, it gives ICE, see yet another ICE with HRTB and RPIT #95647.
  • Using the above test on the return type fails, which is quite surprising since removing the lifetime from impl Copy + 'a makes the test pass. playground. And I believe adding a lifetime bound shouldn't change the semantics in this regard.
  • It would be a backward compatibility risk if we ever want to reject the code in the future.

Meta

stable version: 1.60.0
beta version: 1.61.0-beta.3 2022-04-17 2431a974c2dcf82ba513)

@rustbot label +T-Lang +A-impl-Trait

@rustbot rustbot added T-lang Relevant to the language team, which will review and decide on the PR/issue. A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. labels Apr 18, 2022
@oli-obk oli-obk added the I-lang-nominated Nominated for discussion during a lang team meeting. label Apr 19, 2022
@aliemjay
Copy link
Member Author

aliemjay commented Apr 20, 2022

Just now found out that nested impl Trait in argument position are allowed 🙈

fn test_argument_position(x: impl for<'a> Tr<'a, Assoc = impl Copy>)  {}

and it actually desugars to

T: for<'a> Tr<'a, Assoc = Assoc>,
Assoc: Copy,

instead of:

for<'a> T: Tr<'a>, 
for<'a> <T as Tr<'a>>::Assoc: Copy,

Now I believe this is a strong argument to reject impl for<'a> Tr<'a, Assoc = impl Copy + 'a> in return position!

@aliemjay
Copy link
Member Author

aliemjay commented May 10, 2022

It would be unfortunate if it hits stable with such ambiguous semantics before the language team decides on the issue.

Compiler team might want to intervene in this case.
@rustbot label T-compiler I-prioritize

@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 10, 2022
@inquisitivecrystal
Copy link
Contributor

inquisitivecrystal commented May 10, 2022

@oli-obk: I just wanted to check, was this intended to be stabilized?

Edit: sorry, I didn't see the internals link! That makes it clear that it was unintentional.

@Mark-Simulacrum
Copy link
Member

cc @nikomatsakis / @jackh726 for lack of better contacts in still-forming traits team, this is potentially another issue that is good to urgently-ish take a look at as an accidental stabilization.

@apiraino apiraino added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label May 11, 2022
@apiraino
Copy link
Contributor

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 11, 2022
@apiraino apiraino removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label May 11, 2022
@oli-obk
Copy link
Contributor

oli-obk commented May 12, 2022

Summarizing the status:

Given

trait Tr<'a> {
	type Assoc;
}

And the following impl

impl<'a> Tr<'a> for () {
	type Assoc = ();
}

Then f stack overflows on stable and is accepted on beta

fn f() -> impl for<'a> Tr<'a, Assoc = impl Copy + 'a> {}

while f1 stack works on stable and on beta

fn f1() -> impl for<'a> Tr<'a, Assoc = impl Copy> {}

but with (using the lifetime in the assoc type)

impl<'a> Tr<'a> for i32 {
	type Assoc = &'a ();
}

then f2 stack overflows on stable and ICEs on beta

fn f2() -> impl for<'a> Tr<'a, Assoc = impl Copy + 'a> { 42_i32 }

then f3 stack errors on stable and ICEs on beta

fn f3() -> impl for<'a> Tr<'a, Assoc = impl Copy> { 42_i32 }

In table form

stable beta
fn f() -> impl for<'a> Tr<'a, Assoc = impl Copy + 'a> {} stack overflow
fn f1() -> impl for<'a> Tr<'a, Assoc = impl Copy> {}
fn f2() -> impl for<'a> Tr<'a, Assoc = impl Copy + 'a> { 42_i32 } stack overflow 🧊
fn f3() -> impl for<'a> Tr<'a, Assoc = impl Copy> { 42_i32 } error 🧊

@oli-obk
Copy link
Contributor

oli-obk commented May 12, 2022

Playground with all the code

It looks like this was unintentionally stabilized and changing that now would break the above test (unless we leak the fact that impl Copy captures 'a ??).

fn assert_unique_assoc<Assoc>(_: impl for<'a> Tr<'a, Assoc = Assoc>) {}
fn main() { assert_unique_assoc(f()); }

Works on stable and beta for

fn f1() -> impl for<'a> Tr<'a, Assoc = impl Copy> {}

and stack overflows on stable while erroring on beta for

fn f() -> impl for<'a> Tr<'a, Assoc = impl Copy + 'a> {}

So... afaict the only thing we stabilized is the fact that we allow

fn f() -> impl for<'a> Tr<'a, Assoc = impl Copy + 'a> {}

to compile, without actually giving it any meaning beyond making it a breaking change to add + 'a to the impl Copy (without that giving the user any new code that will compile).

We could outright reject lifetime bounds on nested return-position-impl-trait, which seems like a safe thing to do until we know what we actually want all this to mean, but I believe the current semantics to make sense considering that we always need return-position-impl-trait to opt-in to lifetimes it wants to be able to bind.

@oli-obk
Copy link
Contributor

oli-obk commented May 12, 2022

fn f() -> impl for<'a> Tr<'a, Assoc = impl Copy> {}
It is undocumented whether the inner existential type, impl Copy, is allowed to access 'a or not. In other words, it is equivalent to which of the following?

type Assoc = impl Copy;
fn f() -> impl for<'a> Tr<'a, Assoc = Assoc> {}

or,

type Assoc<'a> = impl Copy;
fn f() -> impl for<'a> Tr<'a, Assoc = Assoc<'a>> {}

It must be the first one. Only if you add + 'a to the nested Assoc = impl Copy will you get the second one. This is consistent to regular lifetimes:

fn f<'a>() -> impl Copy {}

desugars to

type F = impl Copy;
fn f<'a>() -> F {}

while

fn f<'a>() -> impl Copy + 'a {}

desugars to

type F<'a> = impl Copy;
fn f<'a>() -> F<'a> {}

@oli-obk
Copy link
Contributor

oli-obk commented May 12, 2022

cc @rust-lang/lang

given my three comments above, I believe we have not stabilized anything problematic (code that caused a stack overflow is now accepted, without allowing anything interesting). We could still reject the stabilization with a targeted HIR level check, I am currently investigating the beta-backportability of such a change, but I would prefer to just let this ride the trains

@jackh726
Copy link
Member

I think I would strongly prefer a targeted error for lifetime bounds on nested RPIT. We can't know for sure that this doesn't allow more code to compile as-is (it doesn't for the given example, but who knows). I wouldn't want to allow writing a function with them only to later potentially error (or worse, decide we need to ignore them).

That being said, given that it seems like it doesn't fully allow new programs to compile, I don't think this would be the end of the world if this rode the trains. (But this would almost certainly would fall under accidental stabilization until at least semantics are settled)

@oli-obk
Copy link
Contributor

oli-obk commented May 12, 2022

A PR is up at #96970

bors added a commit to rust-lang-ci/rust that referenced this issue May 14, 2022
Forbid nested opaque types to reference HRTB from opaque types.

Avoids rust-lang#96194
Alternative to rust-lang#96970

r? `@oli-obk`
@joshtriplett joshtriplett removed the P-critical Critical priority label May 17, 2022
@joshtriplett joshtriplett added the P-high High priority label May 17, 2022
@Mark-Simulacrum Mark-Simulacrum removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jun 14, 2022
@Mark-Simulacrum
Copy link
Member

Dropping the lang nomination. It doesn't seem clear that there is an active need for lang feedback on this issue, or at least the question is unclear -- @oli-obk (as the nominator), if there is something pending here that needs lang action, can you provide a specific question? In general having short and succinct questions accompanying nominations rather than just nominating an issue expedites in-meeting review.

@pnkfelix
Copy link
Member

Discussed in P-high review meeting.

This is no longer a stability hazard, because we removed support for the questionable syntax .

@rustbot label -P-high

@rustbot rustbot removed the P-high High priority label Nov 11, 2022
@pnkfelix pnkfelix reopened this Nov 11, 2022
@pnkfelix
Copy link
Member

(Whoops, I didn't meant to close this; I'll let @oli-obk do that if they end up opening a separate tracking issue)

@oli-obk
Copy link
Contributor

oli-obk commented Nov 11, 2022

Closing in favour of #104288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants