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

Format doc comments #102285

Closed
wants to merge 2 commits into from
Closed

Conversation

AldaronLau
Copy link
Contributor

I find the lack of consistency in formatting in rust docs for core/std/alloc to be annoying.

This PR formats code blocks in doc comments by running the unstable format_code_in_doc_comments.

Running cargo fmt with this feature does make tidy fail (with trailing whitespace), so it had to be commented out in the rustfmt.toml after adding and running.

One thing I'm not sure about is why there were other formatting changes when I ran it. If needed, I could try to go through them and only grab the doc comment changes for the core/std/alloc libraries, removing the rest of the changes.

@rust-highfive
Copy link
Collaborator

r? @scottmcm

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

@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-rustdoc-json Area: Rustdoc JSON backend T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library 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. labels Sep 25, 2022
@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

@rustbot
Copy link
Collaborator

rustbot commented Sep 25, 2022

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

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

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing src/librustdoc/json/conversions.rs; otherwise, make sure you bump the FORMAT_VERSION constant.

cc @CraftSpider, @aDotInTheVoid, @Enselic, @obi1kenobi

Changes rustc_apfloat. rustc_apfloat is currently in limbo and you almost certainly don't want to change it (see #55993).

cc @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2022
@@ -538,7 +537,6 @@ pub trait Float:
/// NaN -> \c IEK_NAN
/// 0 -> \c IEK_ZERO
/// Inf -> \c IEK_INF
///
Copy link
Member

Choose a reason for hiding this comment

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

Dismissing my ping on the basis of these being trivial comment-only changes (tho I wish we could've just frozen this directory sigh).

If needed, I could try to go through them and only grab the doc comment changes for the core/std/alloc libraries, removing the rest of the changes.

Consider this comment a (tiny) vote for having a PR just for library/, the compiler itself in general is more likely to be chaotic, and even have weird snippets in comments etc.

But if people are willing to review the PR as-is, I have nothing against that, to be clear.

@Dylan-DPC
Copy link
Member

@bors rollup=never

@@ -534,7 +534,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
let receiver_place = loop {
match receiver.layout.ty.kind() {
ty::Ref(..) | ty::RawPtr(..) => break self.deref_operand(&receiver)?,
ty::Dynamic(..) => break receiver.assert_mem_place(), // no immediate unsized values
ty::Dynamic(..) => break receiver.assert_mem_place(), /* no immediate unsized values */
Copy link
Member

Choose a reason for hiding this comment

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

Some of these are strange -- there is no reason this needs changing I think

@RalfJung
Copy link
Member

RalfJung commented Sep 26, 2022

Generally we don't accept formatting PRs unless they are checked by tidy. Also this PR is way too big to be reviewable so it'd be advisable to split it up.

There's a question here of whether the time people will spend reviewing this (in particular for the part outside of library) couldn't be better spent on other work. Reviewer time is one of our most scarce resources.

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
Attempting with retry: make prepare
---
 Documenting core v0.0.0 (/checkout/library/core)
error: unresolved link to `File`
   --> library/core/src/primitive_docs.rs:69:1
    |
69  | / /// The `!` type, also called "never".
70  | | ///
71  | | /// `!` represents the type of computations which never resolve to any value at all. For example,
72  | | /// the [`exit`] function `fn exit(code: i32) -> !` exits the process without ever returning, and
...   |
264 | | /// [`Debug`]: fmt::Debug
265 | | /// [`default()`]: Default::default
    |
    |
    = note: `-D rustdoc::broken-intra-doc-links` implied by `-D warnings`
    = note: the link appears in this line:
            
            Default` for (eg.) [`File`] by just making [`default()`] panic.)
    = note: no item named `File` in scope
    = note: no item named `File` in scope
    = help: to escape `[` and `]` characters, add '\' before them like `\[` or `\]`
error: unresolved link to `default`
   --> library/core/src/primitive_docs.rs:69:1
    |
    |
69  | / /// The `!` type, also called "never".
70  | | ///
71  | | /// `!` represents the type of computations which never resolve to any value at all. For example,
72  | | /// the [`exit`] function `fn exit(code: i32) -> !` exits the process without ever returning, and
...   |
264 | | /// [`Debug`]: fmt::Debug
265 | | /// [`default()`]: Default::default
    |
    = note: the link appears in this line:
            
            
            Default` for (eg.) [`File`] by just making [`default()`] panic.)
                                                        ^^^^^^^^^^^
    = help: to link to the module, prefix with `mod@`: mod@default
    = note: this link resolves to the module `default`, which is not in the value namespace
error: could not document `core`

Caused by:
Caused by:
  process didn't exit successfully: `/checkout/obj/build/bootstrap/debug/rustdoc --edition=2021 --crate-type lib --crate-name core library/core/src/lib.rs --target x86_64-unknown-linux-gnu -o /checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/doc -Zunstable-options --check-cfg 'names()' --check-cfg 'values()' --error-format=json --json=diagnostic-rendered-ansi,artifacts,future-incompat -Z unstable-options --resource-suffix 1.66.0 --markdown-css rust.css --markdown-no-toc --index-page /checkout/src/doc/index.md -C metadata=abbb49baeba48eac -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/x86_64-unknown-linux-gnu/release/deps -L dependency=/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-std/release/deps --cfg=bootstrap -Csymbol-mangling-version=legacy -Zunstable-options -Zunstable-options '--check-cfg=values(bootstrap)' '--check-cfg=values(stdarch_intel_sde)' '--check-cfg=values(no_fp_fmt_parse)' '--check-cfg=values(no_global_oom_handling)' '--check-cfg=values(no_rc)' '--check-cfg=values(no_sync)' '--check-cfg=values(freebsd12)' '--check-cfg=values(backtrace_in_libstd)' '--check-cfg=values(target_env,"libnx")' '--check-cfg=values(target_os,"watchos")' '--check-cfg=values(target_arch,"asmjs","spirv","nvptx","nvptx64","le32","xtensa")' '--check-cfg=values(dont_compile_me)' --document-private-items -Dwarnings '-Wrustdoc::invalid_codeblock_attributes' --crate-version '1.66.0-nightly
  (ad4977e8a
  2022-09-25)' '-Zcrate-attr=doc(html_root_url="https://doc.rust-lang.org/nightly/")'` (exit status: 1)

@bors
Copy link
Contributor

bors commented Sep 26, 2022

☔ The latest upstream changes (presumably #102051) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -501,7 +501,7 @@ fn codegen_stmt<'tcx>(

#[cfg(any())] // This is never true
match &stmt.kind {
StatementKind::StorageLive(..) | StatementKind::StorageDead(..) => {} // Those are not very useful
StatementKind::StorageLive(..) | StatementKind::StorageDead(..) => {} /* Those are not very useful */
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why rust-fmt made that change.

I'll go through each change manually and remove changes like this, and just remove all changes to compiler/ anyway.

Copy link
Member

@scottmcm scottmcm left a comment

Choose a reason for hiding this comment

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

I'm with Ralf on this one -- one PR to do this across everything is too big, and procedurally can't really happen because different people should review the different parts.

I was tempted to just close, but it sounds like you're going to use this one for just library changes, so that would be ok I think. You might still consider closing this one and opening a new one so that you'll have a fresh PR without all the pings to the will-be-irrelevant people and teams.

@scottmcm scottmcm 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 Sep 26, 2022
@wesleywiser
Copy link
Member

Hi @AldaronLau! First off, let me say "thank you" for spending the time and effort to help improve the consistency of the repo's doc comments. 🙂

As @RalfJung mentions, reviewer time is both limited and precious and so for PRs like this is, it's critical that they be optimized for ease of review. To that end, I have a few suggestions that might help:

  • Split the changes into sets of distinct changes. A quick look at the diff suggests at least a few categories of changes (remove empty trailing doc comment, add whitespace around inline comment contents, rustfmt of doc codeblocks, etc) each of which could be their own commits and PRs.
  • Teach tidy to check for these issues. @RalfJung mentioned that we generally don't accept formatting PRs that aren't checked by tidy because it's hard to keep the formatting nice without tidy enforcing it and making it human enforced adds to reviewer load.
  • Try to minimize the scope of each individual PR (@scottmcm's suggestion is a good step towards this). That will require less time from each reviewer and also make it easier to merge the PRs themselves. (This PR has been open <24 hours and already has merge conflicts. Smaller PRs will be less likely to conflict with other PRs in the bors queue.)

Thanks for your help in this area!

@AldaronLau
Copy link
Contributor Author

Teach tidy to check for these issues. @RalfJung mentioned that we generally don't accept formatting PRs that aren't checked by tidy because it's hard to keep the formatting nice without tidy enforcing it and making it human enforced adds to reviewer load.

I'm not familiar with tidy, where can I get more information on how to "teach" it to enforce specific formatting rules?

@wesleywiser
Copy link
Member

The tidy tool lives in src/tools/tidy and you'll probably want to add some new checks in ui_tests.rs for things like "remove empty trailing doc comment" and "add whitespace around inline comment contents" sets of changes. With those two out of the way, perhaps enabling the format_code_in_doc_comments rustfmt option could be done a few crates at a time?

@JohnCSimon
Copy link
Member

ping from triage:
@AldaronLau

Can you please address the merge conflicts - and post your status on this PR?

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@AldaronLau
Copy link
Contributor Author

ping from triage: @AldaronLau

Can you please address the merge conflicts - and post your status on this PR?

FYI: when a PR is ready for review, send a message containing @rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

Sorry, I forgot about this - I'll reopen a smaller PR for /library only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-rustdoc-json Area: Rustdoc JSON backend S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library 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.