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

Regression from stable to beta with HRTB supertraits #91899

Closed
Manishearth opened this issue Dec 14, 2021 · 9 comments · Fixed by #91915
Closed

Regression from stable to beta with HRTB supertraits #91899

Manishearth opened this issue Dec 14, 2021 · 9 comments · Fixed by #91915
Labels
C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. regression-untriaged Untriaged performance or correctness regression.
Milestone

Comments

@Manishearth
Copy link
Member

Manishearth commented Dec 14, 2021

Note: The regression is valid/"allowed" (there's a hole in the typesystem being patched), I'm just filing this issue now so we can make sure that this regression is tracked in case it got "fixed" by accident. Feel free to close if it's a known quantity; I'm just worried that this got "accidentally" fixed in some other change.

My crate is being fixed to not face this problem, but in general there's a chance other crates are falling afoul of this as well; we should perhaps assess the impact and start off with it being a future incompatibility warning if necessary.

Code

I tried this code:

pub trait Yokeable<'a>: 'static {
    type Output: 'a;
}


impl<'a, T: 'static + ?Sized> Yokeable<'a> for &'static T {
    type Output = &'a T;
}


pub trait ZeroCopyFrom<C: ?Sized>: for<'a> Yokeable<'a> {
    /// Clone the cart `C` into a [`Yokeable`] struct, which may retain references into `C`.
    fn zero_copy_from<'b>(cart: &'b C) -> <Self as Yokeable<'b>>::Output;
}

impl<T> ZeroCopyFrom<[T]> for &'static [T] {
    fn zero_copy_from<'b>(cart: &'b [T]) -> &'b [T] {
        cart
    }
}

(playpen)

I expected to see this happen: It compiles, like it does on stable, or at least throws a future incompatibility warning.

Instead, this happened: It errors:

error[E0310]: the parameter type `T` may not live long enough
  --> src/lib.rs:17:5
   |
17 |     fn zero_copy_from<'b>(cart: &'b [T]) -> &'b [T] {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: consider adding an explicit lifetime bound `T: 'static`...
   = note: ...so that the type `[T]` will meet its required lifetime bounds

For more information about this error, try `rustc --explain E0310`.
error: could not compile `playground` due to previous error

Version it worked on

It most recently worked on: Rust 1.56.0

Version with regression

1.58.0-beta.2

(2021-12-04 0e07bcb68b82b54c0c4e)
@Manishearth Manishearth added regression-from-stable-to-beta Performance or correctness regression from stable to beta. C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Dec 14, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 14, 2021
@jyn514 jyn514 added the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Dec 14, 2021
@inquisitivecrystal
Copy link
Contributor

inquisitivecrystal commented Dec 14, 2021

This regressed in #91388, which was a rollup. Further testing shows this resulted from #91243. It's marked as being a partial revert, so I'm not entirely sure what the chronology is here. CC @jackh726. Note that we think this "regression" probably isn't actually a problem, per the note at the top.

@inquisitivecrystal inquisitivecrystal removed the E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc label Dec 14, 2021
@Mark-Simulacrum Mark-Simulacrum added this to the 1.58.0 milestone Dec 14, 2021
@jackh726
Copy link
Member

What the....

@jackh726
Copy link
Member

Okay so reverting #88312 fully also errors. Stable 1.55.0 fails. Checking 0599f34 (a couple commits before #88312) now, but I'm assuming it's going to error.

So, this was actually a bug in 1.56.0 - prior to that, this didn't compile, rightfully.

My guess is because of treating unnormalized function args & output as WF, we saw <Self as Yokeable<'b>>::Output, replaced it with <&'static [T] as Yokeable<'b>>::Output, assumed that was WF, so we didn't actually check that T: 'static.

I'm going to go ahead and make a PR to add a test that closes this issue. This was a bug in 1.56.0, not a regression since.

@Manishearth
Copy link
Member Author

Good to know, thanks!

(And good that this was just a one-version regression; less chance of there being in-the-wild breakage)

@Manishearth
Copy link
Member Author

🤔🤔 I should figure out an excuse to make yoke a dependency of rustc, it seems really good at finding typesystem corner cases and maybe we should just have it be a test 😁

@sffc
Copy link

sffc commented Dec 14, 2021

Isn't it failing on the wrong line? I would have expected it to fail on the

impl<T> ZeroCopyFrom<[T]> for &'static [T]

because &'static [T] is the thing that needs to match for<'a> Yokeable<'a>: 'static

@jackh726
Copy link
Member

🤔🤔 I should figure out an excuse to make yoke a dependency of rustc, it seems really good at finding typesystem corner cases and maybe we should just have it be a test 😁

Haha yeah, it's a thorn in my side 😛

Isn't it failing on the wrong line? I would have expected it to fail on the ... because &'static [T] is the thing that needs to match for<'a> Yokeable<'a>: 'static

So, it's interesting. The check is (&'static [T]): 'static, which is true. What I might expect here is it to fail because you haven't written T: 'static (which is required for &'static [T] to be well-formed. But I think this is implied bounds, since this is fine:

pub trait ZeroCopyFrom {}
impl<T> ZeroCopyFrom for &'static T {}

@Manishearth
Copy link
Member Author

In my experience WFness of &'a T has always been a bit variable where sometimes you can assume it and sometimes it requires you to spell it out, so maybe this is another instance of that.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 14, 2021
…crum

Add another regression test for unnormalized fn args with Self

Closes rust-lang#91899
@bors bors closed this as completed in 05b65ad Dec 15, 2021
@rustbot rustbot removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Dec 15, 2021
@Manishearth
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. regression-untriaged Untriaged performance or correctness regression.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants