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

fix type annotations to not require Self: 'this #4

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

aliemjay
Copy link
Contributor

Hi @WanderLanz,

Just a heads-up, lender will stop compiling after a recent soundness fix to the rust compiler in rust-lang/rust#104098. This was found by this crater run. The error message can be found here and you can try to build lender with that change by installing the toolchain first:

$ rustup-toolchain-install-master 6c5ba887f1fae50b77a36ca00e3d7164c1434657
$ cargo +6c5ba887f1fae50b77a36ca00e3d7164c1434657 build

A minimal example of what has changed in that PR is that the rust compiler used to incorrectly accept the following code:

trait Lending<'lend, _Witness = &'lend Self> {
    type Lend: 'lend;
}

struct Adapter<G>(G); // e.g. IntersperseWith

impl<'this, G> Lending<'this> for Adapter<G> {
    type Lend = (); // Lend does not depend on G
}

fn next<'this, G>() {
    let _: <Adapter<G> as Lending<'this>>::Lend;
    //~^ ERROR `G` may not live long enough
    // The full type is `<Adapter<G> as Lending<'this, &'this Adapter<G>>>::Lend`
    // The default type `&'this Adapter<G>` should require `T: 'this`
}

This can be fixed by adding a where clause G: 'this as suggested by the error message. Alternatively we can change the type annotation <Adapter<G> as Lending<'this>>::Lend to not require additional lifetime constraints. This is what I've done in this PR because it has the advantage of not breaking the public interface (although the other approach is only a theoretical breaking change. I would be surprised if it breaks real dependent code).

@vigna
Copy link
Collaborator

vigna commented Nov 15, 2023

I'm really confused. The commit seem to have nothing to do with the post. Am I missing something?

@@ -44,7 +44,7 @@ where
#[inline]
fn next(&mut self) -> Option<<Self as Lending<'_>>::Lend> {
// SAFETY: 'a: 'lend
Some(unsafe { core::mem::transmute::<<Self as Lending<'a>>::Lend, <Self as Lending<'_>>::Lend>((self.f)()) })
Some(unsafe { core::mem::transmute::<<L as Lending<'a>>::Lend, <L as Lending<'_>>::Lend>((self.f)()) })
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'm really confused. The commit seem to have nothing to do with the post. Am I missing something?

@vigna Before this change, the full annotated type here was <Self as Lending<'a, &'a Self>>::Lend, which requires &'a Self to be a "well-formed" reference type, which requires Self: 'a => RepeatWith<'a, L, F>: 'a => (F: 'a and L: 'a).
L: 'a is satified by the where clause, but F: 'a is not hence the error.

After this change, the full type is <L as Lending<'a, &'a L>>::Lend. This only requires L: 'a which is already satisfied by the bounds.

@WanderLanz
Copy link
Owner

Looks good, thank you for the heads up on future WF changes.

I'm not expecting vigna to have upstream tools to check, but if you do, please feel free to merge. Otherwise, I will merge/fix once I'm able to.

@vigna
Copy link
Collaborator

vigna commented Nov 16, 2023

I tried to run crater on AWS, but documentation is 4yo and the build process dies on a too old version of CMake. I think it is something fairly easy to fix for someone using the toolchain normally, but after a couple of hours of trial and error I stopped.

I mean, I can check that this compiles now if that's sufficient for merge.

@aliemjay
Copy link
Contributor Author

I'm not sure why you want to run crater yourself. The change I made here is definitely not a breaking change to downstream users and, if you want to test how lender builds against the proposed rustc change, you only need to use rustup-toolchain-install-master locally as described above.

@vigna
Copy link
Collaborator

vigna commented Nov 16, 2023

Ah ok. I tried to read online how to get that command and I arrived to crater. I'll check again.

@vigna vigna merged commit c459fe9 into WanderLanz:main Nov 16, 2023
@vigna
Copy link
Collaborator

vigna commented Nov 16, 2023

Ok. Command installed. I verified that the new nightly will stop compiling Lender. The proposed PR fixes this problem. All test pass.

@aliemjay aliemjay deleted the fix-annotations branch November 17, 2023 02:59
@WanderLanz
Copy link
Owner

Awesome, thanks for the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants