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

Add more syscall doc aliases to std docs #113891

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SabrinaJewson
Copy link
Contributor

@SabrinaJewson SabrinaJewson commented Jul 20, 2023

Some of these are quite trivial but we should optimize for the user not having to think about it.

Blocked on rust-lang/std-dev-guide#64

@rustbot
Copy link
Collaborator

rustbot commented Jul 20, 2023

r? @joshtriplett

(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 Jul 20, 2023
Copy link
Contributor Author

@SabrinaJewson SabrinaJewson left a comment

Choose a reason for hiding this comment

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

This PR breaks the doc alias policy in quite a few places, which I’ve now highlighted. I left them in the PR itself because I wasn’t sure what the best course of action was. (In my opinion, after making this PR, the policy is overly restrictive).

@@ -1937,6 +1943,7 @@ pub fn metadata<P: AsRef<Path>>(path: P) -> io::Result<Metadata> {
/// Ok(())
/// }
/// ```
#[doc(alias = "lstat", alias = "GetFileAttributes", alias = "GetFileAttributesEx")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Windows functions here break the doc alias policy, since GetFileAttributes is now mapped to multiple functions. We now have to weigh up the inconsistency of breaking the doc alias policy versus the inconsistency of offering the alises only on Unix.

@@ -2038,6 +2046,10 @@ pub fn rename<P: AsRef<Path>, Q: AsRef<Path>>(from: P, to: Q) -> io::Result<()>
/// Ok(())
/// }
/// ```
#[doc(alias = "cp")]
#[doc(alias = "copy_file_range")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another instance of breaking the doc alias policy, where io::copy is also aliased to copy_file_range. I’m not sure which should win out here, because when a user wants to call copy_file_range they could plausibly end up calling either of these two functions.

library/std/src/fs.rs Outdated Show resolved Hide resolved
@@ -2276,6 +2289,7 @@ pub fn create_dir<P: AsRef<Path>>(path: P) -> io::Result<()> {
/// Ok(())
/// }
/// ```
#[doc(alias = "mkdir", alias = "CreateDirectory")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another instance where we either choose to break the alias policy, or have to decide which one of create_dir vs create_dir_all “wins out” and gets the alias.

@@ -616,6 +617,7 @@ pub fn symlink_file<P: AsRef<Path>, Q: AsRef<Path>>(original: P, link: Q) -> io:
/// the process as an administrator.
///
/// [symlink-security]: https://docs.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/create-symbolic-links
#[doc(alias = "CreateSymbolicLink", alias = "CreateSymbolicLinkW")]
Copy link
Contributor Author

@SabrinaJewson SabrinaJewson Jul 20, 2023

Choose a reason for hiding this comment

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

A case similar to the create_dir[_all] case: Which one of these functions (symlink_file or symlink_dir) should win out and get to take the alias?

library/std/src/time.rs Outdated Show resolved Hide resolved
@joshtriplett
Copy link
Member

All of these seem fine to me, but need review and confirmation regarding the alias policy.

r? libs

@Mark-Simulacrum
Copy link
Member

A couple of thoughts:

  • Putting aliases from many different platforms (e.g., Windows and Linux and WASI and ...) into the same search context feels like it may not be a good idea. Unfortunately we don't make it easy to pick the platform, but it would seem unfortunate to me to pollute the search namespace with a bunch of different names. It's also much more likely to run into collisions (e.g., a single name doing two different things on different platforms).
  • The cases where the alias being added already starts with the same name (e.g., Create* and a Rust function create_...) seems to carry less value to me. If someone copy/pastes or writes the full name before looking at search results, it might be helpful, but it feels like the alias feature isn't a great fit for that interaction pattern. It might make more sense to include the aliases in such a case into the regular doc comments on the relevant functions, so that hopefully a full-text search engine (e.g., Google) can better index the contents.

My sense is that aliases are most helpful where the libc name and concept are somewhat obscure (e.g., popcnt) and it's not clear what the name Rust would choose is. Where the name or location of that concept in the standard library are much clearer, I think the value significantly diminishes. For example, ~all of the aliases on Instant and SystemTime proposed here feel excessive to me: if I'm looking for time, I can already get very good results with time in the search bar.

I'm not sure what the best way to get this reviewed is. @SabrinaJewson, are you still interested in pushing this forward? Perhaps we can split out the non-controversial bits and get those landed first, and then solicit more opinions from the library team on what folks think here?

@Mark-Simulacrum Mark-Simulacrum 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 Feb 17, 2024
@SabrinaJewson
Copy link
Contributor Author

The cases where the alias being added already starts with the same name (e.g., Create* and a Rust function create_...) seems to carry less value to me. If someone copy/pastes or writes the full name before looking at search results, it might be helpful, but it feels like the alias feature isn't a great fit for that interaction pattern.

I think it could still be useful — there’s some value in not having to think, just copypasting the name of a system function you’re looking at the docs of and getting taken exactly to its Rust wrapper. After all, isn’t that what the existing memcpy aliases are for?

I’m still interested in pushing this forward. The speed of this isn’t important to me, and I’m also not certain which parts are non-controversial, so I don’t immediately know how I would split this.

Mainly, I observed there was a steady stream of PRs adding specific doc aliases at random — this PR was made to bring everything in line at once.

@Mark-Simulacrum
Copy link
Member

Let's do this:

  • Split out everything that's not commented on/referenced by comments. We can land those immediately.
  • Drop the aliases on the time types (or put those in a separate PR and I can nominate for libs-api discussion).
  • File a PR against https://github.com/rust-lang/std-dev-guide/blob/master/src/policy/doc-alias.md with revisions indicating that duplicates are OK where Rust has split functionality (e.g., File + Directory), but the underlying function is the same for both. I think that may make sense, we can discuss it there and then land those cases.

Hopefully that gives us an incremental path forward, anything not covered we can revisit once that's all squared away.

Thanks!

@@ -2359,6 +2373,7 @@ pub fn remove_dir<P: AsRef<Path>>(path: P) -> io::Result<()> {
/// Ok(())
/// }
/// ```
#[doc(alias = "rm")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(seems like I forgot this one, it also breaks the policy).

@rustbot rustbot added O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows labels Feb 18, 2024
@SabrinaJewson
Copy link
Contributor Author

PRs have been filed. This PR is now blocked on rust-lang/std-dev-guide#64

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 18, 2024
…, r=Mark-Simulacrum

Add uncontroversial syscall doc aliases to std docs

This PR contains the parts of rust-lang#113891 that don’t break the doc alias policy.

r? `@Mark-Simulacrum`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 18, 2024
Rollup merge of rust-lang#121266 - SabrinaJewson:easy-syscall-aliases, r=Mark-Simulacrum

Add uncontroversial syscall doc aliases to std docs

This PR contains the parts of rust-lang#113891 that don’t break the doc alias policy.

r? `@Mark-Simulacrum`
@Dylan-DPC Dylan-DPC added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows S-blocked Status: Blocked on something else such as an RFC or other implementation work. 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.

5 participants