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

attempt to clarify align_to docs #105245

Merged
merged 1 commit into from
Dec 9, 2022
Merged

attempt to clarify align_to docs #105245

merged 1 commit into from
Dec 9, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Dec 4, 2022

This is not intended the change the docs at all, but @workingjubilee said the current docs are incomprehensible to some people so this is an attempt to fix that. No idea if it helps, so -- feedback welcome.

(Please let's not use this to discuss changing the spec. Whoever wants to change the spec should please make a separate PR for that.)

@rustbot
Copy link
Collaborator

rustbot commented Dec 4, 2022

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@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 4, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 4, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@ehuss
Copy link
Contributor

ehuss commented Dec 4, 2022

Feedback from a random reader: It seems a bit confusing to be discussing "implementations". From my perspective, there is one implementation of the Standard Library, and I'm reading the docs for it. What other implementations are there? Why does this seem to be written as a generalized spec for the Standard Library API? That's not how the rest of the std docs are usually written.

I'd be interested in what was found confusing in the original docs, since they seem somewhat straightforward to me. The mention of an example situation (sanitizers) where the behavior may be different seems useful, but I would just tack that on to the end of the previous version. ("For example, a sanitizer may change the behavior of this function to…")

@workingjubilee
Copy link
Member

workingjubilee commented Dec 5, 2022

I agree "implementations" sounds... wonky.

@ehuss The Rust community by and large has had a pattern of advising each other to avoid this function because of the weakness of the guarantees and the wording being at risk for obfuscating the reasoning why, even when it might be an appropriate function, and that's why we discussed potentially clarifying it.

Also see #95851

@RalfJung
Copy link
Member Author

RalfJung commented Dec 5, 2022

I'd be interested in what was found confusing in the original docs

The specific feedback was that "you can only rely on this guarantee for performance, not correctness" was hard to understand. Sadly I don't know why or how this is hard to understand so all I can do is throw pasta at the wall and see what sticks. 🤷 I figured it'd help to pretend as if there were multiple implementations here and one wants to write code that is correct against all of them. Maybe that's not the right take.

This is not the only place where we do something like that though -- e.g. sort_unstable does not guarantee how equal elements are sorted. That's exactly the same kind of non-determinism. Somehow though that is less confusing? I'm honestly lost here, maybe @workingjubilee should make a PR since they seem to have a better idea for what even is the problem here...

@RalfJung
Copy link
Member Author

RalfJung commented Dec 5, 2022

I gave this another shot, based on a proposal by Jubilee.

@ehuss
Copy link
Contributor

ehuss commented Dec 5, 2022

This is not the only place where we do something like that though -- e.g. sort_unstable does not guarantee how equal elements are sorted. That's exactly the same kind of non-determinism. Somehow though that is less confusing

I read caveats like that in sort_unstable as an indication that the standard library's implementation might change in the future (similar to all the "may change in the future" warnings in the fs module, or anything mentioning the "current implementation"). It's not clear here if the align_to guarantees are lax because this implementation has limitations or might change. Or is it lax only because of things like the sanitizer case. If it is the latter, it's still important to know, but people may be more confident using it if there were stronger guarantees.

(I apologize if my comment veers into "change the spec" territory. But I think it is helpful to clarify exactly what the current intended guarantees are.)

@RalfJung
Copy link
Member Author

RalfJung commented Dec 5, 2022

@ehuss That's fair -- we don't really plan to change this in the future, the reason for the non-det is different here.

What do you think about the new proposal?

@thomcc
Copy link
Member

thomcc commented Dec 7, 2022

Sorry for the delay (real life, etc), this looks great to me. Even if I would like this to always behave correctly (I understand why this is difficult...), it's hard not to see this as a large improvement.

Still, even though I am strongly in favor of this, it adds a new guarantee to this functions documentation's implementation so I can't r+ it -- it needs a libs-api sign-off. Hopefully it will be painless.

@rustbot label +T-libs-api -T-libs

r? @Amanieu

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 7, 2022
@rustbot rustbot assigned Amanieu and unassigned thomcc Dec 7, 2022
@Amanieu
Copy link
Member

Amanieu commented Dec 8, 2022

LGTM! I think the new wording is much clearer than the old one. While it does technically add more guarantees, I think this is sufficiently minor to not require an FCP.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 8, 2022

📌 Commit ee21454 has been approved by Amanieu

It is now in the queue for this repository.

@bors 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 Dec 8, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2022
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#105216 (Remove unused GUI test)
 - rust-lang#105245 (attempt to clarify align_to docs)
 - rust-lang#105387 (Improve Rustdoc scrape-examples UI)
 - rust-lang#105389 (Enable profiler in dist-powerpc64le-linux)
 - rust-lang#105427 (Dont silently ignore rustdoc errors)
 - rust-lang#105442 (rustdoc: clean up docblock table CSS)
 - rust-lang#105443 (Move some queries and methods)
 - rust-lang#105455 (use the correct `Reveal` during validation)
 - rust-lang#105470 (Clippy: backport ICE fix before beta branch)
 - rust-lang#105474 (lib docs: fix typo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0b4d57b into rust-lang:master Dec 9, 2022
@rustbot rustbot added this to the 1.67.0 milestone Dec 9, 2022
@RalfJung RalfJung deleted the align_to branch December 12, 2022 08:49
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
attempt to clarify align_to docs

This is not intended the change the docs at all, but `@workingjubilee` said the current docs are incomprehensible to some people so this is an attempt to fix that. No idea if it helps, so -- feedback welcome.

(Please let's not use this to discuss *changing* the spec. Whoever wants to change the spec should please make a separate PR for that.)
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…iaskrgr

Rollup of 10 pull requests

Successful merges:

 - rust-lang#105216 (Remove unused GUI test)
 - rust-lang#105245 (attempt to clarify align_to docs)
 - rust-lang#105387 (Improve Rustdoc scrape-examples UI)
 - rust-lang#105389 (Enable profiler in dist-powerpc64le-linux)
 - rust-lang#105427 (Dont silently ignore rustdoc errors)
 - rust-lang#105442 (rustdoc: clean up docblock table CSS)
 - rust-lang#105443 (Move some queries and methods)
 - rust-lang#105455 (use the correct `Reveal` during validation)
 - rust-lang#105470 (Clippy: backport ICE fix before beta branch)
 - rust-lang#105474 (lib docs: fix typo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants