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

refactor(common): Remove add_bitflags and update bitflags #7571

Merged
merged 2 commits into from
Jun 23, 2023

Conversation

kosayoda
Copy link
Contributor

The add_flags macro makes impl blocks for the bitflags struct separately from the one generated by bitflags. This is incorrect because the bitflags implementation uses the listed constants to generate certain method bodies.

I opted to manually specify the bitflags macro instead of modifying the add_flags macro to remove complexity (the bitflags structs are not changed often).

The PR also bumps the version of bitflags to 2.3.2 to ensure that the added tests #7542, #7545 still pass.

Fixes: #7513
Related: bitflags/bitflags#365

Details

In the linked issue, commas were not added in the minified codegen because this line:

if !emit_new_line {
list_format -= ListFormat::MultiLine | ListFormat::Indented;
}

Causes list_format to be set to the empty ListFormat due to the SubAssign implementation.

  • Bitflags 2.2.x
    The implementation is like this:
     ListFormat SubAssign -> InternalBitFlags difference() -> u32 BitAnd + u32 Not
    
  • Bitflags 2.3.x
    The implementation is like this:
     ListFormat SubAssign -> InternalBitFlags BitAnd -> u32 BitAnd
                          -> InternalBitFlags BitNot -> InternalBitFlags complement() -> InternalBitFlags from_bits_truncate() + u32 Not
    

The key is the change to use from_bits_truncate. This code:

bitflags::bitflags! {
    #[derive(Clone, Copy)]
    struct Flags: u32 {
        const A = 0b00000001;
        const B = 0b00000010;
        const C = 0b00000100;
    }
}

generates this output:

#[inline]
pub const fn from_bits_truncate(bits: u32) -> Self {
    let bits = bits;
    {
        if bits == <u32 as ::bitflags::Bits>::EMPTY {
            return Self(bits);
        }
        let mut truncated = <u32 as ::bitflags::Bits>::EMPTY;
        // IMPORTANT!
        {
            if bits & Flags::A.bits() == Flags::A.bits() {
                truncated = truncated | Flags::A.bits();
            }
        };
        {
            if bits & Flags::B.bits() == Flags::B.bits() {
                truncated = truncated | Flags::B.bits();
            }
        };
        {
            if bits & Flags::C.bits() == Flags::C.bits() {
                truncated = truncated | Flags::C.bits();
            }
        };
        // IMPORTANT!
        Self(truncated)
    }
}

Using the add_bitflags! macro in swc_common, the flags are manually added to the struct after bitflags code generation, so the middle parts are not generated, eventually causing SubAssign to return an empty flag struct.

The macro makes `impl` blocks for the bitflags struct separately from
the one generated by `bitflags`. This is incorrect because the `bitflags`
implementation uses the listed constants to generate certain method
bodies.
This allows us to surface any incorrect bitflags usage based on tests.
@CLAassistant
Copy link

CLAassistant commented Jun 23, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!


swc-bump:

  • swc_common

@kdy1 kdy1 changed the title Fix incorrect struct generation with bitflags 2.3.x. refactor(common): Remove add_bitflags and update bitflags Jun 23, 2023
@kdy1 kdy1 merged commit 95ac74e into swc-project:main Jun 23, 2023
126 checks passed
@kdy1 kdy1 added this to the Planned milestone Jun 23, 2023
@Swatinem
Copy link

FYI: this should have been a semver-breaking release for swc_common, as it is removing add_bitflags which was used by some older versions of dependent swc crates.

I believe this broke us downstream. No big deal, bumping to the very latest versions of the swc crates was long overdue: getsentry/js-source-scopes#22

@kdy1
Copy link
Member

kdy1 commented Jun 23, 2023

I know, but I decided I don't care.

@kdy1 kdy1 modified the milestones: Planned, v1.3.67 Jun 29, 2023
@swc-project swc-project locked as resolved and limited conversation to collaborators Jul 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Syntax error: Minifying causes commas to be missing between object properties
4 participants