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 duplicates rustfmt::skip #4282

Closed
RalfJung opened this issue Jun 27, 2020 · 4 comments
Closed

rustfmt duplicates rustfmt::skip #4282

RalfJung opened this issue Jun 27, 2020 · 4 comments
Labels
a-rustfmt::skip bug Panic, non-idempotency, invalid code, etc. fixed

Comments

@RalfJung
Copy link
Member

RalfJung commented Jun 27, 2020

When formatting the following code with nightly rustfmt:

fn test(x: &str) {
    // Then handle terminating intrinsics.
    match intrinsic_name {
        // Raw memory accesses
        #[rustfmt::skip]
        | "copy"
        | "copy_nonoverlapping"
        => {
        }
    }
}

The result is that #[rustfmt::skip] gets duplicated.

(We had this problem before and it got fixed, but now it seems it came back: #3974.)

$ rustfmt --version
rustfmt 1.4.18-nightly (c1e9b7b 2020-06-13)
@calebcartwright calebcartwright added a-rustfmt::skip bug Panic, non-idempotency, invalid code, etc. good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted labels Jun 27, 2020
@calebcartwright
Copy link
Member

@topecongiro - feel like we should get this fixed before the 2.0 release so I'm adding to the 2.0 miletsone. let me know if you disagree though!

@calebcartwright calebcartwright modified the milestone: 2.0.0 Jun 27, 2020
@calebcartwright calebcartwright added fixed and removed good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted labels Jun 27, 2020
@calebcartwright
Copy link
Member

Just kidding 😄 this has already been fixed in source and cannot be reproduced on master/rustfmt 2.0

We're not planning on having any more rustfmt 1.x releases, but if we do end up having to do so then we'll backport the fix for this from #3975.

Going to go ahead and close this for now

@RalfJung
Copy link
Member Author

So given that #3974 was closed more than half a year before the fix is actually shipped to users -- maybe it would make sense to keep this one open until the fix is really released (at least on the nightly channel)?

@calebcartwright
Copy link
Member

calebcartwright commented Jun 27, 2020

maybe it would make sense to keep this one open until the fix is really released (at least on the nightly channel)?

It's the practice within rustfmt, and has been my experience in other projects as well, for issues to be closed when the corresponding code changes are merged vs. waiting to close til after a deployment/release.

However, we've been in such a bifurcated development mode between rustfmt 1.x and 2.x for so long that I understand how the actual status of an issue can be confusing and complicated (particularly for those fixed/resolved shortly after 2.x dev started). We've had a lot of rustfmt 1.x releases during that time, but unfortunately a lot of the bugs fixed in source/master just didn't get backported.

As such, I've gone ahead and opened #4283 to ensure that this fix will be in the next version of rustfmt no matter what. Accordingly I'm going to keep this closed given our issue management strategy and this issue being fixed in source.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-rustfmt::skip bug Panic, non-idempotency, invalid code, etc. fixed
Projects
None yet
Development

No branches or pull requests

2 participants