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

GrigorenkoPV · 2024-12-23T16:54:42Z

Tracking issue: #134646

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

try-job: x86_64-msvc

tgross35 · 2024-12-23T21:58:33Z

library/std/src/sync/mod.rs

 #![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)]


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.

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).

Alright, the blank line section breaks are fine but could you remove the double blank lines since rustfmt doesn't do those anywhere? I.e. \n\n\n -> \n\n.

tgross35 · 2024-12-23T22:04:15Z

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 · 2024-12-23T22:17:41Z

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 · 2024-12-23T22:20:32Z

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.

tgross35 · 2024-12-23T22:32:56Z

library/std/src/sync/mod.rs

+//  * RwLock (nonpoison_rwlock)
+
+// FIXME(sync_nonpoison): conditionally select the default flavor based on edition(?).
+use self::poison as default_poison_flavor;


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.

GrigorenkoPV · 2024-12-27T12:30:54Z

#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.

…-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`

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`

…-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`

tgross35 · 2024-12-31T04:20:28Z

It seems like the rustdoc output should be fixed with #134806, could you double check?

With that the only remaining thing is to drop default_poison_flavor #134692 (comment) and the small style nit above, rest lgtm.

@rustbot author

GrigorenkoPV · 2025-01-01T01:18:06Z

It seems like the rustdoc output should be fixed with #134806, could you double check?

Yes, works fine now.

With that the only remaining thing is to drop default_poison_flavor #134692 (comment)

Done.

and the small style nit above

Done

tgross35 · 2025-01-01T11:07:09Z

Thanks!

@bors r+

bors · 2025-01-01T11:07:12Z

📌 Commit 2ff5393 has been approved by tgross35

It is now in the queue for this repository.

Move some things to `std::sync::poison` and reexport them in `std::sync` Tracking issue: rust-lang#134646 r? `@tgross35` I've used `sync_poison_mod` feature flag instead, because `sync_poison` had already been used back in 1.2.

bors · 2025-01-02T06:00:57Z

⌛ Testing commit dd99d4c with merge e3acb18...

bors · 2025-01-02T06:55:47Z

💔 Test failed - checks-actions

GrigorenkoPV · 2025-01-02T12:23:11Z

I was not able to run debuginfo tests locally (they failed, maybe due to llvm version mismatch or something else, no idea), so I tried blessing them blindly and ended up adding an extra ::poison::. I removed it now. Hopefully, now it'll work.

notriddle · 2025-01-02T17:41:29Z

@bors r=tgross35

bors · 2025-01-02T17:41:31Z

📌 Commit ee2ad4d has been approved by tgross35

It is now in the queue for this repository.

tgross35 · 2025-01-02T19:59:31Z

@bors rollup=iffy

bors · 2025-01-03T06:40:32Z

⌛ Testing commit ee2ad4d with merge ac00fe8...

bors · 2025-01-03T09:33:15Z

☀️ Test successful - checks-actions
Approved by: tgross35
Pushing ac00fe8 to master...

rust-timer · 2025-01-03T10:50:55Z

Finished benchmarking commit (ac00fe8): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -4.1%, secondary -3.8%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-4.1%	[-4.1%, -4.1%]	1
Improvements ✅ (secondary)	-3.8%	[-3.8%, -3.8%]	1
All ❌✅ (primary)	-4.1%	[-4.1%, -4.1%]	1

Cycles

Results (primary -3.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.0%	[-3.0%, -3.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.0%	[-3.0%, -3.0%]	1

Binary size

Results (primary 0.0%, secondary 0.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.1%]	23
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.1%]	23

Bootstrap: 763.847s -> 763.746s (-0.01%)
Artifact size: 325.57 MiB -> 325.60 MiB (0.01%)

rustbot assigned tgross35 Dec 23, 2024

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

This comment has been minimized.

Sign in to view

tgross35 reviewed Dec 23, 2024

View reviewed changes

GrigorenkoPV force-pushed the sync_poision branch from 326d1e0 to b75bd5b Compare December 23, 2024 22:00

GrigorenkoPV force-pushed the sync_poision branch from b75bd5b to a39b164 Compare December 23, 2024 22:13

GrigorenkoPV force-pushed the sync_poision branch from a39b164 to 7b45289 Compare December 23, 2024 22:26

tgross35 reviewed Dec 23, 2024

View reviewed changes

GrigorenkoPV mentioned this pull request Dec 23, 2024

Rustdoc regression: stable items reexported from unstable modules appear unstable #134702

Closed

This comment has been minimized.

Sign in to view

GrigorenkoPV force-pushed the sync_poision branch from 7b45289 to 9007792 Compare December 23, 2024 23:53

notriddle mentioned this pull request Dec 26, 2024

rustdoc: use shorter paths as preferred canonical paths #134806

Merged

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 31, 2024

GrigorenkoPV force-pushed the sync_poision branch from 9007792 to 2ff5393 Compare January 1, 2025 01:18

tgross35 approved these changes Jan 1, 2025

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 1, 2025

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 2, 2025

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

ee2ad4d

GrigorenkoPV force-pushed the sync_poision branch from dd99d4c to ee2ad4d Compare January 2, 2025 12:21

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 2, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 3, 2025

bors merged commit ac00fe8 into rust-lang:master Jan 3, 2025
7 checks passed

rustbot added this to the 1.85.0 milestone Jan 3, 2025

This was referenced Jan 3, 2025

Mark OnceState::poison as pub #133240

Closed

Implement From<MutexGuard<'a, T>> for &'a Mutex<T> #134048

Closed

GrigorenkoPV deleted the sync_poision branch January 3, 2025 11:07

GrigorenkoPV mentioned this pull request Jan 23, 2025

Tracking Issue for sync_poison_mod #134646

Open

7 tasks

Uh oh!

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

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

Uh oh!

Conversation

GrigorenkoPV commented Dec 23, 2024 • edited by tgross35 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

tgross35 Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

GrigorenkoPV Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgross35 Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

tgross35 commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GrigorenkoPV commented Dec 23, 2024

Uh oh!

tgross35 commented Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgross35 Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

GrigorenkoPV commented Dec 27, 2024

Uh oh!

tgross35 commented Dec 31, 2024

Uh oh!

GrigorenkoPV commented Jan 1, 2025

Uh oh!

tgross35 commented Jan 1, 2025

Uh oh!

bors commented Jan 1, 2025

Uh oh!

bors commented Jan 2, 2025

Uh oh!

This comment has been minimized.

bors commented Jan 2, 2025

Uh oh!

GrigorenkoPV commented Jan 2, 2025

Uh oh!

notriddle commented Jan 2, 2025

Uh oh!

bors commented Jan 2, 2025

Uh oh!

tgross35 commented Jan 2, 2025

Uh oh!

bors commented Jan 3, 2025

Uh oh!

bors commented Jan 3, 2025

Uh oh!

Uh oh!

rust-timer commented Jan 3, 2025

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

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

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

GrigorenkoPV commented Dec 23, 2024 •

edited by tgross35

Loading

GrigorenkoPV Dec 23, 2024 •

edited

Loading

tgross35 commented Dec 23, 2024 •

edited

Loading

tgross35 commented Dec 23, 2024 •

edited

Loading