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

Fix legacy_const_generic doc arguments display #89954

Merged
merged 2 commits into from
Dec 3, 2021

Conversation

GuillaumeGomez
Copy link
Member

Fixes #83167.

cc @Amanieu

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: rustdoc UI (generated HTML) labels Oct 16, 2021
@rust-highfive
Copy link
Collaborator

r? @ollie27

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 16, 2021
@GuillaumeGomez
Copy link
Member Author

r? @jyn514

@rust-highfive rust-highfive assigned jyn514 and unassigned ollie27 Oct 16, 2021
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

I don't really have time to review this properly ... @Amanieu @Mark-Simulacrum does this look like about the transformation you expect?

src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Updated.

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

I think some indication that the arguments must be const should be given -- there's no canonical Rust syntax for this yet, though.

@jyn514 jyn514 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 Oct 19, 2021
@JohnCSimon JohnCSimon 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 8, 2021
@GuillaumeGomez GuillaumeGomez added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 10, 2021
@GuillaumeGomez
Copy link
Member Author

@Mark-Simulacrum So what should be changed here exactly? If the syntax isn't present yet, I'm not sure what I can do but if you have suggestions, please go ahead. :)

@Mark-Simulacrum
Copy link
Member

There's some discussion starting at #83167 (comment) of options for rendering.

Can you provide a screenshot of what one of these functions looks like before/after this PR? That might help with discussion since currently reading the code seems necessary.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Dec 1, 2021

Sure, here you go.

Current version:

Screenshot from 2021-12-01 20-58-01
Screenshot from 2021-12-01 20-58-04

With this PR (no more generics):

Screenshot from 2021-12-01 21-08-32
Screenshot from 2021-12-01 21-08-29

I also rebased on master.

@Mark-Simulacrum
Copy link
Member

Yeah, ok, that matches what I thought.

I would suggest adding const before the argument name -- that may be a useful signal that the argument is special. Users can't currently create such an argument themselves, which is unfortunate, but feels a little unavoidable to me at this time.

I think that would move us in the right direction, but I'm not sure it's enough, particularly for call-sites which are using the regular const-generic syntax (i.e., with turbofish), which will look like they're omitting valid options. One benefit to us might be that I think all such "weird" functions are currently only leaf functions (i.e., not trait methods, etc) so we may be able to just duplicate the signature twice with an "or" in between or something like that, though it's probably painful to do in rustdoc's current design from what I recall.

Overall, I think it's important to show users both styles if neither style issues deprecation warnings (which I believe is currently the case), and important to signal the need for the argument to be const. It may be worth nominating for libs-api to discuss whether there is a preferred syntax for calling these functions, and whether we wish to deprecate one of the forms; that would alleviate the concern over users encountering both syntaxes in the wild -- at least over time.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Dec 1, 2021

Let's ping them then. cc @rust-lang/libs-api

@GuillaumeGomez GuillaumeGomez added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 1, 2021
@Mark-Simulacrum Mark-Simulacrum added the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Dec 1, 2021
@Amanieu
Copy link
Member

Amanieu commented Dec 1, 2021

I personally think that just having:

pub fn foo(x: usize, const Y: usize, z: usize);

is enough since that is the form used by the vendor APIs. I don't think we need to expose the desugared generic form of the function in the documentation.

@Mark-Simulacrum
Copy link
Member

I largely agree, with a possible caveat of a single paragraph at the root (core::arch module docs) explaining this, if it doesn't exist yet. I think it would definitely be an improvement on the status quo.

@GuillaumeGomez
Copy link
Member Author

It now looks like this:

Screenshot from 2021-12-02 11-20-11
Screenshot from 2021-12-02 11-20-16

@Amanieu
Copy link
Member

Amanieu commented Dec 2, 2021

LGTM

@GuillaumeGomez
Copy link
Member Author

@bors: r=Amanieu

@bors
Copy link
Contributor

bors commented Dec 2, 2021

📌 Commit 5c75a48 has been approved by Amanieu

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 2, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 2, 2021
…-doc, r=Amanieu

Fix legacy_const_generic doc arguments display

Fixes rust-lang#83167.

cc `@Amanieu`
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 2, 2021
…askrgr

Rollup of 12 pull requests

Successful merges:

 - rust-lang#89954 (Fix legacy_const_generic doc arguments display)
 - rust-lang#91321 (Handle placeholder regions in NLL type outlive constraints)
 - rust-lang#91329 (Fix incorrect usage of `EvaluatedToOk` when evaluating `TypeOutlives`)
 - rust-lang#91364 (Improve error message for incorrect field accesses through raw pointers)
 - rust-lang#91387 (Clarify and tidy up explanation of E0038)
 - rust-lang#91410 (Move `#![feature(const_precise_live_drops)]` checks earlier in the pipeline)
 - rust-lang#91435 (Improve diagnostic for missing half of binary operator in `if` condition)
 - rust-lang#91444 (disable tests in Miri that take too long)
 - rust-lang#91457 (Add additional test from rust issue number 91068)
 - rust-lang#91460 (Document how `last_os_error` should be used)
 - rust-lang#91464 (Document file path case sensitivity)
 - rust-lang#91466 (Improve the comments in `Symbol::interner`.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 444635d into rust-lang:master Dec 3, 2021
@rustbot rustbot added this to the 1.59.0 milestone Dec 3, 2021
@GuillaumeGomez GuillaumeGomez deleted the legacy-const-generic-doc branch December 3, 2021 09:14
@dtolnay dtolnay removed the I-libs-api-nominated The issue / PR has been nominated for discussion during a libs-api team meeting. label Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: rustdoc UI (generated HTML) C-discussion Category: Discussion or questions that doesn't represent real issues. 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use const generics for stdarch intrinsics