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

Discussion: Enforce formatting of doc comments #105198

Open
AldaronLau opened this issue Dec 3, 2022 · 14 comments
Open

Discussion: Enforce formatting of doc comments #105198

AldaronLau opened this issue Dec 3, 2022 · 14 comments
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-rustfmt Area: Rustfmt T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@AldaronLau
Copy link
Contributor

Currently, the examples in the Rust documentation is inconsistent in style. For instance, there are many examples that use 3 space tabs (probably accidentally), while most use 4.

How do we enforce that examples are formatted correctly? We have a few options.

Rustfmt

Use rustfmt's format_code_in_doc_comments.

While this initially seems like a good solution, there are a couple problems:

  • There are currently bugs in how this works that add additional whitespace, this is a problem with one correct solution (fixing the bugs).
  • Style of doc comments matches the style of the rest of the code exactly. This would enable use_small_heuristics = "Max", which is very undesirable for doc comments since shorter lines should always be preferred in doc comments.

How do we solve the second problem? I see two possible ways this can be done (by changing rust-fmt).

Add docs table to rustfmt.toml

Rustfmt could add support for alternate rustfmt settings for doc comments.

edition = "2021"
unstable_features = true
use_small_heuristics = "Max"
format_code_in_doc_comments = true

[docs]
use_small_heuristics = "Default"

Add rustfmt-docs.toml

Add an additional file that has the same format as rustfmt.toml, but applies to docs.


What if changes aren't/can't be made in Rustfmt?

There are 3 main solutions in this category.

Move docs out to their own folder with another rustfmt.toml file

This is something that could be done without additional tooling.

```rust
#![doc = include_str!("../docs/name_of_example.rs")]
```

Of course, this would be a very large change from where doc comments are written today, and it could be argued that having it separated out will make things harder to follow.

Extract docs with tooling and check against rustfmt.toml file

A tool could be made (as part of tidy) to verify docs are formatted correctly. Rust code could be parsed for doc comments, and written into temporary files in the same directory with a .rustfmt folder.

Extract docs with tooling and format with another rustfmt.toml file then overwrite

A tool could be made (as part of tidy) to autoformat doc comments. Implementation details would be similar to the previous option.


How should this be approached? I see pros and cons to all solutions, but I think it's something that definitely should be solved. Are there any approaches I missed? I tried to list all of the ones I could think of, but could have missed some.

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-rustfmt Area: Rustfmt labels Dec 3, 2022
@GuillaumeGomez
Copy link
Member

I think I remember an old issue suggesting this too. I think it'd be nice to format doc comments as well but that's something that might better brought up with the @rust-lang/rustfmt team?

@ytmimi
Copy link
Contributor

ytmimi commented Dec 5, 2022

We have a new unstable option called doc_comment_code_block_width that one can use alongside format_code_in_doc_comments to set the width.

We've also got rust-lang/rustfmt#5288 opened to address the trailing whitespace issue.

@AldaronLau
Copy link
Contributor Author

The one thing I'd be skeptical about is since it is desired by many individuals that the docs formatting specialize in a lot more than just code block width, that there would need to be doc_comment_* for a lot of format options. The things I know for sure are going to need to be different for docs for a PR to be accepted by the library team (from previous conversation) are at least:

  • Small heuristics
  • Column width
  • Trailing comment alignment

There are probably others I missed, too.

@tgross35
Copy link
Contributor

tgross35 commented Dec 6, 2022

Echoing what I mentioned here, enabling rustfmt on doc comments should probably wait on rust-lang/rustfmt#5601 and rust-lang/rustfmt#5536 to land

Those are both marked ready for merge so hopefully won't be too long

@ytmimi
Copy link
Contributor

ytmimi commented Dec 10, 2022

@AldaronLau would it be possible to link to some examples of what you're looking for or other discussions that you've had?

  • use_small_heuristics is just shorthand for setting other width configuration options.
  • I don't think we have a notion of column width that differs from the length of a line.
  • There aren't any options to control comment alignment.

Maybe this should be discussed somewhere else, but based on rust-lang/rustfmt#5623 I think it would also be useful to define a new code fence attribute that rustfmt understands as skip formatting and rustdoc understands as still run the doctest.

This could be useful in cases where issues / limitations in rustfmt prevent the user from formatting the doc comment exactly how they want.

@AldaronLau
Copy link
Contributor Author

AldaronLau commented Dec 10, 2022

I don't think we have a notion of column width that differs from the length of a line.

@ytmimi the new doc_comment_code_block_width option works for this.

Comment Formatting

There are many places in the Rust documentation where something like the following:

do_something();      // 1. First step
do_next();           // 2. Second step
do_something_else(); // 3. Third step

Would be reformatted as:

do_something(); // 1. First step
do_next(); // 2. Second step
do_something_else(); // 3. Third step

This is something that needs a flag to disable this kind of comment formatting specifically for doc comments before a new PR can be made.

Heuristics

There places in the Rust documentation where the following:

let abcd = [
    "a",
    "b",
    "c",
    "d",
];

Would be reformatted as:

let abcd = ["a", "b", "c", "d"];

This is undesirable, and needs a separate flag to change use_small_heuristics to Off specifically for doc comments.


Maybe this should be discussed somewhere else, but based on rust-lang/rustfmt#5623 I think it would also be useful to define a new code fence attribute that rustfmt understands as skip formatting and rustdoc understands as still run the doctest.

This could be useful in cases where issues / limitations in rustfmt prevent the user from formatting the doc comment exactly how they want.

If I'm understanding right, that would just be a shorthand for:

/// ```rust
/// # #![rustfmt::skip]
/// ```
pub struct Unit;

Right?

@AldaronLau
Copy link
Contributor Author

@ytmimi These requirements are based on the discussion in the PR comments for #104058, when I attempted to get something in place for this (which was admittedly way too soon).

@ytmimi
Copy link
Contributor

ytmimi commented Dec 10, 2022

the new doc_comment_code_block_width option works for this.

Great!


There are many places in the Rust documentation where something like the following:

do_something();      // 1. First step
do_next();           // 2. Second step
do_something_else(); // 3. Third step

Would be reformatted as:

do_something(); // 1. First step
do_next(); // 2. Second step
do_something_else(); // 3. Third step

Got it. to my knowledge rustfmt doesn't currently have any option to preserver the whitespace before a trailing comment.


needs a separate flag to change use_small_heuristics to Off specifically for doc comments.

If it's just use_small_heuristics then I don't see why we couldn't implement another doc_comment_* config for that, but if we need super fine grained control over doc comment formatting that varies drastically from the other configs then I can see the utility in a [docs] table in rustfmt.toml.

@ytmimi
Copy link
Contributor

ytmimi commented Dec 10, 2022

If I'm understanding right, that would just be a shorthand for:

/// ```rust
/// # #![rustfmt::skip]
/// ```
pub struct Unit;

Right?

You've got the right idea! Not to get too into the weeds, but hidden rustdoc lines prevent us from parsing the code block as valid rust since random #s at the start of a line aren't valid syntax. In order to get around this we convert all hidden lines into custom comments so # #![rustfmt::skip] becomes //#### #![rustfmt::skip] before formatting. Then we just make sure to reverse our transformation before emitting the formatted code block.

I had something more like this in mind:

// using `rustfmt::skip` here for consistency with `#[rustfmt::skip]` and `#![rustfmt::skip]`

/// ```rustfmt::skip
/// do_something();      // 1. First step
/// do_next();           // 2. Second step
/// do_something_else(); // 3. Third step
/// ```
pub struct Unit;

The main advantage here is that we could avoid trying to format the code block altogether instead of parsing the code block just to discover there's a #![rustfmt::skip], which will prevent formatting anyway (and for now hit a bug mentioned in rust-lang/rustfmt#5623).

This skipping behavior already exists when we encounter rustdoc's ignore and compile_fail attributes so the idea is to add a rustfmt specific attribute that won't prevent rustdoc from running doc tests.

Also, as demonstrated above it could be used as a workaround to not mess with comment alignment.

@calebcartwright
Copy link
Member

I'm going to suggest somewhat of a pivot in how this issue is utilized going forward. I'm not keen on discussing rustfmt specifics outside of rust-lang/rustfmt because that's where we have our issue tracker, do development, etc., not to mention that a couple lines of rustfmt-centric discussion on this thread already have well trodden discussions within r-l/rustfmt that I do not want to bifurcate.

We can share succinct updates and/or decisions from the rustfmt perspective here, but I think that's best provided as a short data point that can be used in the consideration of the questions/decisions outlined in the OP

@GuillaumeGomez
Copy link
Member

Or we can move this issue directly on rustfmt repository.

@calebcartwright
Copy link
Member

calebcartwright commented Jan 28, 2023

Or we can move this issue directly on rustfmt repository.

Understand the thinking, but I'm pretty strongly opposed to doing that. The purpose of this issue, at least AIUI, is to figure out whether or not to apply automated formatting to comments in Rust documentation, and if so, how.

In my opinion that's neither a rustfmt consideration nor decision, rustfmt is just one of the options under consideration for the "how".

Moving this issue to r-l/rustfmt would result in us having an issue that's half duplicates of existing rustfmt issues, and half off-topic/non-actionable for rustfmt

@zkrising
Copy link

Somewhat related, a lot of the official documentation uses move|| {} for move closures rather than the rustfmt-preferred move || {}. Is this intentional, or would it be preferable that this is consistent with rustfmt?

@GuillaumeGomez
Copy link
Member

Somewhat related, a lot of the official documentation uses move|| {} for move closures rather than the rustfmt-preferred move || {}. Is this intentional, or would it be preferable that this is consistent with rustfmt?

I think that's a typo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-rustfmt Area: Rustfmt T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants