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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

geeklint
Copy link
Contributor

@geeklint geeklint commented Mar 9, 2023

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
Copy link
Owner

uazu commented Mar 19, 2023

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
Copy link
Contributor Author

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
# 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::<u8>::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::<u8>::new()));
 owner.as_mut().rw(&item).push(1);
 test(owner, &item);
# }

 fn test(owner: Pin<&mut QCellOwnerPinned>, item: &Rc<QCell<Vec<u8>>>) {
     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.

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.

None yet

2 participants