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

Disallow !Sync types in #[once] fixtures #237

Merged
merged 3 commits into from Apr 3, 2024

Conversation

narpfel
Copy link
Contributor

@narpfel narpfel commented Apr 2, 2024

!Sync types must not be shared to different threads by reference.

Note: This PR is an API break (because !Sync types were allowed before). If !Sync types are required for #[once] fixtures, they can be wrapped in a Mutex to prevent data races.

This PR also bumps the MSRV to Rust 1.60 due to the usage of once_cell. Rust 1.60 was released on 2022-04-07.

Resolves #235.

@la10736
Copy link
Owner

la10736 commented Apr 3, 2024

THX for this PR. When I implemented this feature once_cell was not stable yet. I noted just now your're not using OnceCell from std::cell::once_cell. I'm not incline to use an external dependency for it.

Copy link
Owner

@la10736 la10736 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we can remove this external dependency and use std::sync::OnceLock instead.

Moreover your changes to errors.rs file introduce an annoying regression 😢. The compiler stop t show some of the errors in the first lines of the file... this kind of tests are fragile. I solved it by separate in 2 files: I'll share my branch if you like.

Finally, can you add a log line in changelog file?

@la10736
Copy link
Owner

la10736 commented Apr 3, 2024

@narpfel
Copy link
Contributor Author

narpfel commented Apr 3, 2024

I'm not incline to use an external dependency for it.

👍 I was not sure what the MSRV policy for rstest is, so I picked the option that hat a lower MSRV.

Moreover your changes to errors.rs file introduce an annoying regression 😢.

I only tested with nightly locally. 😊

Finally, can you add a log line in changelog file?

Done.

@la10736 la10736 self-requested a review April 3, 2024 16:27
Copy link
Owner

@la10736 la10736 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!! THX

@la10736 la10736 merged commit 3f91774 into la10736:master Apr 3, 2024
1 check passed
@narpfel narpfel deleted the once-fixture-not-sync-types branch April 3, 2024 16:43
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.

#[once] fixtures are unsound for types that are not Sync
2 participants