Initial implementation result_large_err

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/lib.register_all.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_lints.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_perf.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.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/utils/conf.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_utils/src/ty.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,
+ }
+}

tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr

@@ -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