Document `From::from` impls #137330

TimTheBig · 2025-02-20T18:09:28Z

This resolves #51430 by documenting all from impls in lib std and core.
I still need someone to look over a few of the comments to see if the style is correct.

rustbot · 2025-02-20T18:09:39Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2025-02-20T18:09:42Z

The Miri subtree was changed

cc @rust-lang/miri

Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead.

cc @calebzulawski, @programmerjake

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

TimTheBig · 2025-02-20T18:12:09Z

@rustbot label A-docs C-enhancement T-libs-api

TimTheBig · 2025-02-20T18:16:34Z

Should I move Portable SIMD and rust-analyzer changes even though they are small?

RalfJung · 2025-02-20T18:19:54Z

It is generally a good idea to split up large changes. :) You changed a whole bunch of files in rust-analyzer, so yeah that should be split. Miri and clippy have fewer changes, but the fact that there are any changes at all there still contradicts the PR description.

It's too late now, but the best strategy for PRs like this is to start with just a few files, e.g. just core, take the review feedback, and then apply that to the next PRs. That avoids having to re-do large amounts of work when there is overarching feedback.

rustbot · 2025-02-20T18:27:10Z

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Portable SIMD is developed in its own repository. If possible, consider making this change to rust-lang/portable-simd instead.

cc @calebzulawski, @programmerjake

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

TimTheBig · 2025-02-20T18:29:05Z

My mistake, I will revert the rust-analyzer changes.

This reverts commit df90691.

tests/ui/try-block/try-block-bad-type.stderr

library/core/src/ptr/alignment.rs

library/proc_macro/src/bridge/buffer.rs

library/proc_macro/src/bridge/closure.rs

library/proc_macro/src/bridge/rpc.rs

library/proc_macro/src/lib.rs

library/proc_macro/src/bridge/rpc.rs

library/proc_macro/src/bridge/closure.rs

Co-authored-by: Oli Scherer <[email protected]>

src/tools/clippy/clippy_lints/src/attrs/mixed_attributes_style.rs

library/proc_macro/src/bridge/buffer.rs

Co-authored-by: Alejandra González <[email protected]>

oli-obk · 2025-03-07T06:55:29Z

r? libs

There are to many impls I haven't made much review progress

workingjubilee

I do not believe you have self-reviewed this adequately, and I do not agree with the direction of this PR. Please first remove all instances where the documentation is trivial... for example, for From<X> for Y, the documentation is most certainly trivial when it is "Wraps X in Y"... or for only internal-facing APIs.

library/portable-simd/crates/core_simd/src/masks/full_masks.rs

library/portable-simd/crates/core_simd/src/vector.rs

workingjubilee · 2025-03-07T07:31:07Z

library/portable-simd/crates/core_simd/src/masks.rs

+    /// Converts a SIMD mask to an array of bools.
+    ///
+    /// ## Cost
+    /// Copies the bytes of the array


incorrect and furthermore misleading, as the real cost is the usage of instructions for converting the mask types to the boolean values, which can be extremely expensive relative to merely moving the bytes around. these may perform essentially arbitrary computation to extract the mask from the opaque representation into a sequence of bytes where each byte is exactly 0x00 or 0x01.

it's still relatively cheap, but merely moving bytes can easily be, well, the easy part, and for people who care about the cost of something done in SIMD, it can be one of the highest costs in their program!

library/core/src/ptr/non_null.rs

library/core/src/ptr/alignment.rs

library/core/src/fmt/mod.rs

workingjubilee · 2025-03-07T07:35:26Z

library/alloc/src/task.rs

@@ -109,6 +109,7 @@ pub trait Wake {
 impl<W: Wake + Send + Sync + 'static> From<Arc<W>> for Waker {
    /// Use a [`Wake`]-able type as a `Waker`.
    ///
+    /// ## Cost


I do not agree with this new "Cost" heading

Why so, I think it makes it more clear.

workingjubilee

It's too late now,

I do not think it is too late. Factoring this PR to make it reviewable is now a blocking consideration for this PR now that it is in my queue. I'm sorry you had to wait to hear that, @TimTheBig, but this PR has to be reviewed fairly carefully to make sure that it does not present anything that could be misinterpreted as new stable guarantees about how these APIs work, unless they are trivially obvious to guarantee. In many cases these are actually not as obvious to me, so I am going to insist that this is well-organized, at minimum.

The entire library API surface change of this should be separated from anything that documents only internal-facing APIs or only affects things in src/tools

library/alloc/src/bstr.rs

TimTheBig · 2025-03-08T02:30:54Z

@programmerjake, @oli-obk, @blyxyas, @workingjubilee, thanks for all the help.

library/alloc/src/bstr.rs

library/core/src/fmt/mod.rs

library/core/src/ptr/non_null.rs

library/portable-simd/crates/core_simd/src/masks/full_masks.rs

library/portable-simd/crates/core_simd/src/vector.rs

library/core/src/ptr/alignment.rs

Co-authored-by: Jubilee <[email protected]>

rustbot assigned Amanieu Feb 20, 2025

fmease changed the title ~~Document from~~ Document From::from impls Feb 20, 2025

This comment has been minimized.

Sign in to view

RalfJung mentioned this pull request Feb 20, 2025

rustbot sends the same pings multiple times rust-lang/triagebot#1738

Open

This comment has been minimized.

Sign in to view

rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 20, 2025

This comment has been minimized.

Sign in to view

TimTheBig added 5 commits February 20, 2025 14:45

Add from docs for tools

d07d7fc

Add from docs to all impls in lib std and core

a47f69f

Fix formatting and add more docs for bstr

abc0f94

Improve consistence

8de849e

Revert "Add from docs for tools"

d5de4c6

This reverts commit df90691.

TimTheBig commented Feb 21, 2025

View reviewed changes

tests/ui/try-block/try-block-bad-type.stderr Outdated Show resolved Hide resolved

Apply suggestions from code review

e90edee

programmerjake reviewed Feb 21, 2025

View reviewed changes

library/core/src/ptr/alignment.rs Outdated Show resolved Hide resolved

TimTheBig commented Feb 23, 2025

View reviewed changes

library/core/src/ptr/alignment.rs Outdated Show resolved Hide resolved

Update library/core/src/ptr/alignment.rs

5debdd7

oli-obk reviewed Feb 24, 2025

View reviewed changes

TimTheBig commented Feb 24, 2025

View reviewed changes

library/proc_macro/src/lib.rs Outdated Show resolved Hide resolved

TimTheBig commented Feb 24, 2025

View reviewed changes

library/proc_macro/src/bridge/rpc.rs Outdated Show resolved Hide resolved

TimTheBig commented Feb 24, 2025

View reviewed changes

library/proc_macro/src/bridge/closure.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

dd16ccc

Co-authored-by: Oli Scherer <[email protected]>

blyxyas reviewed Feb 24, 2025

View reviewed changes

src/tools/clippy/clippy_lints/src/attrs/mixed_attributes_style.rs Outdated Show resolved Hide resolved

TimTheBig commented Feb 26, 2025

View reviewed changes

library/proc_macro/src/bridge/buffer.rs Outdated Show resolved Hide resolved

Apply suggestions from code review

50687f8

Co-authored-by: Alejandra González <[email protected]>

TimTheBig requested a review from oli-obk March 6, 2025 23:17

rustbot assigned workingjubilee and unassigned Amanieu Mar 7, 2025

workingjubilee requested changes Mar 7, 2025

View reviewed changes

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 Mar 7, 2025

workingjubilee reviewed Mar 7, 2025

View reviewed changes

library/alloc/src/bstr.rs Outdated Show resolved Hide resolved

library/alloc/src/bstr.rs Outdated Show resolved Hide resolved