Apply suggestions from code review

cgm616 · ebroto · cgm616 · commit 03ca52e69241 · 2020-10-22T00:07:54.000-04:00
Minor docs and description changes

Co-authored-by: Eduardo Broto &lt;ebroto@tutanota.com&gt;
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -1139,7 +1139,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(move || box disallowed_method::DisallowedMethod::new(&disallowed_methods));
     store.register_early_pass(|| box asm_syntax::InlineAsmX86AttSyntax);
     store.register_early_pass(|| box asm_syntax::InlineAsmX86IntelSyntax);
-    store.register_early_pass(|| box xor_used_as_pow::XorUsedAsPow);
+    store.register_late_pass(|| box xor_used_as_pow::XorUsedAsPow);
 
 
     store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
diff --git a/clippy_lints/src/xor_used_as_pow.rs b/clippy_lints/src/xor_used_as_pow.rs
@@ -1,79 +1,179 @@
-use crate::utils::{span_lint_and_help, span_lint_and_sugg};
+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::{BinOpKind, Expr, ExprKind, LitKind};
+use rustc_ast::{LitIntType, LitKind};
 use rustc_errors::Applicability;
-use rustc_lint::{EarlyContext, EarlyLintPass};
+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 intended.
+    /// **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 typo.
+    /// **Why is this bad?** This is most probably a mistake.
     ///
     /// **Known problems:** None.
     ///
     /// **Example:**
     ///
-    /// ```rust,ignore
+    /// ```rust
     /// // Bad
-    /// 2 ^ 16;
+    /// let a = 2 ^ 16;
+    /// let b = 10 ^ 4;
     ///
     /// // Good
-    /// 1 << 16;
-    /// 2i32.pow(16);
+    /// let a = 1 << 16;
+    /// let b = 10i32.pow(4);
     /// ```
     pub XOR_USED_AS_POW,
     correctness,
-    "use of `^` operator when exponentiation was intended"
+    "use of `^` operator when exponentiation was probably intended"
 }
 
 declare_lint_pass!(XorUsedAsPow => [XOR_USED_AS_POW]);
 
-impl EarlyLintPass for XorUsedAsPow {
-    fn check_expr(&mut self, cx: &EarlyContext<'_>, expr: &Expr) {
+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 !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(lit) = &left.kind;
-            if let LitKind::Int(lhs, _) = lit.kind;
-            if let ExprKind::Lit(lit) = &right.kind;
-            if let LitKind::Int(rhs, _) = lit.kind;
+            if let ExprKind::Lit(lhs) = &left.kind;
+            if let Some((lhs_val, lhs_type)) = unwrap_dec_int_literal(cx, lhs);
             then {
-                if lhs == 2 {
-                    if rhs == 8 || rhs == 16 || rhs == 32 || rhs == 64 || rhs == 128 {
-                        span_lint_and_sugg(
-                            cx,
-                            XOR_USED_AS_POW,
-                            expr.span,
-                            "it appears you are trying to get the maximum value of an integer, but `^` is not an exponentiation operator",
-                            "try",
-                            format!("std::u{}::MAX", rhs),
-                            Applicability::MaybeIncorrect,
-                        )
-                    } else {
-                        span_lint_and_sugg(
-                            cx,
-                            XOR_USED_AS_POW,
-                            expr.span,
-                            "it appears you are trying to get a power of two, but `^` is not an exponentiation operator",
-                            "use a bitshift instead",
-                            format!("1 << {}", rhs),
-                            Applicability::MaybeIncorrect,
-                        )
+                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);
+                        }
                     }
-                } else {
-                    span_lint_and_help(
-                        cx,
-                        XOR_USED_AS_POW,
-                        expr.span,
-                        "`^` is not an exponentiation operator but appears to have been used as one",
-                        None,
-                        "did you mean to use .pow()?"
-                    )
+                    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)> {
+    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 {
+            return Some((val, val_type));
+        }
+        else {
+            return None;
+        }
+    }
+}
+
+fn report_with_ident(cx: &LateContext<'_>, lhs: u128, rhs: &QPath<'_>, span: Span) {
+    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 {
+        return;
+    }
+    match lhs {
+        2 => {
+            if rhs == 0 {
+                report_pow_of_two(cx, format!("1"), span);
+                return;
+            }
+
+            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()?",
+    )
+}
diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs
@@ -2814,7 +2814,7 @@ vec![
     Lint {
         name: "xor_used_as_pow",
         group: "correctness",
-        desc: "use of `^` operator when exponentiation was intended",
+        desc: "use of `^` operator when exponentiation was probably intended",
         deprecation: None,
         module: "xor_used_as_pow",
     },
diff --git a/tests/ui/xor_used_as_pow.fixed b/tests/ui/xor_used_as_pow.fixed
@@ -0,0 +1,33 @@
+// run-rustfix
+#![warn(clippy::xor_used_as_pow)]
+
+// 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
+
+    // These should fail
+    let _ = 1_u32 << 3;
+    let _ = 10 ^ 4;
+    let _ = 1_u64 << 32;
+    let _ = 1;
+    let _ = 10 ^ 0;
+    {
+        let x = 15;
+        let _ = 1 << x;
+        let _ = 10 ^ x;
+    }
+}
diff --git a/tests/ui/xor_used_as_pow.rs b/tests/ui/xor_used_as_pow.rs
@@ -1,15 +1,33 @@
+// run-rustfix
 #![warn(clippy::xor_used_as_pow)]
 
+// 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
-    // With variables, it's not as clear whether the intention was exponentiation or not
-    let x = 15;
-    println!("{}", 2 ^ x);
-    let y = 2;
-    println!("{}", y ^ 16);
+    // 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
 
     // These should fail
-    println!("{}", 2 ^ 16);
-    println!("{}", 2 ^ 7);
-    println!("{}", 9 ^ 3);
+    let _ = 2 ^ 3;
+    let _ = 10 ^ 4;
+    let _ = 2 ^ 32;
+    let _ = 2 ^ 0;
+    let _ = 10 ^ 0;
+    {
+        let x = 15;
+        let _ = 2 ^ x;
+        let _ = 10 ^ x;
+    }
 }
diff --git a/tests/ui/xor_used_as_pow.stderr b/tests/ui/xor_used_as_pow.stderr
@@ -1,24 +1,66 @@
-error: it appears you are trying to get the maximum value of an integer, but `^` is not an exponentiation operator
-  --> $DIR/xor_used_as_pow.rs:12:20
+error: it appears you are trying to get a power of two, but `^` is not an exponentiation operator
+  --> $DIR/xor_used_as_pow.rs:23:13
    |
-LL |     println!("{}", 2 ^ 16);
-   |                    ^^^^^^ help: try: `std::u16::MAX`
+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: `^` 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;
+   |             ^^^^^^
+   |
+   = help: did you mean to use .pow()?
+
+error: it appears you are trying to get a power of two, but `^` is not an exponentiation operator
+  --> $DIR/xor_used_as_pow.rs:25:13
+   |
+LL |     let _ = 2 ^ 32;
+   |             ^^^^^^ help: use a bitshift or constant instead: `1_u64 << 32`
+
+error: the operation is ineffective. Consider reducing it to `2`
+  --> $DIR/xor_used_as_pow.rs:26:13
+   |
+LL |     let _ = 2 ^ 0;
+   |             ^^^^^
+   |
+   = note: `-D clippy::identity-op` 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.rs:26:13
+   |
+LL |     let _ = 2 ^ 0;
+   |             ^^^^^ help: use a bitshift or constant instead: `1`
+
+error: the operation is ineffective. Consider reducing it to `10`
+  --> $DIR/xor_used_as_pow.rs:27:13
+   |
+LL |     let _ = 10 ^ 0;
+   |             ^^^^^^
+
+error: `^` is not an exponentiation operator but appears to have been used as one
+  --> $DIR/xor_used_as_pow.rs:27:13
+   |
+LL |     let _ = 10 ^ 0;
+   |             ^^^^^^
+   |
+   = help: did you mean to use .pow()?
+
 error: it appears you are trying to get a power of two, but `^` is not an exponentiation operator
-  --> $DIR/xor_used_as_pow.rs:13:20
+  --> $DIR/xor_used_as_pow.rs:30:17
    |
-LL |     println!("{}", 2 ^ 7);
-   |                    ^^^^^ help: use a bitshift instead: `1 << 7`
+LL |         let _ = 2 ^ x;
+   |                 ^^^^^ help: use a bitshift or constant instead: `1 << x`
 
 error: `^` is not an exponentiation operator but appears to have been used as one
-  --> $DIR/xor_used_as_pow.rs:14:20
+  --> $DIR/xor_used_as_pow.rs:31:17
    |
-LL |     println!("{}", 9 ^ 3);
-   |                    ^^^^^
+LL |         let _ = 10 ^ x;
+   |                 ^^^^^^
    |
    = help: did you mean to use .pow()?
 
-error: aborting due to 3 previous errors
+error: aborting due to 9 previous errors
 
