-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New lint: decimal_bit_mask
#15215
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
base: master
Are you sure you want to change the base?
New lint: decimal_bit_mask
#15215
Conversation
|
Lintcheck changes for 35d6cc0
This comment will be updated if you push new changes |
|
Although I annotated my test with |
This comment has been minimized.
This comment has been minimized.
013adec to
12ab9f6
Compare
This comment has been minimized.
This comment has been minimized.
tests/ui/decimal_bit_mask.rs
Outdated
| 99 | 0b1010; //~ decimal_bit_mask | ||
| 99 ^ 0b1010; //~ decimal_bit_mask | ||
| 0xD | 99; //~ decimal_bit_mask | ||
| 88 & 99; //~ decimal_bit_mask |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
| 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_maskdirectly above), and will expect a lint at the same location (in this case, the line containing88 & 99;)
clippy_lints/src/decimal_bit_mask.rs
Outdated
| Expr { | ||
| kind: kind1, | ||
| span: span1, | ||
| .. | ||
| }, | ||
| Expr { | ||
| kind: kind2, | ||
| span: span2, | ||
| .. | ||
| }, | ||
| ) = &e.kind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
clippy_lints/src/decimal_bit_mask.rs
Outdated
| span_lint( | ||
| cx, | ||
| DECIMAL_BIT_MASK, | ||
| e.span, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
clippy_lints/src/decimal_bit_mask.rs
Outdated
| cx, | ||
| DECIMAL_BIT_MASK, | ||
| e.span, | ||
| "Using decimal literal for bit mask. Consider using binary (0b...) or hexadecimal (0x...) notation for better readability.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
clippy_lints/src/decimal_bit_mask.rs
Outdated
| && let Some(snippet) = snippet_opt(cx, *span2) | ||
| && !snippet.starts_with("0b") | ||
| && !snippet.starts_with("0x") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
| && 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")) |
12ab9f6 to
580fe78
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot note remove Feature-freeze |
|
Could you please change the PR description to something like this? """ Example: let masked = x & 15; // Bad: decimal literal as bit mask
let masked = x & 0b1111; // Good: bit pattern is explicitchangelog: [ Fixes #1775 Motivation for the changes:
|
|
If you look at Lintcheck results (displayed over at #15215 (comment)), you can see a couple of problems with the current implementation: Here, the lint fires on some binary operation coming deep from an expansion of Here, 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 with Note that this will also include operands 0 and 1, which is desirable. Octal operands probably also make sense in some contexts, so I'd give them a pass as well. |
|
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. |
This comment has been minimized.
This comment has been minimized.
212fe6e to
a33e249
Compare
|
@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. |
| } | ||
|
|
||
| fn is_not_decimal_number(cx: &LateContext<'_>, span: Span) -> bool { | ||
| span.check_source_text(cx, |src| src.contains("0b") || src.contains("0x") || src.contains("0o")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! You might want to add a small comment about this (e.g. why you didn't use starts_with)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress! Much smaller comments this time (apart from the move-to-operators thing, that might be a bit churny -- sorry about that^^)
| } | ||
|
|
||
| fn is_not_decimal_number(cx: &LateContext<'_>, span: Span) -> bool { | ||
| span.check_source_text(cx, |src| src.contains("0b") || src.contains("0x") || src.contains("0o")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! You might want to add a small comment about this (e.g. why you didn't use starts_with)
| { | ||
| for expr in [left, right] { | ||
| if let ExprKind::Lit(_) = &expr.kind | ||
| && !is_not_decimal_number(cx, expr.span) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The double negation is a bit confusing. Could you please invert the logic of is_not_decimal_number and rename it accordingly?
| for expr in [left, right] { | ||
| if let ExprKind::Lit(_) = &expr.kind | ||
| && !is_not_decimal_number(cx, expr.span) | ||
| && !is_power_of_twoish(expr) | ||
| { | ||
| emit_lint(cx, expr.span); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got really confused here:
-
Usually, the convention is to call the main expression
expr, and subexpressions, if any,e. But you do it the other way around, calling each subexpression (leftandright)expr. Because of that, I almost thought you had a logic bug, due to callingis_not_decimal_numberandemit_linton the main expression instead of the subexpression. I'd be grateful if you could swap their names -
In this function, you call
is_not_decimal_numberandemit_lintonexpr, while incheck_bitwise_assign_expr, you call it onlit. Looking at the test suite (which is very extensive, thanks for that!), one can see the following diagnostics:error: using decimal literal for bitwise operation --> tests/ui/decimal_bit_mask.rs:18:9 | LL | x ^ (99); | ^^^^ error: using decimal literal for bitwise operation --> tests/ui/decimal_bit_mask.rs:21:11 | LL | x ^= (99); | ^^So the assign-op case points at the number itself, as the span of
litapparently doesn't include the parens. I think that comes out a bit nicer, so I'd ask you to uselitin this function as well.Also, as the span the of the literal doesn't include the parens, maybe you might be able use
starts_withinis_not_decimal_numberafter all?
| span, | ||
| "using decimal literal for bitwise operation", | ||
| None, | ||
| "use binary (0b...) or hex (0x...) or octal (0o...) notation for better readability", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "use binary (0b...) or hex (0x...) or octal (0o...) notation for better readability", | |
| "use binary (0b...), hex (0x...), or octal (0o...) notation for better readability", |
| if let ExprKind::AssignOp( | ||
| Spanned { | ||
| node: AssignOpKind::BitAndAssign | AssignOpKind::BitOrAssign | AssignOpKind::BitXorAssign, | ||
| .. | ||
| }, | ||
| _, | ||
| expr @ Expr { | ||
| kind: ExprKind::Lit(lit), | ||
| .. | ||
| }, | ||
| ) = &e.kind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very much a nit: imo, these big patterns are too verbose -- what do you think about something like this?
| if let ExprKind::AssignOp( | |
| Spanned { | |
| node: AssignOpKind::BitAndAssign | AssignOpKind::BitOrAssign | AssignOpKind::BitXorAssign, | |
| .. | |
| }, | |
| _, | |
| expr @ Expr { | |
| kind: ExprKind::Lit(lit), | |
| .. | |
| }, | |
| ) = &e.kind | |
| if let ExprKind::AssignOp(op, _, expr) = &e.kind | |
| && matches!(op.node, AssignOpKind::BitAndAssign | AssignOpKind::BitOrAssign | AssignOpKind::BitXorAssign) | |
| && let ExprKind::Lit(lit) = expr.kind |
| /// | ||
| /// ### Why is this bad? | ||
| /// Using decimal literals for bit masks can make the code less readable and obscure the intended bit pattern. | ||
| /// Binary or hexadecimal or octal literals make the bit pattern more explicit and easier to understand at a glance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Binary or hexadecimal or octal literals make the bit pattern more explicit and easier to understand at a glance. | |
| /// Binary, hexadecimal, or octal literals make the bit pattern more explicit and easier to understand at a glance. |
| #[clippy::version = "1.87.0"] | ||
| pub DECIMAL_BIT_MASK, | ||
| nursery, | ||
| "use binary, hex or octal literals for bitwise operations" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "use binary, hex or octal literals for bitwise operations" | |
| "use binary, hex, or octal literals for bitwise operations" |
| /// ```rust,no_run | ||
| /// let a = 0b1110 & 0b0110; | ||
| /// ``` | ||
| #[clippy::version = "1.87.0"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be updated^^
| /// let a = 0b1110 & 0b0110; | ||
| /// ``` | ||
| #[clippy::version = "1.87.0"] | ||
| pub DECIMAL_BIT_MASK, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to say this earlier, but lint names should be in plural, see https://rust-lang.github.io/rfcs/0344-conventions-galore.html#lints
| "use binary, hex or octal literals for bitwise operations" | ||
| } | ||
|
|
||
| declare_lint_pass!(DecimalBitMask => [DECIMAL_BIT_MASK]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lint would actually fit quite well into the operators group I think. You can take a look at the lints in the operators/ directory for an example of how to integrate a lint into a group
|
☔ The latest upstream changes (possibly c48592e) made this pull request unmergeable. Please resolve the merge conflicts. |
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:
changelog: [
decimal_bit_mask]: new lintFixes #1775