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

Tuple struct field comments consume the field itself, only with doc comment #5215

Closed
danjl1100 opened this issue Feb 3, 2022 · 4 comments · Fixed by #5217
Closed

Tuple struct field comments consume the field itself, only with doc comment #5215

danjl1100 opened this issue Feb 3, 2022 · 4 comments · Fixed by #5217
Assignees
Labels
a-comments bug Panic, non-idempotency, invalid code, etc. help wanted

Comments

@danjl1100
Copy link

danjl1100 commented Feb 3, 2022

Describe the bug

When using the sequence /// doc comment, // regular comment, <field definition> within a tuple struct definition, rustfmt joins the field with the regular comment above, removing the field from the struct definition.

Input

struct MyTuple(
    /// Doc Comments
    // TODO note to add more to Doc Comments
    u32,
);

Output - correctness issue

struct MyTuple(
    /// Doc Comments
    // TODO note to add more to Doc Commentsu32,
);

The same applies for /* block comments */, but in this case the issue is purely cosmetic (does not change the struct definition).

Input (block comment)

struct MyTuple(
    /// Doc Comments
    /* TODO note to add more to Doc Comments */
    u32,
);

Output (block comment) - cosmetic issue

struct MyTuple(
    /// Doc Comments
    /* TODO note to add more to Doc Comments */u32,
);

At the very least, I expect there to be a separating space between the block comment and the u32, similar to the similar example with no doc comment

-    /* TODO note to add more to Doc Comments */u32,
+    /* TODO note to add more to Doc Comments */ u32,

Working cases

However, this issue doesn't seem to appear when the /// doc comment is removed, nor in named-field structs:

  • No doc comment - works as expected
    struct MyTuple(
        // TODO note to add more to Doc Comments
        u32,
    );
    
  • Struct with named fields (non-tuple) - works as expected
    struct MyStruct {
        /// Doc Comments
        // TODO note to add more to Doc Comments
        field: u32,
    }
    

To Reproduce

  • Tuple struct with doc comment, single line comment (broken) - Playground
  • Tuple struct with doc comment, block comment (broken, cosmetic only) - Playground
  • Tuple struct with single line comment, only (not broken, just for comparison) - Playground
  • Struct (named fields) with doc comment, single line comment (not broken, just for comparison) - Playground

Expected behavior

I expect rustfmt to never change the tuple struct definition (MyStruct(u32) --> MyStruct()), and also preserve cosmetic newlines in the block comment case.

Meta

  • rustfmt version: rustfmt 1.4.38-nightly (2022-02-02 27f5d83)
  • From where did you install rustfmt?: using playground only
  • How do you run rustfmt: using playground only

Additional remarks

@ytmimi
Copy link
Contributor

ytmimi commented Feb 3, 2022

Thank you for the report!

There is a lot of great detail here! I appreciate how thorough the report is. I can confirm that I'm able to reproduce this on the current master rustfmt 1.4.38-nightly (8b0b213c 2022-01-29).

@ytmimi ytmimi added a-comments bug Panic, non-idempotency, invalid code, etc. labels Feb 3, 2022
@ytmimi
Copy link
Contributor

ytmimi commented Feb 4, 2022

Did some digging into this. The issue is that we're making the assumption that the type, prefix, and attributes can all fit on a single line if the type is written on a single line. As demonstrated by this issue, we should also consider whether or not a comment was found between the attribute and the prefix.

If anyone's interested in taking this on this issue on they should look at rewrite_struct_field, but specifically these lines:

rustfmt/src/items.rs

Lines 1735 to 1739 in 368a9b7

if let Some(ref ty) = orig_ty {
if !ty.contains('\n') {
return Some(attr_prefix + &spacing + ty);
}
}

@tharun208
Copy link
Contributor

@ytmimi I like to work on this

tharun208 added a commit to tharun208/rustfmt that referenced this issue Feb 4, 2022
@tharun208
Copy link
Contributor

@rustbot claim

tharun208 added a commit to tharun208/rustfmt that referenced this issue Feb 4, 2022
tharun208 added a commit to tharun208/rustfmt that referenced this issue Feb 4, 2022
calebcartwright pushed a commit that referenced this issue Apr 2, 2022
…5217)

* fix(rustfmt): fix struct field formatting with doc comments present

Fixes #5215

* fix review feedbacks

* add unit test without doc comment

* move tests to a seperate file

* add additional test cases

* reintroduce a newline at the of test/souce/structs.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-comments bug Panic, non-idempotency, invalid code, etc. help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants