Auto merge of #9031 - evantypanski:manual_rem_euclid, r=Jarcho

Add [`manual_rem_euclid`] lint

Closes #8883

Adds a lint for checking manual use of `rem_euclid(n)`

changelog: Add [`manual_rem_euclid`] lint

CHANGELOG.md

@@ -3527,6 +3527,7 @@ Released 2018-09-13
 [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
 [`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or
 [`manual_range_contains`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains
+[`manual_rem_euclid`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid
 [`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic
 [`manual_split_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_split_once
 [`manual_str_repeat`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_str_repeat

clippy_lints/src/lib.register_all.rs

@@ -135,6 +135,7 @@
     LintId::of(manual_async_fn::MANUAL_ASYNC_FN),
     LintId::of(manual_bits::MANUAL_BITS),
     LintId::of(manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
+    LintId::of(manual_rem_euclid::MANUAL_REM_EUCLID),
     LintId::of(manual_strip::MANUAL_STRIP),
     LintId::of(map_clone::MAP_CLONE),
     LintId::of(map_unit_fn::OPTION_MAP_UNIT_FN),

clippy_lints/src/lib.register_complexity.rs

@@ -24,6 +24,7 @@
     LintId::of(loops::MANUAL_FLATTEN),
     LintId::of(loops::SINGLE_ELEMENT_LOOP),
     LintId::of(loops::WHILE_LET_LOOP),
+    LintId::of(manual_rem_euclid::MANUAL_REM_EUCLID),
     LintId::of(manual_strip::MANUAL_STRIP),
     LintId::of(map_unit_fn::OPTION_MAP_UNIT_FN),
     LintId::of(map_unit_fn::RESULT_MAP_UNIT_FN),

clippy_lints/src/lib.register_lints.rs

@@ -254,6 +254,7 @@
     manual_bits::MANUAL_BITS,
     manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
     manual_ok_or::MANUAL_OK_OR,
+    manual_rem_euclid::MANUAL_REM_EUCLID,
     manual_strip::MANUAL_STRIP,
     map_clone::MAP_CLONE,
     map_err_ignore::MAP_ERR_IGNORE,

clippy_lints/src/lib.rs

@@ -282,6 +282,7 @@ macro_rules! declare_clippy_lint {
 mod manual_bits;
 mod manual_non_exhaustive;
 mod manual_ok_or;
+mod manual_rem_euclid;
 mod manual_strip;
 mod map_clone;
 mod map_err_ignore;
@@ -912,6 +913,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(as_underscore::AsUnderscore));
     store.register_late_pass(|| Box::new(read_zero_byte_vec::ReadZeroByteVec));
     store.register_late_pass(|| Box::new(default_instead_of_iter_empty::DefaultIterEmpty));
+    store.register_late_pass(move || Box::new(manual_rem_euclid::ManualRemEuclid::new(msrv)));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }
 

clippy_lints/src/manual_rem_euclid.rs

@@ -0,0 +1,123 @@
+use clippy_utils::consts::{constant_full_int, FullInt};
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::{in_constant, meets_msrv, msrvs, path_to_local};
+use rustc_errors::Applicability;
+use rustc_hir::{BinOpKind, Expr, ExprKind, Node, TyKind};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::lint::in_external_macro;
+use rustc_semver::RustcVersion;
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for an expression like `((x % 4) + 4) % 4` which is a common manual reimplementation
+    /// of `x.rem_euclid(4)`.
+    ///
+    /// ### Why is this bad?
+    /// It's simpler and more readable.
+    ///
+    /// ### Example
+    /// ```rust
+    /// let x: i32 = 24;
+    /// let rem = ((x % 4) + 4) % 4;
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let x: i32 = 24;
+    /// let rem = x.rem_euclid(4);
+    /// ```
+    #[clippy::version = "1.63.0"]
+    pub MANUAL_REM_EUCLID,
+    complexity,
+    "manually reimplementing `rem_euclid`"
+}
+
+pub struct ManualRemEuclid {
+    msrv: Option<RustcVersion>,
+}
+
+impl ManualRemEuclid {
+    #[must_use]
+    pub fn new(msrv: Option<RustcVersion>) -> Self {
+        Self { msrv }
+    }
+}
+
+impl_lint_pass!(ManualRemEuclid => [MANUAL_REM_EUCLID]);
+
+impl<'tcx> LateLintPass<'tcx> for ManualRemEuclid {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        if !meets_msrv(self.msrv, msrvs::REM_EUCLID) {
+            return;
+        }
+
+        if in_constant(cx, expr.hir_id) && !meets_msrv(self.msrv, msrvs::REM_EUCLID_CONST) {
+            return;
+        }
+
+        if in_external_macro(cx.sess(), expr.span) {
+            return;
+        }
+
+        if let ExprKind::Binary(op1, expr1, right) = expr.kind
+            && op1.node == BinOpKind::Rem
+            && let Some(const1) = check_for_unsigned_int_constant(cx, right)
+            && let ExprKind::Binary(op2, left, right) = expr1.kind
+            && op2.node == BinOpKind::Add
+            && let Some((const2, expr2)) = check_for_either_unsigned_int_constant(cx, left, right)
+            && let ExprKind::Binary(op3, expr3, right) = expr2.kind
+            && op3.node == BinOpKind::Rem
+            && let Some(const3) = check_for_unsigned_int_constant(cx, right)
+            // Also ensures the const is nonzero since zero can't be a divisor
+            && const1 == const2 && const2 == const3
+            && let Some(hir_id) = path_to_local(expr3)
+            && let Some(Node::Binding(_)) = cx.tcx.hir().find(hir_id) {
+                // Apply only to params or locals with annotated types
+                match cx.tcx.hir().find(cx.tcx.hir().get_parent_node(hir_id)) {
+                    Some(Node::Param(..)) => (),
+                    Some(Node::Local(local)) => {
+                        let Some(ty) = local.ty else { return };
+                        if matches!(ty.kind, TyKind::Infer) {
+                            return;
+                        }
+                    }
+                    _ => return,
+                };
+
+                let mut app = Applicability::MachineApplicable;
+                let rem_of = snippet_with_applicability(cx, expr3.span, "_", &mut app);
+                span_lint_and_sugg(
+                    cx,
+                    MANUAL_REM_EUCLID,
+                    expr.span,
+                    "manual `rem_euclid` implementation",
+                    "consider using",
+                    format!("{rem_of}.rem_euclid({const1})"),
+                    app,
+                );
+        }
+    }
+
+    extract_msrv_attr!(LateContext);
+}
+
+// Checks if either the left or right expressions can be an unsigned int constant and returns that
+// constant along with the other expression unchanged if so
+fn check_for_either_unsigned_int_constant<'a>(
+    cx: &'a LateContext<'_>,
+    left: &'a Expr<'_>,
+    right: &'a Expr<'_>,
+) -> Option<(u128, &'a Expr<'a>)> {
+    check_for_unsigned_int_constant(cx, left)
+        .map(|int_const| (int_const, right))
+        .or_else(|| check_for_unsigned_int_constant(cx, right).map(|int_const| (int_const, left)))
+}
+
+fn check_for_unsigned_int_constant<'a>(cx: &'a LateContext<'_>, expr: &'a Expr<'_>) -> Option<u128> {
+    let Some(int_const) = constant_full_int(cx, cx.typeck_results(), expr) else { return None };
+    match int_const {
+        FullInt::S(s) => s.try_into().ok(),
+        FullInt::U(u) => Some(u),
+    }
+}

clippy_utils/src/msrvs.rs

@@ -13,7 +13,7 @@ macro_rules! msrv_aliases {
 // names may refer to stabilized feature flags or library items
 msrv_aliases! {
     1,53,0 { OR_PATTERNS, MANUAL_BITS }
-    1,52,0 { STR_SPLIT_ONCE }
+    1,52,0 { STR_SPLIT_ONCE, REM_EUCLID_CONST }
     1,51,0 { BORROW_AS_PTR, UNSIGNED_ABS }
     1,50,0 { BOOL_THEN }
     1,47,0 { TAU }
@@ -23,7 +23,7 @@ macro_rules! msrv_aliases {
     1,42,0 { MATCHES_MACRO, SLICE_PATTERNS, PTR_SLICE_RAW_PARTS }
     1,41,0 { RE_REBALANCING_COHERENCE, RESULT_MAP_OR_ELSE }
     1,40,0 { MEM_TAKE, NON_EXHAUSTIVE, OPTION_AS_DEREF }
-    1,38,0 { POINTER_CAST }
+    1,38,0 { POINTER_CAST, REM_EUCLID }
     1,37,0 { TYPE_ALIAS_ENUM_VARIANTS }
     1,36,0 { ITERATOR_COPIED }
     1,35,0 { OPTION_COPIED, RANGE_CONTAINS }

tests/ui/auxiliary/macro_rules.rs

@@ -127,3 +127,11 @@ macro_rules! ptr_as_ptr_cast {
         $ptr as *const i32
     };
 }
+
+#[macro_export]
+macro_rules! manual_rem_euclid {
+    () => {
+        let value: i32 = 5;
+        let _: i32 = ((value % 4) + 4) % 4;
+    };
+}

tests/ui/manual_rem_euclid.fixed

@@ -0,0 +1,55 @@
+// run-rustfix
+// aux-build:macro_rules.rs
+
+#![warn(clippy::manual_rem_euclid)]
+
+#[macro_use]
+extern crate macro_rules;
+
+macro_rules! internal_rem_euclid {
+    () => {
+        let value: i32 = 5;
+        let _: i32 = value.rem_euclid(4);
+    };
+}
+
+fn main() {
+    let value: i32 = 5;
+
+    let _: i32 = value.rem_euclid(4);
+    let _: i32 = value.rem_euclid(4);
+    let _: i32 = value.rem_euclid(4);
+    let _: i32 = value.rem_euclid(4);
+    let _: i32 = 1 + value.rem_euclid(4);
+
+    let _: i32 = (3 + value % 4) % 4;
+    let _: i32 = (-4 + value % -4) % -4;
+    let _: i32 = ((5 % 4) + 4) % 4;
+
+    // Make sure the lint does not trigger if it would cause an error, like with an ambiguous
+    // integer type
+    let not_annotated = 24;
+    let _ = ((not_annotated % 4) + 4) % 4;
+    let inferred: _ = 24;
+    let _ = ((inferred % 4) + 4) % 4;
+
+    // For lint to apply the constant must always be on the RHS of the previous value for %
+    let _: i32 = 4 % ((value % 4) + 4);
+    let _: i32 = ((4 % value) + 4) % 4;
+
+    // Lint in internal macros
+    internal_rem_euclid!();
+
+    // Do not lint in external macros
+    manual_rem_euclid!();
+}
+
+// Should lint for params too
+pub fn rem_euclid_4(num: i32) -> i32 {
+    num.rem_euclid(4)
+}
+
+// Constant version came later, should still lint
+pub const fn const_rem_euclid_4(num: i32) -> i32 {
+    num.rem_euclid(4)
+}

tests/ui/manual_rem_euclid.rs

@@ -0,0 +1,55 @@
+// run-rustfix
+// aux-build:macro_rules.rs
+
+#![warn(clippy::manual_rem_euclid)]
+
+#[macro_use]
+extern crate macro_rules;
+
+macro_rules! internal_rem_euclid {
+    () => {
+        let value: i32 = 5;
+        let _: i32 = ((value % 4) + 4) % 4;
+    };
+}
+
+fn main() {
+    let value: i32 = 5;
+
+    let _: i32 = ((value % 4) + 4) % 4;
+    let _: i32 = (4 + (value % 4)) % 4;
+    let _: i32 = (value % 4 + 4) % 4;
+    let _: i32 = (4 + value % 4) % 4;
+    let _: i32 = 1 + (4 + value % 4) % 4;
+
+    let _: i32 = (3 + value % 4) % 4;
+    let _: i32 = (-4 + value % -4) % -4;
+    let _: i32 = ((5 % 4) + 4) % 4;
+
+    // Make sure the lint does not trigger if it would cause an error, like with an ambiguous
+    // integer type
+    let not_annotated = 24;
+    let _ = ((not_annotated % 4) + 4) % 4;
+    let inferred: _ = 24;
+    let _ = ((inferred % 4) + 4) % 4;
+
+    // For lint to apply the constant must always be on the RHS of the previous value for %
+    let _: i32 = 4 % ((value % 4) + 4);
+    let _: i32 = ((4 % value) + 4) % 4;
+
+    // Lint in internal macros
+    internal_rem_euclid!();
+
+    // Do not lint in external macros
+    manual_rem_euclid!();
+}
+
+// Should lint for params too
+pub fn rem_euclid_4(num: i32) -> i32 {
+    ((num % 4) + 4) % 4
+}
+
+// Constant version came later, should still lint
+pub const fn const_rem_euclid_4(num: i32) -> i32 {
+    ((num % 4) + 4) % 4
+}

tests/ui/manual_rem_euclid.stderr

@@ -0,0 +1,57 @@
+error: manual `rem_euclid` implementation
+  --> $DIR/manual_rem_euclid.rs:19:18
+   |
+LL |     let _: i32 = ((value % 4) + 4) % 4;
+   |                  ^^^^^^^^^^^^^^^^^^^^^ help: consider using: `value.rem_euclid(4)`
+   |
+   = note: `-D clippy::manual-rem-euclid` implied by `-D warnings`
+
+error: manual `rem_euclid` implementation
+  --> $DIR/manual_rem_euclid.rs:20:18
+   |
+LL |     let _: i32 = (4 + (value % 4)) % 4;
+   |                  ^^^^^^^^^^^^^^^^^^^^^ help: consider using: `value.rem_euclid(4)`
+
+error: manual `rem_euclid` implementation
+  --> $DIR/manual_rem_euclid.rs:21:18
+   |
+LL |     let _: i32 = (value % 4 + 4) % 4;
+   |                  ^^^^^^^^^^^^^^^^^^^ help: consider using: `value.rem_euclid(4)`
+
+error: manual `rem_euclid` implementation
+  --> $DIR/manual_rem_euclid.rs:22:18
+   |
+LL |     let _: i32 = (4 + value % 4) % 4;
+   |                  ^^^^^^^^^^^^^^^^^^^ help: consider using: `value.rem_euclid(4)`
+
+error: manual `rem_euclid` implementation
+  --> $DIR/manual_rem_euclid.rs:23:22
+   |
+LL |     let _: i32 = 1 + (4 + value % 4) % 4;
+   |                      ^^^^^^^^^^^^^^^^^^^ help: consider using: `value.rem_euclid(4)`
+
+error: manual `rem_euclid` implementation
+  --> $DIR/manual_rem_euclid.rs:12:22
+   |
+LL |         let _: i32 = ((value % 4) + 4) % 4;
+   |                      ^^^^^^^^^^^^^^^^^^^^^ help: consider using: `value.rem_euclid(4)`
+...
+LL |     internal_rem_euclid!();
+   |     ---------------------- in this macro invocation
+   |
+   = note: this error originates in the macro `internal_rem_euclid` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: manual `rem_euclid` implementation
+  --> $DIR/manual_rem_euclid.rs:49:5
+   |
+LL |     ((num % 4) + 4) % 4
+   |     ^^^^^^^^^^^^^^^^^^^ help: consider using: `num.rem_euclid(4)`
+
+error: manual `rem_euclid` implementation
+  --> $DIR/manual_rem_euclid.rs:54:5
+   |
+LL |     ((num % 4) + 4) % 4
+   |     ^^^^^^^^^^^^^^^^^^^ help: consider using: `num.rem_euclid(4)`
+
+error: aborting due to 8 previous errors
+

tests/ui/min_rust_version_attr.rs

@@ -155,6 +155,11 @@ fn cast_abs_to_unsigned() {
     assert_eq!(10u32, x.abs() as u32);
 }
 
+fn manual_rem_euclid() {
+    let x: i32 = 10;
+    let _: i32 = ((x % 4) + 4) % 4;
+}
+
 fn main() {
     filter_map_next();
     checked_conversion();
@@ -174,6 +179,7 @@ fn main() {
     int_from_bool();
     err_expect();
     cast_abs_to_unsigned();
+    manual_rem_euclid();
 }
 
 mod just_under_msrv {
@@ -211,3 +217,12 @@ fn main() {
         }
     }
 }
+
+mod const_rem_euclid {
+    #![feature(custom_inner_attributes)]
+    #![clippy::msrv = "1.50.0"]
+
+    pub const fn const_rem_euclid_4(num: i32) -> i32 {
+        ((num % 4) + 4) % 4
+    }
+}

tests/ui/min_rust_version_attr.stderr

@@ -1,12 +1,12 @@
 error: stripping a prefix manually
-  --> $DIR/min_rust_version_attr.rs:198:24
+  --> $DIR/min_rust_version_attr.rs:204:24
    |
 LL |             assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!");
    |                        ^^^^^^^^^^^^^^^^^^^^
    |
    = note: `-D clippy::manual-strip` implied by `-D warnings`
 note: the prefix was tested here
-  --> $DIR/min_rust_version_attr.rs:197:9
+  --> $DIR/min_rust_version_attr.rs:203:9
    |
 LL |         if s.starts_with("hello, ") {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -17,13 +17,13 @@ LL ~             assert_eq!(<stripped>.to_uppercase(), "WORLD!");
    |
 
 error: stripping a prefix manually
-  --> $DIR/min_rust_version_attr.rs:210:24
+  --> $DIR/min_rust_version_attr.rs:216:24
    |
 LL |             assert_eq!(s["hello, ".len()..].to_uppercase(), "WORLD!");
    |                        ^^^^^^^^^^^^^^^^^^^^
    |
 note: the prefix was tested here
-  --> $DIR/min_rust_version_attr.rs:209:9
+  --> $DIR/min_rust_version_attr.rs:215:9
    |
 LL |         if s.starts_with("hello, ") {
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^