no_magic_number - Ignore numbers involved in bit-wise operations (like << or >> or | or &)

[LowAmmo]

New Issue Checklist

• Updated SwiftLint to the latest version
• I searched for existing GitHub issues

New rule request

(I debated if this is technically a bug or request, but it's definitely not a "new" rule, more of just improving an existing rule.)

What I'm running into is the "no_magic_number" rule triggering on bitwise structs or enums that we have in our code -

struct LogType: OptionSet {
    /// Returns the raw bitmask value of the option and satisfies the `RawRepresentable` protocol.
    public let rawValue: UInt

    /// Logging to the operating system
    public static let operatingSystem: LogType= LogType(rawValue: 1 << 0)

    /// Logging to the debug console
    public static let debugConsole: LogType= LogType(rawValue: 1 << 1)

    /// Logging to a file
    public static let file: LogType= LogType(rawValue: 1 << 2) // swiftlint:disable:this no_magic_numbers

    /// Storing Logs in memory
    public static let inMemory: LogType= LogType(rawValue: 1 << 3) // swiftlint:disable:this no_magic_numbers

    /// Testing logger
    public static let testing: LogType= ulogLogType(rawValue: 1 << 4) // swiftlint:disable:this no_magic_numbers

    /// Visually presents logs
    public static let visual: LogType= LogType(rawValue: 1 << 5) // swiftlint:disable:this no_magic_numbers

    /// The log event is reporting to another delegate/system
    public static let reporting: LogType= LogType(rawValue: 1 << 6) // swiftlint:disable:this no_magic_numbers

    /// Other (bucket for any 'other' type of logging not applicable to any of the existing types
    public static let other: LogType= LogType(rawValue: 1 << 7) // swiftlint:disable:this no_magic_numbers

    /**
     Creates a new instance of the options set from the specified raw value
     */
    public init(rawValue: UInt) {
        self.rawValue = rawValue
    }
}

Obviously, the bit-wise left shift operators appear to have "magic numbers", but being part of a left (or right shift) operation kind of excludes it from being "magic".

To make it helpful, could enforce that the number is less than 32 (or 64) (or whatever number of bits are in that specified storage type), depending on how smart the rule can be.

An additional idea would be to just make it a configuration point on the no_magic_number rule. Along the lines of "ignore_bitwise_operations: true"

Hopefully that makes sense. As you can see from the code sample...it's not the hardest thing in the world to just disable the rule for the lines where it applies. But it makes the code messier than it needs to be for cases that (I think) are universally okay for having "magic numbers" (as long as they are less than 64).

[mildm8nnered]

So I think excluding 1 << 2 and friends by default is ok - I don't think we'd even need a configuration parameter, because I would count those as "always ok".

Do we need to worry about the other bitwise operators? I think << is by far the most common case.

[SimplyDanny]

Do we need to worry about the other bitwise operators? I think << is by far the most common case.

I'd agree that << can definitely be ignored. For all other bitwise operators, one should rather use defined constant instead of raw literals, isn't it? Thus, the rule triggering for these operators is okay from my point of view.

[LowAmmo]

@SimplyDanny - Very much agree that the most common use case if for bit shifting (<<).

It does feel weird to declare a variable something like "let the4thBit = 8" or "let allBitsFor8Bit = 255".

But, I'm also seeing that binary notation (0b11111111) apparently does NOT trigger the no_magic_number, so that's probably a reasonable way to handle the bit-wise & or |.

[LowAmmo]

@mildm8nnered - Thanks for getting out a PR so quickly!

Any reason you think we shouldn't do the right shift (>>) also?

I'm assuming @SimplyDanny mentioned left shift "<<" but that he meant left or right shift are both totally okay...?? (but...maybe I misunderstood).

-Thanks!

[mildm8nnered]

Actually, looking through my codebase, I have some >> cases, so I'll add that in as well.

[LowAmmo]

Thanks, @mildm8nnered!

[SimplyDanny]

Actually, looking through my codebase, I have some >> cases, so I'll add that in as well.

Could you list a few (obfuscated) examples here, @mildm8nnered? I try to understand if the >> cases are really always so clear to understand with raw literals.

I understand that the rule is annoying for the << cases which are typically used to define options and the like. But when it comes to & and | it's better to have their operands named. For >>, I'm lacking some good examples apart from bare computations which would be rather uncommon in Swift code. And if they are relevant, one would probably disable the whole no_magic_numbers globally, for a complete file or a section in a file.

[mildm8nnered]

extension UInt32 {

    // From https://gist.github.com/bpolania/704901156020944d3e20fef515e73d61
    var data: Data { // swiftlint:disable:this unused_declaration
        var int = self
        return Data(bytes: &int, count: MemoryLayout<UInt32>.size)
    }

    var byteArrayLittleEndian: [UInt8] {
        return [
            UInt8((self & 0xFF000000) >> 24),
            UInt8((self & 0x00FF0000) >> 16),
            UInt8((self & 0x0000FF00) >> 8),
            UInt8(self & 0x000000FF),
        ]
    }
}

[mildm8nnered]

I think the above example will actually trigger on the 0xFF000000, but I have no_magic_numbers off in the codebase this is in.

[SimplyDanny]

extension UInt32 {

// From https://gist.github.com/bpolania/704901156020944d3e20fef515e73d61
var data: Data { // swiftlint:disable:this unused_declaration
    var int = self
    return Data(bytes: &int, count: MemoryLayout<UInt32>.size)
}

var byteArrayLittleEndian: [UInt8] {
    return [
        UInt8((self & 0xFF000000) >> 24),
        UInt8((self & 0x00FF0000) >> 16),
        UInt8((self & 0x0000FF00) >> 8),
        UInt8(self & 0x000000FF),
    ]
}

}

This could be an example for the "bare computations" I mentioned above. I wonder if this is really so common that it justifies the rule to ignore it completely. In the example, one could also comfortably live with a disable command I guess.

Right now, I see a few possibilities to let the rule handle magic operands of bit-wise operators by some means:

1. Ignore all magic operands in all these operators.
2. Ignore only the right operands of shift operators.
3. Ignore only the right operand of <<.
4. Protect some or all of these cases behind configuration options.

I'm not yet sure what's the right way to go.

[LowAmmo]

Probably obvious... But my vote would be for option 1 or 2...

I feel like the heart or idea behind not having magic numbers is that you don't want to be reading through code and just see a "42" somewhere, with no justification for what it's doing or why.

But (at least for me), shift operations tend to be a block of code where it's obvious what is going on (usually setting up values for an enum, or performing an encoding/decoding type of operation.

So, even if someone says that these are just "magic numbers", I think the heart of the rule is still preserved by make these changes.

-Thanks!

[SimplyDanny]

Makes sense. I think you convinced me. 😅 In case of complaints, we may hide this behavior behind an option later.