uazu / qcell

Statically-checked alternatives to RefCell and RwLock
Apache License 2.0
356 stars 22 forks source link

replace pin_utils::pin_mut with std::pin::pin #44

Open geeklint opened 1 year ago

geeklint commented 1 year ago

Rust 1.68 was just released and it stabilizes the std::pin::pin! macro, which provides a way to do with just std what qcell was previously using pin_utils::pin_mut for. This PR would remove that dependency, but would also bump the MSRV for some of the tests to 1.68.0.

I think it would be good for the examples to use the std macro (since I assume this is the preferred way now that it's stable), but if there are MSRV concerns this PR doesn't have to happen immediately.

uazu commented 1 year ago

I agree with the change, but yes the Rust version is an issue. As far as I can see for earlier versions of Rust it only affects cargo test (not clippy or a build), but it affects both normal tests and doc-tests, so it's not easy to disable those tests on earlier versions of Rust. (I tried using rustversion and hit issues.) A normal build that doesn't run cargo test should succeed, though, so maybe crate-users would be unaffected. Actually reading further, it seems that running cargo test on dependencies is not possible and cannot be supported in general because many crates don't package all the files required for testing. However people may have implemented workarounds for that, because testing dependencies on new or little-used platforms does turn up bugs.

So the only downside would be that it is no longer possible to test this crate on earlier versions of Rust, but crate-users on older versions of Rust should be unaffected. It's important to support users on some earlier versions of Rust, to some distance in the past (not sure how far back though), because I'm sure they are out there, and at my workplace we would also be affected.

Do you have any other ideas on how to handle this, apart from just waiting some time before applying the PR?

geeklint commented 1 year ago

Correct, it's my understanding that it should only effect people testing qcell itself, not users of the crate.

The greatest value is probably just the examples in the doc comment for the QCellOwnerPinned type; we could leave the code in the tests submodule and the doctest_qcell_no_alloc module unchanged, and the cfg the two doctests attached to that type.

I just played around with it a bit and it looks like we can actually make the doctest use rustversion, it's just a little involved:

doctest with multiple versions ```rust # use std::{rc::Rc, pin::Pin}; use qcell::{QCell, QCellOwnerPinned}; # #[rustversion::before(1.68)] # fn main() { # use pin_utils::pin_mut; # let mut owner = QCellOwnerPinned::new(); # pin_mut!(owner); # let item = Rc::new(owner.as_ref().cell(Vec::::new())); # owner.as_mut().rw(&item).push(1); # test(owner, &item); # # } # #[rustversion::since(1.68)] # fn main() { # use std::pin::pin; let mut owner = pin!(QCellOwnerPinned::new()); let item = Rc::new(owner.as_ref().cell(Vec::::new())); owner.as_mut().rw(&item).push(1); test(owner, &item); # } fn test(owner: Pin<&mut QCellOwnerPinned>, item: &Rc>>) { owner.rw(&item).push(2); } ```

It appears to me like we would have to make rustversion a full dependency (not a dev-dep) to completely gate the test so it doesn't exist on older versions, alternatively we could use a feature flag instead of rustversion which has the benefit of not requiring the dependency and could gate the entire test, but is somewhat easier to misuse.