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

Rustfmt removes comments in uses #3984

Closed
Tracked by #4991
WaffleLapkin opened this issue Dec 26, 2019 · 12 comments
Closed
Tracked by #4991

Rustfmt removes comments in uses #3984

WaffleLapkin opened this issue Dec 26, 2019 · 12 comments
Labels
a-comments a-imports `use` syntax bug Panic, non-idempotency, invalid code, etc.

Comments

@WaffleLapkin
Copy link
Member

Rustfmt sometimes removes comments in uses with {}. For example (diffs from cargo fmt -- --check):

-use a::{item /* comment */};
+use a::item;

With merge_imports = true:

-use a::{
-    x,
-    // comment
-    item,
-};
+use a::{item, x};
@WaffleLapkin
Copy link
Member Author

Another interesting case:

-use b::{
-    x,
-    y,
-};
-use a::item; // comment
+use a::item;
+use b::{x, y}; // comment

// comment moved to other use

@WaffleLapkin
Copy link
Member Author

I've just found new variant:

-use b::{
-    x,
-    y,
-};
-use a::item /* comment */;
+use a::item;
+use b::{x, y};

Sounds like there are some problems with formatting comments in uses :D

@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Dec 26, 2019

Should I open a separate issue for this?

Wrong indent:

-use a::item; // really long comment (with `use` exactly 100 characters) ____________________________
+use a::item; /* really long comment (with `use` exactly 100 characters)
+               * ____________________________ */
 use b::{x, y};

(is not reproducable with rustfmt 1.4.12-nightly (9f53665 2020-02-10))

@calebcartwright
Copy link
Member

@WaffleLapkin - what version of rustfmt are you using, and do you have a rustfmt config file/are you passing any config options via the cli?

@WaffleLapkin
Copy link
Member Author

WaffleLapkin commented Dec 26, 2019

@calebcartwright version:

% cargo +nightly fmt -- --version
rustfmt 1.4.11-nightly (1838235 2019-12-03)

I don't have rustfmt config nor I pass config options via the cli, except the second example where I have rustfmt.toml with the following line:

merge_imports = true

@calebcartwright
Copy link
Member

Thanks! I can't reproduce your last example with the latest version of rustfmt on the master branch (which contains a lot of unreleased changes), but the others are still all reproducible

@topecongiro topecongiro added a-comments a-imports `use` syntax bug Panic, non-idempotency, invalid code, etc. labels Dec 27, 2019
@MarcoIeni
Copy link

MarcoIeni commented Feb 22, 2020

I worked on this with wise86-android.

We noticed that when the use is in the form use a::{...}, comments are handled correctly.
However, there are problems when there are not curly braces.
For example, use a::item /* comment */; is converted to use a::item; // comment */;, while what we would expect is that it remains unchanged or something like use a::item; // comment.

We see that the comment transformation is done here:

pub(crate) fn rewrite_comment(
orig: &str,
block_style: bool,
shape: Shape,
config: &Config,
) -> Option<String> {
identify_comment(orig, block_style, shape, config, false)
}

In the case above,/* comment1 */; is the orig parameter and// comment1 */; is returned.

Maybe the problem is that the orig parameter has a trailing ;

@sharnoff
Copy link

sharnoff commented Sep 11, 2020

@calebcartwright It seems like the case fixed by PR #3999 does not currently work. I was a little surprised by this - given that there's a test case here, but (I would assume) the playground does not lie. The following example (playground link) exhibits the old behavior:

-use std::foo::{/* it's a comment! */ bar /* and another */};
+use std::foo::bar;

// filler

-use std::foo::{/* it's a comment! */ bar};
+use std::foo::bar;

// filler

-use std::foo::{bar /* and another */};
+use std::foo::bar;

I noticed this occuring with rustfmt 1.4.18-stable, and (per the playground) it seems to still be there in 1.14.21-nightly (2020-09-04 01f2ead). As a side note, it still occurs with just std::{ ... } -- there's no need for two leading path segments for this to happen.

@calebcartwright
Copy link
Member

calebcartwright commented Sep 11, 2020

@sharnoff - note that the referenced PR was merged to the master branch, so it's included in the unreleased rustfmt v2.0, but not in the released 1.x rustfmt versions you referenced

@sharnoff
Copy link

@calebcartwright Ah, that makes a bunch of sense! Thanks :)

@ytmimi
Copy link
Contributor

ytmimi commented Jul 21, 2022

Another interesting case:

-use b::{
-    x,
-    y,
-};
-use a::item; // comment
+use a::item;
+use b::{x, y}; // comment

// comment moved to other use

This issue is a duplicate of #3720

@ytmimi
Copy link
Contributor

ytmimi commented Aug 31, 2023

Now that #5853 is merged comments within uses are preserved. As noted, this example #3984 (comment) is a duplicate of another issue, so I'm going to close this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-comments a-imports `use` syntax bug Panic, non-idempotency, invalid code, etc.
Projects
None yet
Development

No branches or pull requests

6 participants