New lint: decimal_bit_mask

[Artur-Sulej]

Add a new lint that detects the use of decimal literals as bit masks in bitwise operations. Using decimal literals for bit masks can obscure the intended bit pattern and reduce code readability. This lint encourages the use of binary (0b...) or hexadecimal (0x...) notation to make bit patterns explicit and easier to understand at a glance.

Example:

let masked = x & 14; // Bad: decimal literal as bit mask
let masked = x & 0b1110; // Good: bit pattern is explicit

changelog: [decimal_bit_mask]: new lint

Fixes #1775

[github-actions]

Lintcheck changes for 4dca87e

Lint	Added	Removed	Changed
clippy::decimal_bit_mask	35	0	0

This comment will be updated if you push new changes

[Artur-Sulej]

Although I annotated my test with //~ decimal_bit_mask, Clippy still reports the lint as an unmatched diagnostic and the test fails.
Any idea what I might be missing?

[rustbot]

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[ada4a]

Although I annotated my test with //~ decimal_bit_mask, Clippy still reports the lint as an unmatched diagnostic and the test fails.
Any idea what I might be missing?

That's because in this case, you're firing the lint twice, once for each operand, so you'll need to add a second error mark. That's the easiest to do by bringing them to under the linted expression:

Suggested change

	88 & 99; //~ decimal_bit_mask
	88 & 99;
	//~^ decimal_bit_mask
	//~| decimal_bit_mask

• ^ means the lint should happen on the line above the comment
• | "attaches" the error mark to another one on a neighbouring line (in this case the //~^ decimal_bit_mask directly above), and will expect a lint at the same location (in this case, the line containing 88 & 99;)

[ada4a]

It would be a bit more concise to have these simply as left and right, and then use left.span, right.kind etc. throughout the code

[ada4a]

You can improve the output by pointing the lint at the exact operand that is decimal -- for example, here you would use span1 (or left.span, as per previous comment)

[ada4a]

The second sentence should be emitted separately, as a help message -- see https://doc.rust-lang.org/clippy/development/emitting_lints.html#emitting-a-lint-1 for more info.

Try using span_lint_and_help here

[ada4a]

You can use SpanRangeExt::get_source_code here to avoid allocating a String (which is what snippet_opt does)

                && let Some(snippet) = span2.get_source_text(cx)

In fact, since you're only using the snippet to check its contents, you can get away with SpanRangeExt::check_source_code:

Suggested change

	&& let Some(snippet) = snippet_opt(cx, *span2)
	&& !snippet.starts_with("0b")
	&& !snippet.starts_with("0x")
	&& span2.check_source_text(cx, |src| !src.starts_with("0b") && !src.starts_with("0x"))

[ada4a]

@rustbot note remove Feature-freeze

[ada4a]

Could you please change the PR description to something like this?

"""
Add a new lint that detects the use of decimal literals as bit masks in bitwise operations. Using decimal literals for bit masks can obscure the intended bit pattern and reduce code readability. This lint encourages the use of binary (0b...) or hexadecimal (0x...) notation to make bit patterns explicit and easier to understand at a glance.

Example:

let masked = x & 15; // Bad: decimal literal as bit mask
let masked = x & 0b1111; // Good: bit pattern is explicit

changelog: [decimal_bit_mask]: new lint

Fixes #1775
"""

Motivation for the changes:

• the changelog message should fit on one line
• by using "Fixes", you make it so that when this PR is merged, the corresponding issue is automatically closed

[ada4a]

If you look at Lintcheck results (displayed over at #15215 (comment)), you can see a couple of problems with the current implementation:

warning: Using decimal literal for bit mask. Consider using binary (0b...) or hexadecimal (0x...) notation for better readability.
   --> target/lintcheck/sources/sha2-0.11.0-pre.3/src/sha256/x86.rs:100:1
    |
100 | cpufeatures::new!(shani_cpuid, "sha", "sse2", "ssse3", "sse4.1");
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `--force-warn clippy::decimal-bit-mask` implied by `--force-warn clippy::nursery`
    = note: this warning originates in the macro `$crate::__detect_target_features` which comes from the expansion of the macro `cpufeatures::new` (in Nightly builds, run with -Z macro-backtrace for more info)

Here, the lint fires on some binary operation coming deep from an expansion of cpufeatures::new!. Typically, you want to avoid linting things coming from expansion -- please use !e.span.from_expansion() to filter those out. See https://doc.rust-lang.org/clippy/development/macro_expansions.html#the-spanfrom_expansion-method for more information

warning: Using decimal literal for bit mask. Consider using binary (0b...) or hexadecimal (0x...) notation for better readability.
    --> target/lintcheck/sources/chrono-0.4.38/src/naive/date/mod.rs:1344:9
     |
1344 |         self.yof() & (0b1000) == 0
     |         ^^^^^^^^^^^^^^^^^^^^^

Here, 0b1000 isn't recognized as being a binary literal, because the way you check for that is by looking at the span of the literal, but here the span actually contains the parentheses surrounding the literal. Because of that, you'll need to first trim starting paren(s), then possibly some whitespace, and only then performing the actual checks.

warning: Using decimal literal for bit mask. Consider using binary (0b...) or hexadecimal (0x...) notation for better readability.
   --> target/lintcheck/sources/ahash-0.8.11/src/fallback_hash.rs:197:19
    |
197 |         let rot = (self.buffer & 63) as u32;
    |                   ^^^^^^^^^^^^^^^^^^
    |
    = note: `--force-warn clippy::decimal-bit-mask` implied by `--force-warn clippy::nursery`

Here, and in a couple more cases, the decimal operand is a power of 2, or (power of 2 - 1). I think it'd be appropriate to give them a pass. To get the value of a literal, you can go

if let ExprKind::Lit(lit) = left
    && let LitKind::Int(val, _) = lit

with val being your value.

Note that this will also include operands 0 and 1, which is desirable.

warning: Using decimal literal for bit mask. Consider using binary (0b...) or hexadecimal (0x...) notation for better readability.
   --> target/lintcheck/sources/libc-0.2.155/src/unix/linux_like/linux/gnu/b64/x86_64/mod.rs:416:32
    |
416 | pub const O_TMPFILE: ::c_int = 0o20000000 | O_DIRECTORY;
    |                                ^^^^^^^^^^^^^^^^^^^^^^^^

Octal operands probably also make sense in some contexts, so I'd give them a pass as well.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Artur-Sulej]

@ada4a Thank you for the very thorough review and the clear explanations of all the details! I’ve addressed all the issues, and the PR is ready for another round of review.

[Artur-Sulej]

Here, 0b1000 isn't recognized as being a binary literal, because the way you check for that is by looking at the span of the literal, but here the span actually contains the parentheses surrounding the literal. Because of that, you'll need to first trim starting paren(s), then possibly some whitespace, and only then performing the actual checks.

@ada4a I simplified this part (using contains) to handle parentheses, whitespaces and comments. It might miss some complex cases, but that’s an intentional trade-off. Such cases usually mean special constructs and skipping the lint seems more appropriate.