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

[WIP] Add new lint for xor-ing when pow was probably intended #6182

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2047,6 +2047,7 @@ Released 2018-09-13
[`wrong_pub_self_convention`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_pub_self_convention
[`wrong_self_convention`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
[`wrong_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_transmute
[`xor_used_as_pow`]: https://rust-lang.github.io/rust-clippy/master/index.html#xor_used_as_pow
[`zero_divided_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_divided_by_zero
[`zero_prefixed_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_prefixed_literal
[`zero_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_ptr
Expand Down
5 changes: 5 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,7 @@ mod verbose_file_reads;
mod wildcard_dependencies;
mod wildcard_imports;
mod write;
mod xor_used_as_pow;
mod zero_div_zero;
// end lints modules, do not remove this comment, it’s used in `update_lints`

Expand Down Expand Up @@ -909,6 +910,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&write::WRITELN_EMPTY_STRING,
&write::WRITE_LITERAL,
&write::WRITE_WITH_NEWLINE,
&xor_used_as_pow::XOR_USED_AS_POW,
&zero_div_zero::ZERO_DIVIDED_BY_ZERO,
]);
// end register lints, do not remove this comment, it’s used in `update_lints`
Expand Down Expand Up @@ -1148,6 +1150,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_early_pass(|| box asm_syntax::InlineAsmX86AttSyntax);
store.register_early_pass(|| box asm_syntax::InlineAsmX86IntelSyntax);
store.register_late_pass(|| box undropped_manually_drops::UndroppedManuallyDrops);
store.register_late_pass(|| box xor_used_as_pow::XorUsedAsPow);


store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
Expand Down Expand Up @@ -1557,6 +1560,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&write::WRITELN_EMPTY_STRING),
LintId::of(&write::WRITE_LITERAL),
LintId::of(&write::WRITE_WITH_NEWLINE),
LintId::of(&xor_used_as_pow::XOR_USED_AS_POW),
LintId::of(&zero_div_zero::ZERO_DIVIDED_BY_ZERO),
]);

Expand Down Expand Up @@ -1820,6 +1824,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&unused_io_amount::UNUSED_IO_AMOUNT),
LintId::of(&unwrap::PANICKING_UNWRAP),
LintId::of(&vec_resize_to_zero::VEC_RESIZE_TO_ZERO),
LintId::of(&xor_used_as_pow::XOR_USED_AS_POW),
]);

store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
Expand Down
174 changes: 174 additions & 0 deletions clippy_lints/src/xor_used_as_pow.rs
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,
cgm616 marked this conversation as resolved.
Show resolved Hide resolved
"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)> {
Copy link
Member

Choose a reason for hiding this comment

The 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) {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 clippy already? Particularly, someone might have something like this:

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.

Copy link
Member

Choose a reason for hiding this comment

The 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_TYPE_ON_DIAGNOSTIC_ITEM, in the function path_to_matched_type. I think that could help.

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()?",
)
}
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2853,6 +2853,13 @@ vec![
deprecation: None,
module: "transmute",
},
Lint {
name: "xor_used_as_pow",
group: "correctness",
desc: "use of `^` operator when exponentiation was probably intended",
deprecation: None,
module: "xor_used_as_pow",
},
Lint {
name: "zero_divided_by_zero",
group: "complexity",
Expand Down
29 changes: 29 additions & 0 deletions tests/ui/xor_used_as_pow.rs
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;
}
Copy link
Member

Choose a reason for hiding this comment

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

We're missing two tests:

  • The RHS is a const
  • The RHS is an immutable binding initialized with a const

}
19 changes: 19 additions & 0 deletions tests/ui/xor_used_as_pow.stderr
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

31 changes: 31 additions & 0 deletions tests/ui/xor_used_as_pow_fixable.fixed
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;
}
}
31 changes: 31 additions & 0 deletions tests/ui/xor_used_as_pow_fixable.rs
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
Copy link
Member

Choose a reason for hiding this comment

The 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;
}
}
22 changes: 22 additions & 0 deletions tests/ui/xor_used_as_pow_fixable.stderr
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