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

SWC - Update to 2.3.x causes incorrect output #365

Closed
MolotovCherry opened this issue Jun 21, 2023 · 9 comments · Fixed by #366
Closed

SWC - Update to 2.3.x causes incorrect output #365

MolotovCherry opened this issue Jun 21, 2023 · 9 comments · Fixed by #366

Comments

@MolotovCherry
Copy link

MolotovCherry commented Jun 21, 2023

Linking from this issue here -
swc-project/swc#7513

There was a problem in SWC where commas were being omitted in the output. It was determined that this was caused by bitflags. Something changed between 2.2.1 to 2.3.x which broke SWC's output.

(I'm not the maintainer of SWC, so I can't offer any input on details on SWC's usage of bitflags, or what/how/why anything works/doesn't work)

Anyways, as of this filing date, all users who start new SWC projects (because they aren't cargo locked to an older version of bitflags) will get broken output due to this.

Edit: I have a reproducible repo here. You can verify downgrading bitflags fixes it with cargo update -p bitflags@2.3.1 --precise 2.2.1

@KodrAus
Copy link
Member

KodrAus commented Jun 22, 2023

Thanks for the report @MolotovCherry. I’ll check this out. I wouldn’t expect things to break between minor releases unless there’s some reliance on specific debug formatting.

@kosayoda
Copy link

I found the root cause of the bug, and it's not an issue with bitflags, rather swc doing something unsanctioned that just happened to work for about 5 years until it didn't because of a change in implementation detail.

I'll write up the issue (or a PR) with details on the swc repo and link here.

@KodrAus
Copy link
Member

KodrAus commented Jun 22, 2023

Thanks for the update @kosayoda. Those are my favourite kinds of bugs 😄

I’ve been on holidays this week so haven’t had a chance to dig into this yet.

@KodrAus
Copy link
Member

KodrAus commented Jun 23, 2023

Following the linked PR it looks like you might have run into this issue.

For what it’s worth, I think that is a bug in bitflags and am currently working through revamping our test suite to better specify the behaviour of methods when there are bits set that don’t correspond to known flags.

@kosayoda
Copy link

Ah that does make sense. I think it boils down to whether adding new flags external to the bitflags macro ("external API" use case in the linked issue) should be sanctioned, which on first instinct I don't think it should due to the amount of edge cases in the implementation details.

@KodrAus
Copy link
Member

KodrAus commented Jun 24, 2023

I think where possible we’ll try respect bits that don’t correspond to known flags. So if the bitwise operation on the underlying integer would retain that bit then the operation on the flags type will too.

The only case that gets a bit weird I think is !, where we don’t want to set all unset bits, so it just unconditionally clears any bits that don’t correspond to known flags.

@KodrAus
Copy link
Member

KodrAus commented Jun 27, 2023

I've opened #366 to fix this, and think we've hashed out a plan for a 3.x release that should make these operators fully consistent in their support flags types with externally defined flags.

If you've got any thoughts on that I'd love to hear them!

@KodrAus
Copy link
Member

KodrAus commented Jun 27, 2023

The original bug here should be fixed as of 2.3.3.

@MolotovCherry
Copy link
Author

This is great. I tested 2.3.3 with the original example which had the problem, and it now shows the correct output. Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants