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

Move some things to std::sync::poison and reexport them in std::sync #134692

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

Conversation

GrigorenkoPV
Copy link
Contributor

@GrigorenkoPV GrigorenkoPV commented Dec 23, 2024

Tracking issue: #134646

r? @tgross35

I've used sync_poison_mod feature flag instead, because sync_poison had already been used back in 1.2.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 23, 2024
@rust-log-analyzer

This comment has been minimized.

@@ -167,6 +167,11 @@

#![stable(feature = "rust1", since = "1.0.0")]

// No formatting: this file is just re-exports, and their order is worth preserving.
#![cfg_attr(rustfmt, rustfmt::skip)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this skip just get applied to specific blocks, like in https://github.com/rust-lang/rust/blob/addbd001ec56741829f20a3000892f8620dd0843/library/core/src/unicode/mod.rs? Applying to the whole file would probably make it a bit too easy for things to get messy.

Copy link
Contributor Author

@GrigorenkoPV GrigorenkoPV Dec 23, 2024

Choose a reason for hiding this comment

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

Then pretty much every block of uses would have to be annotated, because otherwise rustfmt will regroup them.

IMO this looks even noisier and less accurate with all the additional attributes and inability to leave two blank lines between the groups (rustfmt removes double empty lines).

@tgross35
Copy link
Contributor

tgross35 commented Dec 23, 2024

I don't think this plays with rustdoc nicely. Searching for Mutex now gives the absolute std::sync::poison::Mutex path, where it has an "unstable" mark. But if you open it in std::sync::Mutex, it is fine.

These should probably be marked doc(inline) for now. But @rust-lang/rustdoc is there something we can do better for stable types reexported from unstable modules? This is also a problem for other types too, e.g. FromBytesUntilNulError looks like it is unstable because it got moved from core::ffi to core::ffi::c_str.

Edit: opened #134702

@GrigorenkoPV
Copy link
Contributor Author

These should probably be marked doc(inline) for now.

Done. This helped with the search, but not with the things like MutexGuard's doc's reference to Mutex::lock being converted to the absolute (unstable) std::sync::poison::Mutex path.

@tgross35
Copy link
Contributor

tgross35 commented Dec 23, 2024

I am not sure what you mean, could you post a screenshot (edit: on the rustdoc issue, #134702)? Unfortunately we probably should wait on a Rustdoc fix then, this would be a lot of API to have misleading documentation.

// * RwLock (nonpoison_rwlock)

// FIXME(sync_nonpoison): conditionally select the default flavor based on edition(?).
use self::poison as default_poison_flavor;
Copy link
Contributor

Choose a reason for hiding this comment

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

We are at least two years off from an edition where we would change these defaults, so I don't think this needs to be addressed at this point. Just reexporting from self::poison should be fine, things might change between now and the next edition.

@rust-log-analyzer

This comment has been minimized.

@GrigorenkoPV
Copy link
Contributor Author

#134702 got closed, but #134702 (comment) is still a problem.
I guess we can manually qualify all the links in docs to lead to std::sync::* until the std::sync::poison module is stabilized, but that sounds a bit suboptimal.

jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 27, 2024
…-better, r=GuillaumeGomez

rustdoc: use shorter paths as preferred canonical paths

This is a solution to [the `std::sync::poison` linking problem](rust-lang#134692 (comment)), and, in general, makes intra-doc links shorter and clearer.

> Done. This helped with the search, but not with the things like `MutexGuard`'s doc's reference to `Mutex::lock` being converted to the absolute (unstable) `std::sync::poison::Mutex` path.

cc `@tgross35`

r? `@GuillaumeGomez`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 27, 2024
Rollup merge of rust-lang#134806 - notriddle:notriddle/parent-path-is-better, r=GuillaumeGomez

rustdoc: use shorter paths as preferred canonical paths

This is a solution to [the `std::sync::poison` linking problem](rust-lang#134692 (comment)), and, in general, makes intra-doc links shorter and clearer.

> Done. This helped with the search, but not with the things like `MutexGuard`'s doc's reference to `Mutex::lock` being converted to the absolute (unstable) `std::sync::poison::Mutex` path.

cc `@tgross35`

r? `@GuillaumeGomez`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants