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_code_in_doc_comments changes formatting of trailing comments in a list #5568

Closed
lopopolo opened this issue Oct 22, 2022 · 9 comments · Fixed by #5536
Closed

format_code_in_doc_comments changes formatting of trailing comments in a list #5568

lopopolo opened this issue Oct 22, 2022 · 9 comments · Fixed by #5536
Labels
a-comments only-with-option requires a non-default option value to reproduce

Comments

@lopopolo
Copy link

The format_code_in_doc_comments option changes the formatting of inline comments that trail each item in a multi-line list to no longer be aligned.

Version

$ rustc +nightly -Vv
rustc 1.66.0-nightly (5c8bff74b 2022-10-21)
binary: rustc
commit-hash: 5c8bff74bc1c52bef0c79f3689bb227f51f3e82d
commit-date: 2022-10-21
host: x86_64-apple-darwin
release: 1.66.0-nightly
LLVM version: 15.0.2
$ rustfmt +nightly -Vv
rustfmt 1.5.1-nightly (5c8bff74 2022-10-21)

Diff

diff --git i/artichoke-backend/build.rs w/artichoke-backend/build.rs
index 5f4d1def48..d14feb11e8 100644
--- i/artichoke-backend/build.rs
+++ w/artichoke-backend/build.rs
@@ -99,10 +99,10 @@ mod libs {
             "mrbgems/mruby-eval/src/eval.c",         // eval, instance_eval, and friends
             "mrbgems/mruby-fiber/src/fiber.c",       // Fiber class from core, required by `Enumerator`
             "mrbgems/mruby-metaprog/src/metaprog.c", // APIs on Kernel and Module for accessing classes and variables
-            "mrbgems/mruby-method/src/method.c",     // `Method`, `UnboundMethod`, and method APIs on Kernel and Module
-            "mrbgems/mruby-pack/src/pack.c",         // Array#pack and String#unpack
-            "mrbgems/mruby-proc-ext/src/proc.c",     // NOTE(GH-32): This gem is required by `mruby-method`.
-            "mrbgems/mruby-sprintf/src/sprintf.c",   // Kernel#sprintf, Kernel#format, String#%
+            "mrbgems/mruby-method/src/method.c", // `Method`, `UnboundMethod`, and method APIs on Kernel and Module
+            "mrbgems/mruby-pack/src/pack.c",     // Array#pack and String#unpack
+            "mrbgems/mruby-proc-ext/src/proc.c", // NOTE(GH-32): This gem is required by `mruby-method`.
+            "mrbgems/mruby-sprintf/src/sprintf.c", // Kernel#sprintf, Kernel#format, String#%
         ]
         .into_iter()
         .map(|source| paths::mruby_root().join(source))
diff --git i/rustfmt.toml w/rustfmt.toml
index 07378fa859..5b0a52aa0e 100644
--- i/rustfmt.toml
+++ w/rustfmt.toml
@@ -13,3 +13,4 @@ use_try_shorthand = true
 
 # nightly only option
 group_imports = "StdExternalCrate"
+format_code_in_doc_comments = true
lopopolo added a commit to artichoke/artichoke that referenced this issue Oct 22, 2022
Use the nightly-only rustdoc option `format_code_in_doc_comments`:
https://rust-lang.github.io/rustfmt/?version=v1.5.1&search=format_code_in_doc_comments#format_code_in_doc_comments

Don't check this option into `rustfmt.toml` because it has some bugs
with how it formats comments. See:

- rust-lang/rustfmt#5568
@ytmimi
Copy link
Contributor

ytmimi commented Oct 22, 2022

@lopopolo thanks for the report.

I'm having a hard time figuring out what's going on from the example you posted. Could you please provide a minimal reproducible example without the diff info, thanks!

@ytmimi ytmimi added needs-mcve needs a Minimal Complete and Verifiable Example only-with-option requires a non-default option value to reproduce labels Oct 22, 2022
@lopopolo
Copy link
Author

Hi @ytmimi, here's a example project that shows the problem. The bad formatting happens in build.rs. This project doesn't compile most likely, but has the rustfmt problem.

The archive is a git repository with a couple of commits, the last one showing the bad formatting.

rustdoc-comment-example.zip

@ytmimi
Copy link
Contributor

ytmimi commented Oct 22, 2022

Thanks, but Ideally we'd like to not work with an arbitrary zip. An inline code example would be best if you have time to put a code example together.

@lopopolo
Copy link
Author

lopopolo commented Oct 22, 2022

The zip I provided I thought was the minimized example 😅 . It was an empty cargo project with the rustfmt.toml and build.rs copied in. Here it is minimized further and posted inline.

rustfmt.toml

# Artichoke crates always use the 2021 edition.
edition = "2021"

hard_tabs = false
newline_style = "Unix"

# Wider width lines and less-aggressive line breaking.
max_width = 119
use_small_heuristics = "Default"

use_field_init_shorthand = true
use_try_shorthand = true

# nightly only option
group_imports = "StdExternalCrate"
format_code_in_doc_comments = true

build.rs

Before formatting

#![warn(clippy::all)]
#![warn(clippy::pedantic)]
#![allow(clippy::restriction)]

mod libs {
    fn mrbgems_sources() -> impl Iterator<Item = PathBuf> {
        [
            "mrbgems/mruby-class-ext/src/class.c",   // NOTE(GH-32): Pending removal.
            "mrbgems/mruby-compiler/core/codegen.c", // Ruby parser and bytecode generation
            "mrbgems/mruby-compiler/core/y.tab.c",   // Ruby parser and bytecode generation
            "mrbgems/mruby-error/src/exception.c",   // `mrb_raise`, `mrb_protect`
            "mrbgems/mruby-eval/src/eval.c",         // eval, instance_eval, and friends
            "mrbgems/mruby-fiber/src/fiber.c",       // Fiber class from core, required by `Enumerator`
            "mrbgems/mruby-metaprog/src/metaprog.c", // APIs on Kernel and Module for accessing classes and variables
            "mrbgems/mruby-method/src/method.c",     // `Method`, `UnboundMethod`, and method APIs on Kernel and Module
            "mrbgems/mruby-pack/src/pack.c",         // Array#pack and String#unpack
            "mrbgems/mruby-proc-ext/src/proc.c",     // NOTE(GH-32): This gem is required by `mruby-method`.
            "mrbgems/mruby-sprintf/src/sprintf.c",   // Kernel#sprintf, Kernel#format, String#%
        ]
        .into_iter()
        .map(|source| paths::mruby_root().join(source))
        .chain(
            ["src/gem_init.c", "src/mrbgems.c"]
                .into_iter()
                .map(|source| paths::mrbgems_root().join(source)),
        )
    }

    fn mrbgems_include_dirs() -> impl Iterator<Item = PathBuf> {
        [
            "mrbgems/mruby-class-ext/include", // NOTE(GH-32): Pending removal.
            "mrbgems/mruby-compiler/core",     // Ruby parser and bytecode generation
            "mrbgems/mruby-error/include",     // `mrb_raise`, `mrb_protect`
            "mrbgems/mruby-eval/include",      // eval, instance_eval, and friends
            "mrbgems/mruby-fiber/include",     // Fiber class from core, required by `Enumerator`
            "mrbgems/mruby-metaprog/include",  // APIs on Kernel and Module for accessing classes and variables
            "mrbgems/mruby-method/include",    // `Method`, `UnboundMethod`, and method APIs on Kernel and Module
            "mrbgems/mruby-pack/include",      // Array#pack and String#unpack
            "mrbgems/mruby-proc-ext/include",  // NOTE(GH-32): This gem is required by `mruby-method`.
            "mrbgems/mruby-sprintf/include",   // Kernel#sprintf, Kernel#format, String#%
        ]
        .into_iter()
        .map(|dir| paths::mruby_root().join(dir))
        .chain(mruby_include_dirs())
    }
}

After formatting

#![warn(clippy::all)]
#![warn(clippy::pedantic)]
#![allow(clippy::restriction)]

mod libs {
    fn mrbgems_sources() -> impl Iterator<Item = PathBuf> {
        [
            "mrbgems/mruby-class-ext/src/class.c",   // NOTE(GH-32): Pending removal.
            "mrbgems/mruby-compiler/core/codegen.c", // Ruby parser and bytecode generation
            "mrbgems/mruby-compiler/core/y.tab.c",   // Ruby parser and bytecode generation
            "mrbgems/mruby-error/src/exception.c",   // `mrb_raise`, `mrb_protect`
            "mrbgems/mruby-eval/src/eval.c",         // eval, instance_eval, and friends
            "mrbgems/mruby-fiber/src/fiber.c",       // Fiber class from core, required by `Enumerator`
            "mrbgems/mruby-metaprog/src/metaprog.c", // APIs on Kernel and Module for accessing classes and variables
            "mrbgems/mruby-method/src/method.c", // `Method`, `UnboundMethod`, and method APIs on Kernel and Module
            "mrbgems/mruby-pack/src/pack.c",     // Array#pack and String#unpack
            "mrbgems/mruby-proc-ext/src/proc.c", // NOTE(GH-32): This gem is required by `mruby-method`.
            "mrbgems/mruby-sprintf/src/sprintf.c", // Kernel#sprintf, Kernel#format, String#%
        ]
        .into_iter()
        .map(|source| paths::mruby_root().join(source))
        .chain(
            ["src/gem_init.c", "src/mrbgems.c"]
                .into_iter()
                .map(|source| paths::mrbgems_root().join(source)),
        )
    }

    fn mrbgems_include_dirs() -> impl Iterator<Item = PathBuf> {
        [
            "mrbgems/mruby-class-ext/include", // NOTE(GH-32): Pending removal.
            "mrbgems/mruby-compiler/core",     // Ruby parser and bytecode generation
            "mrbgems/mruby-error/include",     // `mrb_raise`, `mrb_protect`
            "mrbgems/mruby-eval/include",      // eval, instance_eval, and friends
            "mrbgems/mruby-fiber/include",     // Fiber class from core, required by `Enumerator`
            "mrbgems/mruby-metaprog/include",  // APIs on Kernel and Module for accessing classes and variables
            "mrbgems/mruby-method/include",    // `Method`, `UnboundMethod`, and method APIs on Kernel and Module
            "mrbgems/mruby-pack/include",      // Array#pack and String#unpack
            "mrbgems/mruby-proc-ext/include",  // NOTE(GH-32): This gem is required by `mruby-method`.
            "mrbgems/mruby-sprintf/include",   // Kernel#sprintf, Kernel#format, String#%
        ]
        .into_iter()
        .map(|dir| paths::mruby_root().join(dir))
        .chain(mruby_include_dirs())
    }
}

@ytmimi
Copy link
Contributor

ytmimi commented Oct 22, 2022

Thank you. Having the inline code snippet will help us dig into what's happening here

@ytmimi ytmimi added a-comments and removed needs-mcve needs a Minimal Complete and Verifiable Example labels Oct 22, 2022
@ytmimi
Copy link
Contributor

ytmimi commented Oct 25, 2022

Here's a slightly smaller code snippet that reproduces the issuse:

Input

mod libs {
    fn mrbgems_sources() {
        [
            "mrbgems/mruby-compiler/core/codegen.c", // Ruby parser and bytecode generation
            "mrbgems/mruby-compiler/core/y.tab.c",   // Ruby parser and bytecode generation
            "mrbgems/mruby-metaprog/src/metaprog.c", // APIs on Kernel and Module for accessing classes and variables
            "mrbgems/mruby-method/src/method.c",     // `Method`, `UnboundMethod`, and method APIs on Kernel and Module
            "mrbgems/mruby-pack/src/pack.c",         // Array#pack and String#unpack
        ]
    }
}

output (--config=max_width=119) Note the output is unchanged

mod libs {
    fn mrbgems_sources() {
        [
            "mrbgems/mruby-compiler/core/codegen.c", // Ruby parser and bytecode generation
            "mrbgems/mruby-compiler/core/y.tab.c",   // Ruby parser and bytecode generation
            "mrbgems/mruby-metaprog/src/metaprog.c", // APIs on Kernel and Module for accessing classes and variables
            "mrbgems/mruby-method/src/method.c",     // `Method`, `UnboundMethod`, and method APIs on Kernel and Module
            "mrbgems/mruby-pack/src/pack.c",         // Array#pack and String#unpack
        ]
    }
}

output (--config=max_width=119,format_code_in_doc_comments=true)

mod libs {
    fn mrbgems_sources() {
        [
            "mrbgems/mruby-compiler/core/codegen.c", // Ruby parser and bytecode generation
            "mrbgems/mruby-compiler/core/y.tab.c",   // Ruby parser and bytecode generation
            "mrbgems/mruby-metaprog/src/metaprog.c", // APIs on Kernel and Module for accessing classes and variables
            "mrbgems/mruby-method/src/method.c", // `Method`, `UnboundMethod`, and method APIs on Kernel and Module
            "mrbgems/mruby-pack/src/pack.c",     // Array#pack and String#unpack
        ]
    }
}

@ytmimi
Copy link
Contributor

ytmimi commented Oct 25, 2022

Also was looking into this and it seems #5536 resolves this issue

@calebcartwright
Copy link
Member

Is this not, at least in part, related to rustfmt's current behavior of speciously trying to vertically/visually align trailing comments in list contexts (#4108)? I'd have expected this behavior to be reproducible independently of format_code_in_doc_comments and it gives me some pause that another PR may be modifying that behavior (however buggy it is, any changes to it would potentially be stability breaking)

@ytmimi
Copy link
Contributor

ytmimi commented Sep 19, 2023

#4108 is still reproducible. I think this issue is related, but I think the format_code_in_doc_comments issue is a distinct case of that problem. I have a feeling that there's some odd interaction with max_width and format_code_in_doc_comments, which is side stepped by #5536.

In the example above we could maintain the alignment by bumping the max_width by 1 to 120. I tested this rustfmt 1.5.3-nightly (f4b80cac 2023-06-30).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-comments only-with-option requires a non-default option value to reproduce
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants