Rewrite EndianBytes lint pass (#17363)

Changes:

* Check the method name before type checking.
* Don't use `get_def_path` for `format!` when linting.
* Check that the `from_*_bytes` method used is defined on one of the
primitives.
* Add suggestions for the other endian conversions if applicable.
* Lint in the context of the method name.
* Lint all uses of the methods, not just calls.
* Split the tests into one file per lint.

The old test file broke `ui_test` so something had to be done. It was
also hard to actually check if it was correct with all the repeated
macro expansions.

changelog: [`big_endian_bytes`], [`host_endian_bytes`],
[`little_endian_bytes`]: Lint any time the methods are named instead of
just when called.
changelog: [`big_endian_bytes`], [`host_endian_bytes`],
[`little_endian_bytes`]: Lint when `to_*_bytes` is called via UFCS.
changelog: [`big_endian_bytes`], [`host_endian_bytes`],
[`little_endian_bytes`]: Always lint when the method name comes from the
current crate.

Author: llogiq

clippy_lints/src/endian_bytes.rs

@@ -1,12 +1,12 @@
-use crate::Lint;
 use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::res::{MaybeDef as _, MaybeTypeckRes as _};
 use clippy_utils::{is_lint_allowed, sym};
-use rustc_hir::{Expr, ExprKind};
-use rustc_lint::{LateContext, LateLintPass, LintContext as _};
-use rustc_middle::ty::Ty;
+use core::ptr;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind, QPath};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty;
 use rustc_session::declare_lint_pass;
-use rustc_span::Symbol;
-use std::fmt::Write as _;
 
 declare_clippy_lint! {
     /// ### What it does
@@ -69,133 +69,67 @@ declare_lint_pass!(EndianBytes => [
     LITTLE_ENDIAN_BYTES,
 ]);
 
-const HOST_NAMES: [Symbol; 2] = [sym::from_ne_bytes, sym::to_ne_bytes];
-const LITTLE_NAMES: [Symbol; 2] = [sym::from_le_bytes, sym::to_le_bytes];
-const BIG_NAMES: [Symbol; 2] = [sym::from_be_bytes, sym::to_be_bytes];
-
-#[derive(Clone, Debug)]
-enum LintKind {
-    Host,
-    Little,
-    Big,
-}
-
-#[derive(Clone, Copy, PartialEq)]
-enum Prefix {
+#[derive(Clone, Copy)]
+enum Direction {
     From,
     To,
 }
 
-impl LintKind {
-    fn allowed(&self, cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
-        is_lint_allowed(cx, self.as_lint(), expr.hir_id)
-    }
-
-    fn as_lint(&self) -> &'static Lint {
-        match self {
-            LintKind::Host => HOST_ENDIAN_BYTES,
-            LintKind::Little => LITTLE_ENDIAN_BYTES,
-            LintKind::Big => BIG_ENDIAN_BYTES,
-        }
-    }
-
-    fn as_name(&self, prefix: Prefix) -> Symbol {
-        let index = usize::from(prefix == Prefix::To);
-
-        match self {
-            LintKind::Host => HOST_NAMES[index],
-            LintKind::Little => LITTLE_NAMES[index],
-            LintKind::Big => BIG_NAMES[index],
-        }
-    }
-}
-
 impl LateLintPass<'_> for EndianBytes {
-    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
-        let (prefix, name, ty_expr) = match expr.kind {
-            ExprKind::MethodCall(method_name, receiver, [], ..) => (Prefix::To, method_name.ident.name, receiver),
-            ExprKind::Call(function, ..)
-                if let ExprKind::Path(qpath) = function.kind
-                    && let Some(def_id) = cx.qpath_res(&qpath, function.hir_id).opt_def_id()
-                    && let Some(function_name) = cx.get_def_path(def_id).last() =>
-            {
-                (Prefix::From, *function_name, expr)
+    fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) {
+        let (sp, direction, lint, msg) = match e.kind {
+            // rustfmt wants to break each arm into one line per tuple element which
+            // really hurts readability.
+            #[rustfmt::skip]
+            ExprKind::MethodCall(seg, _, [], _) => match seg.ident.name {
+                sym::to_ne_bytes => (seg.ident.span, Direction::To, HOST_ENDIAN_BYTES, "use of `to_ne_bytes`"),
+                sym::to_le_bytes => (seg.ident.span, Direction::To, LITTLE_ENDIAN_BYTES, "use of `to_le_bytes`"),
+                sym::to_be_bytes => (seg.ident.span, Direction::To, BIG_ENDIAN_BYTES, "use of `to_be_bytes`"),
+                _ => return,
+            },
+            #[rustfmt::skip]
+            ExprKind::Path(QPath::TypeRelative(_, seg)) => match seg.ident.name {
+                sym::from_ne_bytes => (seg.ident.span, Direction::From, HOST_ENDIAN_BYTES, "use of `from_ne_bytes`"),
+                sym::from_le_bytes => (seg.ident.span, Direction::From, LITTLE_ENDIAN_BYTES, "use of `from_le_bytes`"),
+                sym::from_be_bytes => (seg.ident.span, Direction::From, BIG_ENDIAN_BYTES, "use of `from_be_bytes`"),
+                sym::to_ne_bytes => (seg.ident.span, Direction::To, HOST_ENDIAN_BYTES, "use of `to_ne_bytes`"),
+                sym::to_le_bytes => (seg.ident.span, Direction::To, LITTLE_ENDIAN_BYTES, "use of `to_le_bytes`"),
+                sym::to_be_bytes => (seg.ident.span, Direction::To, BIG_ENDIAN_BYTES, "use of `to_be_bytes`"),
+                _ => return,
             },
             _ => return,
         };
-        if !expr.span.in_external_macro(cx.sess().source_map())
-            && let ty = cx.typeck_results().expr_ty(ty_expr)
-            && ty.is_primitive_ty()
+        if let Some(ty) = cx.ty_based_def(e.hir_id).opt_parent(cx).opt_impl_ty(cx)
+            && let ty::Uint(_) | ty::Int(_) | ty::Float(_) = *ty.instantiate_identity().skip_normalization().kind()
+            // Only check where the name itself comes from. The point of the lints is to
+            // catch when the wrong byte order is used so we only care if the current crate
+            // decided on the byte order. Which crate actually assembled the path/call
+            // isn't relevant for these lints.
+            && !sp.in_external_macro(cx.tcx.sess.source_map())
         {
-            maybe_lint_endian_bytes(cx, expr, prefix, name, ty);
-        }
-    }
-}
-
-fn maybe_lint_endian_bytes(cx: &LateContext<'_>, expr: &Expr<'_>, prefix: Prefix, name: Symbol, ty: Ty<'_>) {
-    let ne = LintKind::Host.as_name(prefix);
-    let le = LintKind::Little.as_name(prefix);
-    let be = LintKind::Big.as_name(prefix);
-
-    let (lint, other_lints) = match name {
-        name if name == ne => ((&LintKind::Host), [(&LintKind::Little), (&LintKind::Big)]),
-        name if name == le => ((&LintKind::Little), [(&LintKind::Host), (&LintKind::Big)]),
-        name if name == be => ((&LintKind::Big), [(&LintKind::Host), (&LintKind::Little)]),
-        _ => return,
-    };
-
-    span_lint_and_then(
-        cx,
-        lint.as_lint(),
-        expr.span,
-        format!(
-            "usage of the {}`{ty}::{}`{}",
-            if prefix == Prefix::From { "function " } else { "" },
-            lint.as_name(prefix),
-            if prefix == Prefix::To { " method" } else { "" },
-        ),
-        move |diag| {
-            // all lints disallowed, don't give help here
-            if [&[lint], other_lints.as_slice()]
-                .concat()
-                .iter()
-                .all(|lint| !lint.allowed(cx, expr))
-            {
-                return;
-            }
-
-            // ne_bytes and all other lints allowed
-            if lint.as_name(prefix) == ne && other_lints.iter().all(|lint| lint.allowed(cx, expr)) {
-                diag.help("specify the desired endianness explicitly");
-                return;
-            }
-
-            // le_bytes where ne_bytes allowed but be_bytes is not, or le_bytes where ne_bytes allowed but
-            // le_bytes is not
-            if (lint.as_name(prefix) == le || lint.as_name(prefix) == be) && LintKind::Host.allowed(cx, expr) {
-                diag.help("use the native endianness instead");
-                return;
-            }
-
-            let allowed_lints = other_lints.iter().filter(|lint| lint.allowed(cx, expr));
-            let len = allowed_lints.clone().count();
-
-            let mut help_str = "use ".to_owned();
-
-            for (i, lint) in allowed_lints.enumerate() {
-                let only_one = len == 1;
-                if !only_one {
-                    help_str.push_str("either of ");
+            span_lint_and_then(cx, lint, sp, msg, |diag| {
+                if !ptr::addr_eq(lint, HOST_ENDIAN_BYTES) && is_lint_allowed(cx, HOST_ENDIAN_BYTES, e.hir_id) {
+                    let (msg, sugg) = match direction {
+                        Direction::From => ("convert from native endian", "from_ne_bytes"),
+                        Direction::To => ("convert to native endian", "to_ne_bytes"),
+                    };
+                    diag.span_suggestion(sp, msg, sugg, Applicability::MaybeIncorrect);
                 }
-
-                write!(help_str, "`{ty}::{}` ", lint.as_name(prefix)).unwrap();
-
-                if i != len && !only_one {
-                    help_str.push_str("or ");
+                if !ptr::addr_eq(lint, LITTLE_ENDIAN_BYTES) && is_lint_allowed(cx, LITTLE_ENDIAN_BYTES, e.hir_id) {
+                    let (msg, sugg) = match direction {
+                        Direction::From => ("convert from little endian", "from_le_bytes"),
+                        Direction::To => ("convert to little endian", "to_le_bytes"),
+                    };
+                    diag.span_suggestion(sp, msg, sugg, Applicability::MaybeIncorrect);
                 }
-            }
-            help_str.push_str("instead");
-            diag.help(help_str);
-        },
-    );
+                if !ptr::addr_eq(lint, BIG_ENDIAN_BYTES) && is_lint_allowed(cx, BIG_ENDIAN_BYTES, e.hir_id) {
+                    let (msg, sugg) = match direction {
+                        Direction::From => ("convert from big endian", "from_be_bytes"),
+                        Direction::To => ("convert to big endian", "to_be_bytes"),
+                    };
+                    diag.span_suggestion(sp, msg, sugg, Applicability::MaybeIncorrect);
+                }
+            });
+        }
+    }
 }

clippy_lints/src/lib.rs

@@ -418,7 +418,7 @@ mod zombie_processes;
 use clippy_config::{Conf, get_configuration_metadata, sanitize_explanation};
 use clippy_utils::macros::FormatArgsStorage;
 use rustc_data_structures::fx::FxHashSet;
-use rustc_lint::{Lint, is_lint_pass_required};
+use rustc_lint::is_lint_pass_required;
 use rustc_middle::ty::TyCtxt;
 use utils::attr_collector::AttrStorage;
 

clippy_utils/src/res.rs

@@ -339,6 +339,8 @@ impl<'tcx, T: Copy + MaybeQPath<'tcx>> MaybeQPath<'tcx> for &Option<T> {
 /// A resolved path and the explicit `Self` type if there is one.
 type OptResPath<'tcx> = (Option<&'tcx hir::Ty<'tcx>>, Option<&'tcx Path<'tcx>>);
 
+type OptTyRelPath<'tcx> = Option<(&'tcx hir::Ty<'tcx>, &'tcx PathSegment<'tcx>)>;
+
 /// A HIR node which might be a `QPath::Resolved`.
 ///
 /// The following are resolved paths:
@@ -354,6 +356,10 @@ pub trait MaybeResPath<'a>: Copy {
     /// type associated with it.
     fn opt_res_path(self) -> OptResPath<'a>;
 
+    /// If this node is a type relative path gets both the type and the final
+    /// segments of the path.
+    fn opt_ty_rel_path(self) -> OptTyRelPath<'a>;
+
     /// If this node is a resolved path gets it's resolution. Returns `Res::Err`
     /// otherwise.
     #[inline]
@@ -391,6 +397,11 @@ impl<'a> MaybeResPath<'a> for &'a Path<'a> {
         (None, Some(self))
     }
 
+    #[inline]
+    fn opt_ty_rel_path(self) -> OptTyRelPath<'a> {
+        None
+    }
+
     #[inline]
     fn basic_res(self) -> &'a Res {
         &self.res
@@ -404,6 +415,14 @@ impl<'a> MaybeResPath<'a> for &QPath<'a> {
             QPath::TypeRelative(..) => (None, None),
         }
     }
+
+    #[inline]
+    fn opt_ty_rel_path(self) -> OptTyRelPath<'a> {
+        match *self {
+            QPath::TypeRelative(ty, seg) => Some((ty, seg)),
+            QPath::Resolved(..) => None,
+        }
+    }
 }
 impl<'a> MaybeResPath<'a> for &Expr<'a> {
     #[inline]
@@ -413,6 +432,14 @@ impl<'a> MaybeResPath<'a> for &Expr<'a> {
             _ => (None, None),
         }
     }
+
+    #[inline]
+    fn opt_ty_rel_path(self) -> OptTyRelPath<'a> {
+        match &self.kind {
+            ExprKind::Path(qpath) => qpath.opt_ty_rel_path(),
+            _ => None,
+        }
+    }
 }
 impl<'a> MaybeResPath<'a> for &PatExpr<'a> {
     #[inline]
@@ -422,6 +449,14 @@ impl<'a> MaybeResPath<'a> for &PatExpr<'a> {
             PatExprKind::Lit { .. } => (None, None),
         }
     }
+
+    #[inline]
+    fn opt_ty_rel_path(self) -> OptTyRelPath<'a> {
+        match &self.kind {
+            PatExprKind::Path(qpath) => qpath.opt_ty_rel_path(),
+            PatExprKind::Lit { .. } => None,
+        }
+    }
 }
 impl<'a, AmbigArg> MaybeResPath<'a> for &hir::Ty<'a, AmbigArg> {
     #[inline]
@@ -431,6 +466,14 @@ impl<'a, AmbigArg> MaybeResPath<'a> for &hir::Ty<'a, AmbigArg> {
             _ => (None, None),
         }
     }
+
+    #[inline]
+    fn opt_ty_rel_path(self) -> OptTyRelPath<'a> {
+        match &self.kind {
+            TyKind::Path(qpath) => qpath.opt_ty_rel_path(),
+            _ => None,
+        }
+    }
 }
 impl<'a> MaybeResPath<'a> for &Pat<'a> {
     #[inline]
@@ -440,6 +483,14 @@ impl<'a> MaybeResPath<'a> for &Pat<'a> {
             _ => (None, None),
         }
     }
+
+    #[inline]
+    fn opt_ty_rel_path(self) -> OptTyRelPath<'a> {
+        match self.kind {
+            PatKind::Expr(e) => e.opt_ty_rel_path(),
+            _ => None,
+        }
+    }
 }
 impl<'a, T: MaybeResPath<'a>> MaybeResPath<'a> for Option<T> {
     #[inline]
@@ -450,6 +501,11 @@ impl<'a, T: MaybeResPath<'a>> MaybeResPath<'a> for Option<T> {
         }
     }
 
+    #[inline]
+    fn opt_ty_rel_path(self) -> OptTyRelPath<'a> {
+        self.and_then(T::opt_ty_rel_path)
+    }
+
     #[inline]
     fn basic_res(self) -> &'a Res {
         self.map_or(&Res::Err, T::basic_res)