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 implied_bounds_entailment future compat lint #14

Merged

Conversation

lcnr
Copy link

@lcnr lcnr commented Jan 12, 2023

Hey 👋 the current API of this crate sadly relies on an unsound compiler bug. For now, this bug has been fixed as a lint (rust-lang/rust#105572) which we will soon change to a hard error. While the other crates found via crater all had pretty simple and self-contained fixes, spair unfortunately depends on this bug in a quite intrusive way. This is not your fault and was caused by a high priority bug in rustc.

This PR implements probably the best way to solve this issue. What follows is a summary of what's going on together with an explanation of the approach taken in this PR. Feel free to reach out to me by either simply replying on this PR, via email, or using the official Rust zulip chat.

A huge thanks to @BoxyUwU who actually implemented this PR.

What is the issue

You pretty much have a bunch of the following setup:

struct XUpdater<'s> {
    fields: &'s String, // dummy field
}

trait XUpdaterMut {
    fn updater_mut<'a>(&'a mut self) -> &'a mut XUpdater<'a>;
}

impl<'s> XUpdaterMut for XUpdater<'s> {
    fn updater_mut<'a>(&'a mut self) -> &'s mut XUpdater<'a> {
        self
    }
}

And this definitely makes sense as an API, there's just one small issue: you accidentally got the compiler to incorrectly accept unsound code without any unsafe. This sucks and is getting fixed in the compiler.

Why is this unsound

When checking the body of function we are assuming that its arguments and return type are well-formed, i.e. they are "sensible". This is correct because the caller has to prove that they are.

This is used in the implementation of updater_mut. To return a mutable reference to self you must not change lifetimes in self1, as it could otherwise get replaced with a new object with these different lifetimes. We do however change the lifetime of XUpdater from 's to 'a so that should fail.

However, the type of &'a mut self is &'a mut XUpdater<'s>, so we can assume that 's: 'a holds and the return type is &'s mut XUpdater<'a> so we can assume that 'a: 's holds. This means that 's and 'a are simply equal here.

While this may be surprising, it is completely sound. The issue is that we do not have the requirement 'a: 's in the trait as there is no way to even name 's at that point. When using functions, we check the signature of the trait, not the signature of the method, so at no point do we check 'a: 's when using <XUpdater<'s> as XUpdaterMut>::updater_mut.

For a showcase of how this is unsound, see this playground example.

How to fix this issue

You cannot write these impls without the compiler bug. So we have to change the trait definition to properly propagate the right bound to the caller of this method.

trait XUpdaterMut {
    // How can we name `'s` here, it's a lifetime we only have as
    // a part of the self type?
    fn updater_mut<'a>(&'a mut self) -> &'a mut XUpdater<'s>;
}

While there are some other potential approaches to deal with this, the only sensible one we could come up with for now is to name 's as an argument of the trait which is what we've done in this PR.

trait XUpdaterMut<'s> {
    fn updater_mut<'a>(&'a mut self) -> &'a mut XUpdater<'s>;
}

This does infect quite a lot of code and also changes your public API. While unfortunate, I expect this to be unavoidable.

Footnotes

  1. i.e. the lifetime of the pointee of mutable references is invariant. See the nomicon for more details

@aclueless
Copy link
Owner

Thank you very much to both of you @lcnr and @BoxyUwU. If I have to implement the fix myself, I would probably need days to figure out how things should be. Thanks again!

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