Initial implementation of result_large_err

Author: lukaslueg

CHANGELOG.md

@@ -4025,6 +4025,7 @@ Released 2018-09-13
 [`replace_consts`]: https://rust-lang.github.io/rust-clippy/master/index.html#replace_consts
 [`rest_pat_in_fully_bound_structs`]: https://rust-lang.github.io/rust-clippy/master/index.html#rest_pat_in_fully_bound_structs
 [`result_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_expect_used
+[`result_large_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_large_err
 [`result_map_or_into_option`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_or_into_option
 [`result_map_unit_fn`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unit_fn
 [`result_map_unwrap_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#result_map_unwrap_or_else

clippy_lints/src/functions/mod.rs

@@ -1,6 +1,6 @@
 mod must_use;
 mod not_unsafe_ptr_arg_deref;
-mod result_unit_err;
+mod result;
 mod too_many_arguments;
 mod too_many_lines;
 
@@ -217,17 +217,62 @@ declare_clippy_lint! {
     "public function returning `Result` with an `Err` type of `()`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for functions that return `Result` with an unusually large
+    /// `Err`-variant.
+    ///
+    /// ### Why is this bad?
+    /// A `Result` is at least as large as the `Err`-variant. While we
+    /// expect that variant to be seldomly used, the compiler needs to reserve
+    /// and move that much memory every single time.
+    ///
+    /// ### Known problems
+    /// The size determined by Clippy is platform-dependent.
+    ///
+    /// ### Examples
+    /// ```rust
+    /// pub enum ParseError {
+    ///     UnparsedBytes([u8; 512]),
+    ///     UnexpectedEof,
+    /// }
+    ///
+    /// // The `Result` has at least 512 bytes, even in the `Ok`-case
+    /// pub fn parse() -> Result<(), ParseError> {
+    ///     Ok(())
+    /// }
+    /// ```
+    /// should be
+    /// ```
+    /// pub enum ParseError {
+    ///     UnparsedBytes(Box<[u8; 512]>),
+    ///     UnexpectedEof,
+    /// }
+    ///
+    /// // The `Result` is slightly larger than a pointer
+    /// pub fn parse() -> Result<(), ParseError> {
+    ///     Ok(())
+    /// }
+    /// ```
+    #[clippy::version = "1.64.0"]
+    pub RESULT_LARGE_ERR,
+    perf,
+    "function returning `Result` with large `Err` type"
+}
+
 #[derive(Copy, Clone)]
 pub struct Functions {
     too_many_arguments_threshold: u64,
     too_many_lines_threshold: u64,
+    large_error_threshold: u64,
 }
 
 impl Functions {
-    pub fn new(too_many_arguments_threshold: u64, too_many_lines_threshold: u64) -> Self {
+    pub fn new(too_many_arguments_threshold: u64, too_many_lines_threshold: u64, large_error_threshold: u64) -> Self {
         Self {
             too_many_arguments_threshold,
             too_many_lines_threshold,
+            large_error_threshold,
         }
     }
 }
@@ -240,6 +285,7 @@ impl_lint_pass!(Functions => [
     DOUBLE_MUST_USE,
     MUST_USE_CANDIDATE,
     RESULT_UNIT_ERR,
+    RESULT_LARGE_ERR,
 ]);
 
 impl<'tcx> LateLintPass<'tcx> for Functions {
@@ -259,18 +305,18 @@ impl<'tcx> LateLintPass<'tcx> for Functions {
 
     fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) {
         must_use::check_item(cx, item);
-        result_unit_err::check_item(cx, item);
+        result::check_item(cx, item, self.large_error_threshold);
     }
 
     fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) {
         must_use::check_impl_item(cx, item);
-        result_unit_err::check_impl_item(cx, item);
+        result::check_impl_item(cx, item, self.large_error_threshold);
     }
 
     fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) {
         too_many_arguments::check_trait_item(cx, item, self.too_many_arguments_threshold);
         not_unsafe_ptr_arg_deref::check_trait_item(cx, item);
         must_use::check_trait_item(cx, item);
-        result_unit_err::check_trait_item(cx, item);
+        result::check_trait_item(cx, item, self.large_error_threshold);
     }
 }

clippy_lints/src/functions/result.rs

@@ -0,0 +1,100 @@
+use rustc_errors::Diagnostic;
+use rustc_hir as hir;
+use rustc_lint::{LateContext, LintContext};
+use rustc_middle::lint::in_external_macro;
+use rustc_middle::ty::{self, Ty};
+use rustc_span::{sym, Span};
+use rustc_typeck::hir_ty_to_ty;
+
+use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_then};
+use clippy_utils::trait_ref_of_method;
+use clippy_utils::ty::{approx_ty_size, is_type_diagnostic_item};
+
+use super::{RESULT_LARGE_ERR, RESULT_UNIT_ERR};
+
+/// The type of the `Err`-variant in a `std::result::Result` returned by the
+/// given `FnDecl`
+fn result_err_ty<'tcx>(
+    cx: &LateContext<'tcx>,
+    decl: &hir::FnDecl<'tcx>,
+    item_span: Span,
+) -> Option<(&'tcx hir::Ty<'tcx>, Ty<'tcx>)> {
+    if !in_external_macro(cx.sess(), item_span)
+        && let hir::FnRetTy::Return(hir_ty) = decl.output
+        && let ty = hir_ty_to_ty(cx.tcx, hir_ty)
+        && is_type_diagnostic_item(cx, ty, sym::Result)
+        && let ty::Adt(_, substs) = ty.kind()
+    {
+        let err_ty = substs.type_at(1);
+        Some((hir_ty, err_ty))
+    } else {
+        None
+    }
+}
+
+pub(super) fn check_item<'tcx>(cx: &LateContext<'tcx>, item: &hir::Item<'tcx>, large_err_threshold: u64) {
+    if let hir::ItemKind::Fn(ref sig, _generics, _) = item.kind
+        && let Some((hir_ty, err_ty)) = result_err_ty(cx, sig.decl, item.span)
+    {
+        if cx.access_levels.is_exported(item.def_id) {
+            let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
+            check_result_unit_err(cx, err_ty, fn_header_span);
+        }
+        check_result_large_err(cx, err_ty, hir_ty.span, large_err_threshold);
+    }
+}
+
+pub(super) fn check_impl_item<'tcx>(cx: &LateContext<'tcx>, item: &hir::ImplItem<'tcx>, large_err_threshold: u64) {
+    // Don't lint if method is a trait's implementation, we can't do anything about those
+    if let hir::ImplItemKind::Fn(ref sig, _) = item.kind
+        && let Some((hir_ty, err_ty)) = result_err_ty(cx, sig.decl, item.span)
+        && trait_ref_of_method(cx, item.def_id).is_none()
+    {
+        if cx.access_levels.is_exported(item.def_id) {
+            let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
+            check_result_unit_err(cx, err_ty, fn_header_span);
+        }
+        check_result_large_err(cx, err_ty, hir_ty.span, large_err_threshold);
+    }
+}
+
+pub(super) fn check_trait_item<'tcx>(cx: &LateContext<'tcx>, item: &hir::TraitItem<'tcx>, large_err_threshold: u64) {
+    if let hir::TraitItemKind::Fn(ref sig, _) = item.kind {
+        let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
+        if let Some((hir_ty, err_ty)) = result_err_ty(cx, sig.decl, item.span) {
+            if cx.access_levels.is_exported(item.def_id) {
+                check_result_unit_err(cx, err_ty, fn_header_span);
+            }
+            check_result_large_err(cx, err_ty, hir_ty.span, large_err_threshold);
+        }
+    }
+}
+
+fn check_result_unit_err(cx: &LateContext<'_>, err_ty: Ty<'_>, fn_header_span: Span) {
+    if err_ty.is_unit() {
+        span_lint_and_help(
+            cx,
+            RESULT_UNIT_ERR,
+            fn_header_span,
+            "this returns a `Result<_, ()>`",
+            None,
+            "use a custom `Error` type instead",
+        );
+    }
+}
+
+fn check_result_large_err<'tcx>(cx: &LateContext<'tcx>, err_ty: Ty<'tcx>, hir_ty_span: Span, large_err_threshold: u64) {
+    let ty_size = approx_ty_size(cx, err_ty);
+    if ty_size >= large_err_threshold {
+        span_lint_and_then(
+            cx,
+            RESULT_LARGE_ERR,
+            hir_ty_span,
+            "the `Err`-variant returned from this function is very large",
+            |diag: &mut Diagnostic| {
+                diag.span_label(hir_ty_span, format!("the `Err`-variant is at least {ty_size} bytes"));
+                diag.help(format!("try reducing the size of `{err_ty}`, for example by boxing large elements or replacing it with `Box<{err_ty}>`"));
+            },
+        );
+    }
+}

clippy_lints/src/functions/result_unit_err.rs

@@ -80,6 +80,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(functions::DOUBLE_MUST_USE),
     LintId::of(functions::MUST_USE_UNIT),
     LintId::of(functions::NOT_UNSAFE_PTR_ARG_DEREF),
+    LintId::of(functions::RESULT_LARGE_ERR),
     LintId::of(functions::RESULT_UNIT_ERR),
     LintId::of(functions::TOO_MANY_ARGUMENTS),
     LintId::of(if_let_mutex::IF_LET_MUTEX),

clippy_lints/src/lib.register_all.rs

@@ -171,6 +171,7 @@ store.register_lints(&[
     functions::MUST_USE_CANDIDATE,
     functions::MUST_USE_UNIT,
     functions::NOT_UNSAFE_PTR_ARG_DEREF,
+    functions::RESULT_LARGE_ERR,
     functions::RESULT_UNIT_ERR,
     functions::TOO_MANY_ARGUMENTS,
     functions::TOO_MANY_LINES,

clippy_lints/src/lib.register_lints.rs

@@ -7,6 +7,7 @@ store.register_group(true, "clippy::perf", Some("clippy_perf"), vec![
     LintId::of(escape::BOXED_LOCAL),
     LintId::of(format_args::FORMAT_IN_FORMAT_ARGS),
     LintId::of(format_args::TO_STRING_IN_FORMAT_ARGS),
+    LintId::of(functions::RESULT_LARGE_ERR),
     LintId::of(large_const_arrays::LARGE_CONST_ARRAYS),
     LintId::of(large_enum_variant::LARGE_ENUM_VARIANT),
     LintId::of(loops::MANUAL_MEMCPY),

clippy_lints/src/lib.register_perf.rs

@@ -668,10 +668,12 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(move || Box::new(disallowed_names::DisallowedNames::new(disallowed_names.clone())));
     let too_many_arguments_threshold = conf.too_many_arguments_threshold;
     let too_many_lines_threshold = conf.too_many_lines_threshold;
+    let large_error_threshold = conf.large_error_threshold;
     store.register_late_pass(move || {
         Box::new(functions::Functions::new(
             too_many_arguments_threshold,
             too_many_lines_threshold,
+            large_error_threshold,
         ))
     });
     let doc_valid_idents = conf.doc_valid_idents.iter().cloned().collect::<FxHashSet<_>>();

clippy_lints/src/lib.rs

@@ -379,6 +379,10 @@ define_Conf! {
     ///
     /// Whether `dbg!` should be allowed in test functions
     (allow_dbg_in_tests: bool = false),
+    /// Lint: RESULT_LARGE_ERR
+    ///
+    /// The maximum size of the `Err`-variant in a `Result` returned from a function
+    (large_error_threshold: u64 = 128),
 }
 
 /// Search for the configuration file.

clippy_lints/src/utils/conf.rs

@@ -831,3 +831,53 @@ pub fn ty_is_fn_once_param<'tcx>(tcx: TyCtxt<'_>, ty: Ty<'tcx>, predicates: &'tc
         })
         .unwrap_or(false)
 }
+
+/// Comes up with an "at least" guesstimate for the type's size, not taking into
+/// account the layout of type parameters.
+pub fn approx_ty_size<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> u64 {
+    use rustc_middle::ty::layout::LayoutOf;
+    if !is_normalizable(cx, cx.param_env, ty) {
+        return 0;
+    }
+    match (cx.layout_of(ty).map(|layout| layout.size.bytes()), ty.kind()) {
+        (Ok(size), _) => size,
+        (Err(_), ty::Tuple(list)) => list.as_substs().types().map(|t| approx_ty_size(cx, t)).sum(),
+        (Err(_), ty::Array(t, n)) => {
+            n.try_eval_usize(cx.tcx, cx.param_env).unwrap_or_default() * approx_ty_size(cx, *t)
+        },
+        (Err(_), ty::Adt(def, subst)) if def.is_struct() => def
+            .variants()
+            .iter()
+            .map(|v| {
+                v.fields
+                    .iter()
+                    .map(|field| approx_ty_size(cx, field.ty(cx.tcx, subst)))
+                    .sum::<u64>()
+            })
+            .sum(),
+        (Err(_), ty::Adt(def, subst)) if def.is_enum() => def
+            .variants()
+            .iter()
+            .map(|v| {
+                v.fields
+                    .iter()
+                    .map(|field| approx_ty_size(cx, field.ty(cx.tcx, subst)))
+                    .sum::<u64>()
+            })
+            .max()
+            .unwrap_or_default(),
+        (Err(_), ty::Adt(def, subst)) if def.is_union() => def
+            .variants()
+            .iter()
+            .map(|v| {
+                v.fields
+                    .iter()
+                    .map(|field| approx_ty_size(cx, field.ty(cx.tcx, subst)))
+                    .max()
+                    .unwrap_or_default()
+            })
+            .max()
+            .unwrap_or_default(),
+        (Err(_), _) => 0,
+    }
+}

clippy_utils/src/ty.rs

@@ -19,6 +19,7 @@ error: error reading Clippy's configuration file `$DIR/clippy.toml`: unknown fie
            enforced-import-renames
            enum-variant-name-threshold
            enum-variant-size-threshold
+           large-error-threshold
            literal-representation-threshold
            max-fn-params-bools
            max-include-file-size