-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Changes from all commits
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,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 { | ||
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), | ||
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. Should the suggestion be about 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 we add a suggestion for 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. 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 🤔 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. Good point! I would suggest the bitshift, but write this in the documentation of the lint. 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. (Additionally, arguably, why should bitshift be more efficient than 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. Another interesting thought here for consideration, perhaps recommending the 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.
Yeah it should be one for 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 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 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 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");
} 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. 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 |
||
Applicability::MaybeIncorrect, | ||
) | ||
} | ||
} | ||
} | ||
} |
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); | ||
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. This should also get linted, shouldn't it? |
||
let y = 2; | ||
println!("{}", y ^ 16); | ||
} |
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 | ||
|
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.
Please also make sure that it doesn't trigger in external macros (using
in_external_macro
)