-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[WIP] Add new lint for xor-ing when pow
was probably intended
#6182
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
Changes from all commits
4336396
773dc76
282aafe
12f150e
ebb71f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,174 @@ | ||
use crate::utils::{ | ||
last_path_segment, numeric_literal::NumericLiteral, qpath_res, snippet_opt, span_lint_and_help, span_lint_and_sugg, | ||
}; | ||
use if_chain::if_chain; | ||
use rustc_ast::{LitIntType, LitKind}; | ||
use rustc_errors::Applicability; | ||
use rustc_hir::{ | ||
def::{DefKind, Res}, | ||
BinOpKind, BindingAnnotation, Expr, ExprKind, ItemKind, Lit, Node, PatKind, QPath, | ||
}; | ||
use rustc_lint::{LateContext, LateLintPass, LintContext}; | ||
use rustc_middle::lint::in_external_macro; | ||
use rustc_session::{declare_lint_pass, declare_tool_lint}; | ||
use rustc_span::Span; | ||
|
||
declare_clippy_lint! { | ||
/// **What it does:** Checks for use of `^` operator when exponentiation was probably intended. | ||
/// A caret is commonly an ASCII-compatible/keyboard-accessible way to write down exponentiation in docs, | ||
/// readmes, and comments, and copying and pasting a formula can inadvertedly introduce this error. | ||
/// Moreover, `^` means exponentiation in other programming languages. | ||
/// | ||
/// **Why is this bad?** This is most probably a mistake. | ||
/// | ||
/// **Known problems:** None. | ||
/// | ||
/// **Example:** | ||
/// | ||
/// ```rust | ||
/// // Bad | ||
/// let a = 2 ^ 16; | ||
/// let b = 10 ^ 4; | ||
/// | ||
/// // Good | ||
/// let a = 1 << 16; | ||
/// let b = 10i32.pow(4); | ||
/// ``` | ||
pub XOR_USED_AS_POW, | ||
correctness, | ||
"use of `^` operator when exponentiation was probably intended" | ||
} | ||
|
||
declare_lint_pass!(XorUsedAsPow => [XOR_USED_AS_POW]); | ||
|
||
impl LateLintPass<'_> for XorUsedAsPow { | ||
fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) { | ||
let parent_id = cx.tcx.hir().get_parent_item(expr.hir_id); | ||
if let Some(Node::Item(parent_item)) = cx.tcx.hir().find(parent_id) { | ||
if let ItemKind::Enum(_, _) = parent_item.kind { | ||
return; | ||
} | ||
} | ||
|
||
if_chain! { | ||
if !in_external_macro(cx.sess(), expr.span); | ||
if let ExprKind::Binary(op, left, right) = &expr.kind; | ||
if BinOpKind::BitXor == op.node; | ||
if let ExprKind::Lit(lhs) = &left.kind; | ||
if let Some((lhs_val, _)) = unwrap_dec_int_literal(cx, lhs); | ||
then { | ||
cgm616 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
match &right.kind { | ||
ExprKind::Lit(rhs) => { | ||
if let Some((rhs_val, _)) = unwrap_dec_int_literal(cx, rhs) { | ||
report_with_lit(cx, lhs_val, rhs_val, expr.span); | ||
} | ||
} | ||
ExprKind::Path(qpath) => { | ||
match qpath_res(cx, qpath, right.hir_id) { | ||
Res::Local(hir_id) => { | ||
if_chain! { | ||
let node = cx.tcx.hir().get(hir_id); | ||
if let Node::Binding(pat) = node; | ||
if let PatKind::Binding(bind_ann, ..) = pat.kind; | ||
if !matches!(bind_ann, BindingAnnotation::RefMut | | ||
BindingAnnotation::Mutable); | ||
let parent_node = cx.tcx.hir().get_parent_node(hir_id); | ||
if let Some(Node::Local(parent_let_expr)) = cx.tcx.hir().find(parent_node); | ||
if let Some(init) = parent_let_expr.init; | ||
then { | ||
match init.kind { | ||
// immutable bindings that are initialized with literal | ||
ExprKind::Lit(..) => report_with_ident(cx, lhs_val, qpath, expr.span), | ||
// immutable bindings that are initialized with constant | ||
ExprKind::Path(ref path) => { | ||
let res = qpath_res(cx, path, init.hir_id); | ||
if let Res::Def(DefKind::Const, ..) = res { | ||
report_with_ident(cx, lhs_val, qpath, expr.span); | ||
} | ||
} | ||
_ => {}, | ||
} | ||
} | ||
} | ||
}, | ||
// constant | ||
Res::Def(DefKind::Const, ..) => report_with_ident(cx, lhs_val, qpath, expr.span), | ||
_ => {}, | ||
} | ||
} | ||
_ => {} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
fn unwrap_dec_int_literal(cx: &LateContext<'_>, lit: &Lit) -> Option<(u128, LitIntType)> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that we are not using the second element of the returned tuple |
||
if_chain! { | ||
if let LitKind::Int(val, val_type) = lit.node; | ||
if let Some(snippet) = snippet_opt(cx, lit.span); | ||
if let Some(decoded) = NumericLiteral::from_lit_kind(&snippet, &lit.node); | ||
if decoded.is_decimal(); | ||
then { | ||
Some((val, val_type)) | ||
} | ||
else { | ||
None | ||
} | ||
} | ||
} | ||
|
||
fn report_with_ident(cx: &LateContext<'_>, lhs: u128, rhs: &QPath<'_>, span: Span) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In these cases we're not handling the overflow. let x = 42;
let _ = 2 ^ x; The suggestion would result in code that does not compile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking about this while writing the relevant portions. In order to figure out if this overflows, I'll need to get the literal from the definition (or const or const-binding). Is there a utility for this in const A: u32 = 42;
const B: u32 = A;
fn main() {
let c = B;
let _ = 2 ^ c;
} It seems like I'd need to iterate over the chain of definitions and grab the one that ends in a literal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm not mistaken, there is nothing like this in utils. I remember doing something similar for the internal lint |
||
match lhs { | ||
2 => { | ||
let ident = last_path_segment(rhs).ident.name.to_ident_string(); | ||
report_pow_of_two(cx, format!("1 << {}", ident), span); | ||
}, | ||
10 => report_pow_of_ten(cx, span), | ||
_ => {}, | ||
} | ||
} | ||
|
||
fn report_with_lit(cx: &LateContext<'_>, lhs: u128, rhs: u128, span: Span) { | ||
if rhs > 127 || rhs == 0 { | ||
return; | ||
} | ||
match lhs { | ||
2 => { | ||
let lhs_str = if rhs <= 31 { | ||
"1_u32" | ||
} else if rhs <= 63 { | ||
"1_u64" | ||
} else { | ||
"1_u127" | ||
}; | ||
|
||
report_pow_of_two(cx, format!("{} << {}", lhs_str, rhs), span); | ||
}, | ||
10 => report_pow_of_ten(cx, span), | ||
_ => {}, | ||
} | ||
} | ||
|
||
fn report_pow_of_two(cx: &LateContext<'_>, sugg: String, span: Span) { | ||
span_lint_and_sugg( | ||
cx, | ||
XOR_USED_AS_POW, | ||
span, | ||
"it appears you are trying to get a power of two, but `^` is not an exponentiation operator", | ||
"use a bitshift or constant instead", | ||
sugg, | ||
Applicability::MaybeIncorrect, | ||
) | ||
} | ||
|
||
fn report_pow_of_ten(cx: &LateContext<'_>, span: Span) { | ||
span_lint_and_help( | ||
cx, | ||
XOR_USED_AS_POW, | ||
span, | ||
"`^` is not an exponentiation operator but appears to have been used as one", | ||
None, | ||
"did you mean to use .pow()?", | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
#![warn(clippy::xor_used_as_pow)] | ||
cgm616 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#![allow(clippy::identity_op)] | ||
|
||
// Should not be linted | ||
#[allow(dead_code)] | ||
enum E { | ||
First = 1 ^ 8, | ||
Second = 2 ^ 8, | ||
Third = 3 ^ 8, | ||
Tenth = 10 ^ 8, | ||
} | ||
|
||
fn main() { | ||
cgm616 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// These should succeed: | ||
let _ = 9 ^ 3; // lhs other than 2 or 10 | ||
let _ = 0x02 ^ 6; // lhs not decimal | ||
let _ = 2 ^ 0x10; // rhs hexadecimal | ||
let _ = 10 ^ 0b0101; // rhs binary | ||
let _ = 2 ^ 0o1; // rhs octal | ||
let _ = 10 ^ -18; // negative rhs | ||
let _ = 2 ^ 0; // zero rhs | ||
|
||
// These should fail | ||
let _ = 10 ^ 4; | ||
{ | ||
let x = 15; | ||
let _ = 10 ^ x; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're missing two tests:
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
error: `^` is not an exponentiation operator but appears to have been used as one | ||
--> $DIR/xor_used_as_pow.rs:24:13 | ||
| | ||
LL | let _ = 10 ^ 4; | ||
| ^^^^^^ | ||
| | ||
= note: `-D clippy::xor-used-as-pow` implied by `-D warnings` | ||
= help: did you mean to use .pow()? | ||
|
||
error: `^` is not an exponentiation operator but appears to have been used as one | ||
--> $DIR/xor_used_as_pow.rs:27:17 | ||
| | ||
LL | let _ = 10 ^ x; | ||
| ^^^^^^ | ||
| | ||
= help: did you mean to use .pow()? | ||
|
||
error: aborting due to 2 previous errors | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// run-rustfix | ||
#![warn(clippy::xor_used_as_pow)] | ||
#![allow(clippy::identity_op)] | ||
|
||
// Should not be linted | ||
#[allow(dead_code)] | ||
enum E { | ||
First = 1 ^ 8, | ||
Second = 2 ^ 8, | ||
Third = 3 ^ 8, | ||
Tenth = 10 ^ 8, | ||
} | ||
|
||
fn main() { | ||
// These should succeed: | ||
let _ = 9 ^ 3; // lhs other than 2 or 10 | ||
let _ = 0x02 ^ 6; // lhs not decimal | ||
let _ = 2 ^ 0x10; // rhs hexadecimal | ||
let _ = 10 ^ 0b0101; // rhs binary | ||
let _ = 2 ^ 0o1; // rhs octal | ||
let _ = 10 ^ -18; // negative rhs | ||
let _ = 2 ^ 0; // zero rhs | ||
|
||
// These should fail | ||
let _ = 1_u32 << 3; | ||
let _ = 1_u64 << 32; | ||
{ | ||
let x = 15; | ||
let _ = 1 << x; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// run-rustfix | ||
#![warn(clippy::xor_used_as_pow)] | ||
#![allow(clippy::identity_op)] | ||
|
||
// Should not be linted | ||
#[allow(dead_code)] | ||
enum E { | ||
First = 1 ^ 8, | ||
Second = 2 ^ 8, | ||
Third = 3 ^ 8, | ||
Tenth = 10 ^ 8, | ||
} | ||
|
||
fn main() { | ||
// These should succeed: | ||
let _ = 9 ^ 3; // lhs other than 2 or 10 | ||
let _ = 0x02 ^ 6; // lhs not decimal | ||
let _ = 2 ^ 0x10; // rhs hexadecimal | ||
let _ = 10 ^ 0b0101; // rhs binary | ||
let _ = 2 ^ 0o1; // rhs octal | ||
let _ = 10 ^ -18; // negative rhs | ||
let _ = 2 ^ 0; // zero rhs | ||
Comment on lines
+7
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can remove the cases that are already tested in the non-fixable file. |
||
|
||
// These should fail | ||
let _ = 2 ^ 3; | ||
let _ = 2 ^ 32; | ||
{ | ||
let x = 15; | ||
let _ = 2 ^ x; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
error: it appears you are trying to get a power of two, but `^` is not an exponentiation operator | ||
--> $DIR/xor_used_as_pow_fixable.rs:25:13 | ||
| | ||
LL | let _ = 2 ^ 3; | ||
| ^^^^^ help: use a bitshift or constant instead: `1_u32 << 3` | ||
| | ||
= note: `-D clippy::xor-used-as-pow` implied by `-D warnings` | ||
|
||
error: it appears you are trying to get a power of two, but `^` is not an exponentiation operator | ||
--> $DIR/xor_used_as_pow_fixable.rs:26:13 | ||
| | ||
LL | let _ = 2 ^ 32; | ||
| ^^^^^^ help: use a bitshift or constant instead: `1_u64 << 32` | ||
|
||
error: it appears you are trying to get a power of two, but `^` is not an exponentiation operator | ||
--> $DIR/xor_used_as_pow_fixable.rs:29:17 | ||
| | ||
LL | let _ = 2 ^ x; | ||
| ^^^^^ help: use a bitshift or constant instead: `1 << x` | ||
|
||
error: aborting due to 3 previous errors | ||
|
Uh oh!
There was an error while loading. Please reload this page.