Fix soundness issues with covariant `Q` argument

[steffahn]

Closes #20

I didn’t see any good common place to put Invariant<Q> struct, so I put a copy of it in both modules. Edit: Changed in additional commit.

[steffahn]

Technically a breaking change, but also a soundness-bug fix (and it only breaks code that’s very close to exploiting the unsoundness that's being fixed, anyways; in other words, it shouldn't affect sane users of this crate); so a new minor version seems appropriate when releasing this.

Also consider yanking old versions with an unsound API (once a version containing this PR is published)

[steffahn]

Added some commits that use Invariant<Q> for the Id<'id> type as-well, without the implication on Syncness that the previously used Cell<T> type implies. This also fixes up the auto implementations of [Ref]UnwindSafe for LCellOwner.

[uazu]

Thanks for the PR. I've read the Rustonomicon to understand things better and thought it all through. This all makes sense. Just one thing: I think using a tuple in (*const (), Invariant<Q>) is not necessary. It can just be *const Invariant<Q>. The comment above that line is incorrect -- using the tuple or using Invariant doesn't make any difference to UnwindSafe for TCell/TLCell. The only difference to UnwindSafe is due to the change to Id<'id> in LCell. It's worth getting the comments right, because this part of Rust is confusing enough as it is.

I think there are two options: Either I accept the PR as it is and I fix up things as above, or if you have time you could do it. I don't mind either way. I will add further changes afterwards anyway to add comments referencing the Rustonomicon and justifying it in my own way, to make things clearer for anyone who looks at this later on. Locally I have also added a couple of tests for the problem situation you documented in the bug report, which I will also check in later. These fail before the change and succeed afterwards, as expected, so that is all fine.

As you suggest, I will probably yank all versions back to the first release with TCell in it, once this is published.

[steffahn]

I think using a tuple in (*const (), Invariant<Q>) is not necessary. It can just be *const Invariant<Q>.

Ah, right… I guess I compared to *const Q which you’ve had previously (which does put extra restrictions on UnwindSafe).

The comment above that line is incorrect -- using the tuple or using Invariant doesn't make any difference to UnwindSafe for TCell/TLCell.

Let me take another look at those comments.

[steffahn]

Just noticing that TCell could simply drop the *const () part and get the Send implementation for free due to UnsafeCell…

[steffahn]

Another idea: Following the example of RefCell and Cell, it’s probably a good idea not to implement RefUnwindSafe on any of the *Cell types?

Edit: That would be a non-soundness-related breaking change though, so let’s not do that here.

Nevermind, UnsafeCell already gives us this anyway. I must have misread the docs somehow.

[steffahn]

The comment regarding UnwindSafe-ty is comparing Invariant<Q> with other options like Cell<Q> or *mut Q which would also be invariant in Q. So I think it’s correct, though it’s perhaps not clear enough to be understandable.

[steffahn]

The question of (*const (), Invariant<Q>) vs *const Invariant<Q> is a question of separation of concerns.

While *const Invariant<Q> works, it feels like hacking together two unrelated things. Maybe even better would be two separate PhantomData fields… the tuple was an attempt to avoid that, but looking at those error messages I’ve blessed, I guess those would become better if it was separate fields

[uazu]

So you're suggesting returning to using *const (), but in a separate PhantomData field in order to get clearer error messages? I think this sounds okay if you want to do that.

My own priorities are making the behaviour clear and have comments documenting why things are done as they are. I think the comment about UnwindSafe above uses of Invariant is not relevant to those places. Perhaps it could go on the Invariant definition to explain why that particular implementation of Invariant is used.

TLCell will need similar changes to TCell.

I will add tests after you finish so that the behaviour regarding auto-traits is well-defined and can be tested as Rust updates. Thanks

[steffahn]

So I’ve added a few commits now 😃

[steffahn]

TLCell will need similar changes to TCell.

right… I forgot that…

[uazu]

I think TCell needs to be like TLCell now, i.e. put back in the manual implementation of Send, because that gave a better error message.

[steffahn]

right, the error message would be slightly better for Send, because it wouldn’t mention the step involving UnsafeCell. On the other hand it’s somewhat worse for Sync (for TLCell) because it mentions both the UnsafeCell and the *const () (previously *const Q) when the marker field is present.

[steffahn]

I personally feel a bit reluctant about adding an additional marker fields and a manual unsafe impl for Send when UnsafeCell already provides perfectly reasonable defaults.

Actually, I guess the marker fields aren't necessary even with the manual impl. Nonetheless, I feel like UnsafeCell is doing a great job here, providing a conservative choice of !Sync and Send for UnsafeCell<T> where T: Send.

Also, the handling of Send via UnsafeCell is consistent with how QCell and LCell already do it.

[uazu]

Yes, that part is all fine. I will add tests to make sure that all the auto-traits come out as expected, so that won't get broken and we'll pick up on anything weird that occurs in Rust's implementation (or they'll pick it up in a crater run).

I was just reading a bit more on the drop checker (since PhantomData<Q> apparently invokes the drop checker (somehow), but PhantomData<Invariant<Q>> won't). But as far as I can see it is not relevant to this situation.

I think things are all pretty much fine to merge now. I'll just go over it all again to be sure though.

[uazu]

Thanks for putting back in those comments, BTW. I was going to do the same, but you've already done it so that's great.

[uazu]

I'll finish checking things over after lunch and merge then. Thanks!

[uazu]

Thanks again for this. I've added a changelog to the release now, and I've put all the credits for contributions in there.