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

Add lint for use of ^ operator as pow. #4213

Closed
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 @@ -1157,6 +1157,7 @@ All notable changes to this project will be documented in this file.
[`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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.

[There are 305 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
[There are 306 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)

We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:

Expand Down
4 changes: 4 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,7 @@ pub mod use_self;
pub mod vec;
pub mod wildcard_dependencies;
pub mod write;
pub mod xor_used_as_pow;
pub mod zero_div_zero;
// end lints modules, do not remove this comment, it’s used in `update_lints`

Expand Down Expand Up @@ -582,6 +583,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
reg.register_late_lint_pass(box path_buf_push_overwrite::PathBufPushOverwrite);
reg.register_late_lint_pass(box checked_conversions::CheckedConversions);
reg.register_late_lint_pass(box integer_division::IntegerDivision);
reg.register_early_lint_pass(box xor_used_as_pow::XorUsedAsPow);

reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![
arithmetic::FLOAT_ARITHMETIC,
Expand Down Expand Up @@ -889,6 +891,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &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,
]);

Expand Down Expand Up @@ -1105,6 +1108,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
types::UNIT_CMP,
unicode::ZERO_WIDTH_SPACE,
unused_io_amount::UNUSED_IO_AMOUNT,
xor_used_as_pow::XOR_USED_AS_POW,
]);

reg.register_lint_group("clippy::perf", Some("clippy_perf"), vec![
Expand Down
54 changes: 54 additions & 0 deletions clippy_lints/src/xor_used_as_pow.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
use crate::utils::span_lint_and_sugg;
use if_chain::if_chain;
use rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass};
use rustc::{declare_lint_pass, declare_tool_lint};
use rustc_errors::Applicability;
use syntax::ast::{BinOpKind, Expr, ExprKind, LitKind};

declare_clippy_lint! {
/// **What it does:** Checks for use of `^` operator when exponentiation was intended.
///
/// **Why is this bad?** This is most probably a typo.
///
/// **Known problems:** None.
///
/// **Example:**
///
/// ```rust,ignore
/// // Bad
/// 2 ^ 16;
///
/// // Good
/// 1 << 16;
/// 2i32.pow(16);
/// ```
pub XOR_USED_AS_POW,
correctness,
"use of `^` operator when exponentiation was intended"
}

declare_lint_pass!(XorUsedAsPow => [XOR_USED_AS_POW]);

impl EarlyLintPass for XorUsedAsPow {
fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
if_chain! {
if let ExprKind::Binary(op, left, right) = &expr.node;
if BinOpKind::BitXor == op.node;
if let ExprKind::Lit(lit) = &left.node;
if let LitKind::Int(2, _) = lit.node;
if let ExprKind::Lit(lit) = &right.node;
if let LitKind::Int(right, _) = lit.node;
then {
Copy link
Member

Choose a reason for hiding this comment

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

Please also make sure that it doesn't trigger in external macros (using in_external_macro)

span_lint_and_sugg(
cx,
XOR_USED_AS_POW,
expr.span,
"`^` is not an exponentiation operator but was used as one",
"did you mean to write",
format!("1 << {}", right),
Copy link

Choose a reason for hiding this comment

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

Should the suggestion be about .pow() instead? The premise is, quote, "^ is not an exponentiation operator but was used as one", so it might make more sense to suggest an actual exponentiation operator than a seemingly completely unrelated bit shift?

Copy link
Member

Choose a reason for hiding this comment

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

2^x == 1 << x (That's only true for powers of 2). You don't want to use pow for powers of 2, because of performance reasons.

If we add a suggestion for 10^x, the lint should obviously suggest 10.pow(x).

Copy link

Choose a reason for hiding this comment

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

Nod, I'm aware of how the bitshift works for powers of two, was just wondering if that makes for a somewhat confusing suggestion - telling the user they probably intended exponentiation, and then suggesting a bitshift. Especially, but not limited to, if the mistake is made by a beginner, the suggestion might seem strange 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Good point! I would suggest the bitshift, but write this in the documentation of the lint.

Copy link

Choose a reason for hiding this comment

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

(Additionally, arguably, why should bitshift be more efficient than .pow(), especially for known-at-compile-time values, shouldn't that be a compiler optimization that the programmer doesn't need to worry about? 🤔 )

Copy link

Choose a reason for hiding this comment

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

Another interesting thought here for consideration, perhaps recommending the std::_::MAX values, if applicable 🤔 rust-lang/rust#61934 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't that be a compiler optimization

Yeah it should be one for pow(some_power_of_two, x). But using a bitshift is always performant (and a pretty standard thing to do). Relying on the compiler might not always produce performant code. And Clippy should always suggest the best possible code IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I end up picking this up (assuming that's desired or needed), would it be worthwhile to put out a different error based on whether the number is 2 or not? If it's two, output the suggestion of bitshift, if it's not, mention pow?

Copy link
Member

Choose a reason for hiding this comment

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

We don't need an extra lint for this, I think. But you can do something like:

// NOTE: This is pseudo code and the messages need improvement. ;)
if lhs == 2 {
    span_lint(LINT, "this looks like you want to pow", "try bitshift");
} else {
    span_lint(LINT, "..", "try pow");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's akin to what I was thinking. I think I'll also look at incorporating Walther's suggestion (and the comment from this thread) to recommend std::_::MAX in the case of rhs being an Int literal of 8, 16, 32, or 64.

Applicability::MaybeIncorrect,
)
}
}
}
}
9 changes: 8 additions & 1 deletion src/lintlist/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub use lint::Lint;
pub use lint::LINT_LEVELS;

// begin lint list, do not remove this comment, it’s used in `update_lints`
pub const ALL_LINTS: [Lint; 305] = [
pub const ALL_LINTS: [Lint; 306] = [
Lint {
name: "absurd_extreme_comparisons",
group: "correctness",
Expand Down Expand Up @@ -2114,6 +2114,13 @@ pub const ALL_LINTS: [Lint; 305] = [
deprecation: None,
module: "transmute",
},
Lint {
name: "xor_used_as_pow",
group: "correctness",
desc: "use of `^` operator when exponentiation was intended",
deprecation: None,
module: "xor_used_as_pow",
},
Lint {
name: "zero_divided_by_zero",
group: "complexity",
Expand Down
10 changes: 10 additions & 0 deletions tests/ui/xor_used_as_pow.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
#![warn(clippy::xor_used_as_pow)]

fn main() {
println!("{}", 2 ^ 16);
// Should be allowed
let x = 16;
println!("{}", 2 ^ x);
Copy link
Member

@flip1995 flip1995 Jun 17, 2019

Choose a reason for hiding this comment

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

This should also get linted, shouldn't it?

let y = 2;
println!("{}", y ^ 16);
}
10 changes: 10 additions & 0 deletions tests/ui/xor_used_as_pow.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error: `^` is not an exponentiation operator but was used as one
--> $DIR/xor_used_as_pow.rs:4:20
|
LL | println!("{}", 2 ^ 16);
| ^^^^^^ help: did you mean to write: `1 << 16`
|
= note: `-D clippy::xor-used-as-pow` implied by `-D warnings`

error: aborting due to previous error