diff options
| -rw-r--r-- | clippy_lints/src/functions.rs | 738 | ||||
| -rw-r--r-- | clippy_lints/src/functions/mod.rs | 267 | ||||
| -rw-r--r-- | clippy_lints/src/functions/must_use.rs | 272 | ||||
| -rw-r--r-- | clippy_lints/src/functions/not_unsafe_ptr_arg_deref.rs | 125 | ||||
| -rw-r--r-- | clippy_lints/src/functions/result_unit_err.rs | 66 | ||||
| -rw-r--r-- | clippy_lints/src/functions/too_many_arguments.rs | 73 | ||||
| -rw-r--r-- | clippy_lints/src/functions/too_many_lines.rs | 68 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 6 |
8 files changed, 874 insertions, 741 deletions
diff --git a/clippy_lints/src/functions.rs b/clippy_lints/src/functions.rs deleted file mode 100644 index 730492fc7e3..00000000000 --- a/clippy_lints/src/functions.rs +++ /dev/null @@ -1,738 +0,0 @@ -use clippy_utils::diagnostics::{span_lint, span_lint_and_help, span_lint_and_then}; -use clippy_utils::source::{snippet, snippet_opt}; -use clippy_utils::ty::{is_must_use_ty, is_type_diagnostic_item, type_is_unsafe_function}; -use clippy_utils::{ - attr_by_name, attrs::is_proc_macro, is_trait_impl_item, iter_input_pats, match_def_path, must_use_attr, - path_to_local, return_ty, trait_ref_of_method, -}; -use if_chain::if_chain; -use rustc_ast::ast::Attribute; -use rustc_data_structures::fx::FxHashSet; -use rustc_errors::Applicability; -use rustc_hir as hir; -use rustc_hir::intravisit; -use rustc_hir::{def::Res, def_id::DefId, QPath}; -use rustc_lint::{LateContext, LateLintPass, LintContext}; -use rustc_middle::hir::map::Map; -use rustc_middle::lint::in_external_macro; -use rustc_middle::ty::{self, Ty}; -use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::source_map::Span; -use rustc_span::sym; -use rustc_target::spec::abi::Abi; -use rustc_typeck::hir_ty_to_ty; - -declare_clippy_lint! { - /// **What it does:** Checks for functions with too many parameters. - /// - /// **Why is this bad?** Functions with lots of parameters are considered bad - /// style and reduce readability (“what does the 5th parameter mean?”). Consider - /// grouping some parameters into a new type. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```rust - /// # struct Color; - /// fn foo(x: u32, y: u32, name: &str, c: Color, w: f32, h: f32, a: f32, b: f32) { - /// // .. - /// } - /// ``` - pub TOO_MANY_ARGUMENTS, - complexity, - "functions with too many arguments" -} - -declare_clippy_lint! { - /// **What it does:** Checks for functions with a large amount of lines. - /// - /// **Why is this bad?** Functions with a lot of lines are harder to understand - /// due to having to look at a larger amount of code to understand what the - /// function is doing. Consider splitting the body of the function into - /// multiple functions. - /// - /// **Known problems:** None. - /// - /// **Example:** - /// ```rust - /// fn im_too_long() { - /// println!(""); - /// // ... 100 more LoC - /// println!(""); - /// } - /// ``` - pub TOO_MANY_LINES, - pedantic, - "functions with too many lines" -} - -declare_clippy_lint! { - /// **What it does:** Checks for public functions that dereference raw pointer - /// arguments but are not marked unsafe. - /// - /// **Why is this bad?** The function should probably be marked `unsafe`, since - /// for an arbitrary raw pointer, there is no way of telling for sure if it is - /// valid. - /// - /// **Known problems:** - /// - /// * It does not check functions recursively so if the pointer is passed to a - /// private non-`unsafe` function which does the dereferencing, the lint won't - /// trigger. - /// * It only checks for arguments whose type are raw pointers, not raw pointers - /// got from an argument in some other way (`fn foo(bar: &[*const u8])` or - /// `some_argument.get_raw_ptr()`). - /// - /// **Example:** - /// ```rust,ignore - /// // Bad - /// pub fn foo(x: *const u8) { - /// println!("{}", unsafe { *x }); - /// } - /// - /// // Good - /// pub unsafe fn foo(x: *const u8) { - /// println!("{}", unsafe { *x }); - /// } - /// ``` - pub NOT_UNSAFE_PTR_ARG_DEREF, - correctness, - "public functions dereferencing raw pointer arguments but not marked `unsafe`" -} - -declare_clippy_lint! { - /// **What it does:** Checks for a [`#[must_use]`] attribute on - /// unit-returning functions and methods. - /// - /// [`#[must_use]`]: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute - /// - /// **Why is this bad?** Unit values are useless. The attribute is likely - /// a remnant of a refactoring that removed the return type. - /// - /// **Known problems:** None. - /// - /// **Examples:** - /// ```rust - /// #[must_use] - /// fn useless() { } - /// ``` - pub MUST_USE_UNIT, - style, - "`#[must_use]` attribute on a unit-returning function / method" -} - -declare_clippy_lint! { - /// **What it does:** Checks for a [`#[must_use]`] attribute without - /// further information on functions and methods that return a type already - /// marked as `#[must_use]`. - /// - /// [`#[must_use]`]: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute - /// - /// **Why is this bad?** The attribute isn't needed. Not using the result - /// will already be reported. Alternatively, one can add some text to the - /// attribute to improve the lint message. - /// - /// **Known problems:** None. - /// - /// **Examples:** - /// ```rust - /// #[must_use] - /// fn double_must_use() -> Result<(), ()> { - /// unimplemented!(); - /// } - /// ``` - pub DOUBLE_MUST_USE, - style, - "`#[must_use]` attribute on a `#[must_use]`-returning function / method" -} - -declare_clippy_lint! { - /// **What it does:** Checks for public functions that have no - /// [`#[must_use]`] attribute, but return something not already marked - /// must-use, have no mutable arg and mutate no statics. - /// - /// [`#[must_use]`]: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute - /// - /// **Why is this bad?** Not bad at all, this lint just shows places where - /// you could add the attribute. - /// - /// **Known problems:** The lint only checks the arguments for mutable - /// types without looking if they are actually changed. On the other hand, - /// it also ignores a broad range of potentially interesting side effects, - /// because we cannot decide whether the programmer intends the function to - /// be called for the side effect or the result. Expect many false - /// positives. At least we don't lint if the result type is unit or already - /// `#[must_use]`. - /// - /// **Examples:** - /// ```rust - /// // this could be annotated with `#[must_use]`. - /// fn id<T>(t: T) -> T { t } - /// ``` - pub MUST_USE_CANDIDATE, - pedantic, - "function or method that could take a `#[must_use]` attribute" -} - -declare_clippy_lint! { - /// **What it does:** Checks for public functions that return a `Result` - /// with an `Err` type of `()`. It suggests using a custom type that - /// implements [`std::error::Error`]. - /// - /// **Why is this bad?** Unit does not implement `Error` and carries no - /// further information about what went wrong. - /// - /// **Known problems:** Of course, this lint assumes that `Result` is used - /// for a fallible operation (which is after all the intended use). However - /// code may opt to (mis)use it as a basic two-variant-enum. In that case, - /// the suggestion is misguided, and the code should use a custom enum - /// instead. - /// - /// **Examples:** - /// ```rust - /// pub fn read_u8() -> Result<u8, ()> { Err(()) } - /// ``` - /// should become - /// ```rust,should_panic - /// use std::fmt; - /// - /// #[derive(Debug)] - /// pub struct EndOfStream; - /// - /// impl fmt::Display for EndOfStream { - /// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - /// write!(f, "End of Stream") - /// } - /// } - /// - /// impl std::error::Error for EndOfStream { } - /// - /// pub fn read_u8() -> Result<u8, EndOfStream> { Err(EndOfStream) } - ///# fn main() { - ///# read_u8().unwrap(); - ///# } - /// ``` - /// - /// Note that there are crates that simplify creating the error type, e.g. - /// [`thiserror`](https://docs.rs/thiserror). - pub RESULT_UNIT_ERR, - style, - "public function returning `Result` with an `Err` type of `()`" -} - -#[derive(Copy, Clone)] -pub struct Functions { - threshold: u64, - max_lines: u64, -} - -impl Functions { - pub fn new(threshold: u64, max_lines: u64) -> Self { - Self { threshold, max_lines } - } -} - -impl_lint_pass!(Functions => [ - TOO_MANY_ARGUMENTS, - TOO_MANY_LINES, - NOT_UNSAFE_PTR_ARG_DEREF, - MUST_USE_UNIT, - DOUBLE_MUST_USE, - MUST_USE_CANDIDATE, - RESULT_UNIT_ERR, -]); - -impl<'tcx> LateLintPass<'tcx> for Functions { - fn check_fn( - &mut self, - cx: &LateContext<'tcx>, - kind: intravisit::FnKind<'tcx>, - decl: &'tcx hir::FnDecl<'_>, - body: &'tcx hir::Body<'_>, - span: Span, - hir_id: hir::HirId, - ) { - let unsafety = match kind { - intravisit::FnKind::ItemFn(_, _, hir::FnHeader { unsafety, .. }, _) => unsafety, - intravisit::FnKind::Method(_, sig, _) => sig.header.unsafety, - intravisit::FnKind::Closure => return, - }; - - // don't warn for implementations, it's not their fault - if !is_trait_impl_item(cx, hir_id) { - // don't lint extern functions decls, it's not their fault either - match kind { - intravisit::FnKind::Method( - _, - &hir::FnSig { - header: hir::FnHeader { abi: Abi::Rust, .. }, - .. - }, - _, - ) - | intravisit::FnKind::ItemFn(_, _, hir::FnHeader { abi: Abi::Rust, .. }, _) => { - self.check_arg_number(cx, decl, span.with_hi(decl.output.span().hi())) - }, - _ => {}, - } - } - - Self::check_raw_ptr(cx, unsafety, decl, body, hir_id); - self.check_line_number(cx, span, body); - } - - fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { - let attrs = cx.tcx.hir().attrs(item.hir_id()); - let attr = must_use_attr(attrs); - if let hir::ItemKind::Fn(ref sig, ref _generics, ref body_id) = item.kind { - let is_public = cx.access_levels.is_exported(item.hir_id()); - let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); - if is_public { - check_result_unit_err(cx, &sig.decl, item.span, fn_header_span); - } - if let Some(attr) = attr { - check_needless_must_use(cx, &sig.decl, item.hir_id(), item.span, fn_header_span, attr); - return; - } - if is_public && !is_proc_macro(cx.sess(), attrs) && attr_by_name(attrs, "no_mangle").is_none() { - check_must_use_candidate( - cx, - &sig.decl, - cx.tcx.hir().body(*body_id), - item.span, - item.hir_id(), - item.span.with_hi(sig.decl.output.span().hi()), - "this function could have a `#[must_use]` attribute", - ); - } - } - } - - fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) { - if let hir::ImplItemKind::Fn(ref sig, ref body_id) = item.kind { - let is_public = cx.access_levels.is_exported(item.hir_id()); - let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); - if is_public && trait_ref_of_method(cx, item.hir_id()).is_none() { - check_result_unit_err(cx, &sig.decl, item.span, fn_header_span); - } - let attrs = cx.tcx.hir().attrs(item.hir_id()); - let attr = must_use_attr(attrs); - if let Some(attr) = attr { - check_needless_must_use(cx, &sig.decl, item.hir_id(), item.span, fn_header_span, attr); - } else if is_public && !is_proc_macro(cx.sess(), attrs) && trait_ref_of_method(cx, item.hir_id()).is_none() - { - check_must_use_candidate( - cx, - &sig.decl, - cx.tcx.hir().body(*body_id), - item.span, - item.hir_id(), - item.span.with_hi(sig.decl.output.span().hi()), - "this method could have a `#[must_use]` attribute", - ); - } - } - } - - fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) { - if let hir::TraitItemKind::Fn(ref sig, ref eid) = item.kind { - // don't lint extern functions decls, it's not their fault - if sig.header.abi == Abi::Rust { - self.check_arg_number(cx, &sig.decl, item.span.with_hi(sig.decl.output.span().hi())); - } - let is_public = cx.access_levels.is_exported(item.hir_id()); - let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); - if is_public { - check_result_unit_err(cx, &sig.decl, item.span, fn_header_span); - } - - let attrs = cx.tcx.hir().attrs(item.hir_id()); - let attr = must_use_attr(attrs); - if let Some(attr) = attr { - check_needless_must_use(cx, &sig.decl, item.hir_id(), item.span, fn_header_span, attr); - } - if let hir::TraitFn::Provided(eid) = *eid { - let body = cx.tcx.hir().body(eid); - Self::check_raw_ptr(cx, sig.header.unsafety, &sig.decl, body, item.hir_id()); - - if attr.is_none() && is_public && !is_proc_macro(cx.sess(), attrs) { - check_must_use_candidate( - cx, - &sig.decl, - body, - item.span, - item.hir_id(), - item.span.with_hi(sig.decl.output.span().hi()), - "this method could have a `#[must_use]` attribute", - ); - } - } - } - } -} - -impl<'tcx> Functions { - fn check_arg_number(self, cx: &LateContext<'_>, decl: &hir::FnDecl<'_>, fn_span: Span) { - let args = decl.inputs.len() as u64; - if args > self.threshold { - span_lint( - cx, - TOO_MANY_ARGUMENTS, - fn_span, - &format!("this function has too many arguments ({}/{})", args, self.threshold), - ); - } - } - - fn check_line_number(self, cx: &LateContext<'_>, span: Span, body: &'tcx hir::Body<'_>) { - if in_external_macro(cx.sess(), span) { - return; - } - - let code_snippet = snippet(cx, body.value.span, ".."); - let mut line_count: u64 = 0; - let mut in_comment = false; - let mut code_in_line; - - // Skip the surrounding function decl. - let start_brace_idx = code_snippet.find('{').map_or(0, |i| i + 1); - let end_brace_idx = code_snippet.rfind('}').unwrap_or_else(|| code_snippet.len()); - let function_lines = code_snippet[start_brace_idx..end_brace_idx].lines(); - - for mut line in function_lines { - code_in_line = false; - loop { - line = line.trim_start(); - if line.is_empty() { - break; - } - if in_comment { - if let Some(i) = line.find("*/") { - line = &line[i + 2..]; - in_comment = false; - continue; - } - } else { - let multi_idx = line.find("/*").unwrap_or_else(|| line.len()); - let single_idx = line.find("//").unwrap_or_else(|| line.len()); - code_in_line |= multi_idx > 0 && single_idx > 0; - // Implies multi_idx is below line.len() - if multi_idx < single_idx { - line = &line[multi_idx + 2..]; - in_comment = true; - continue; - } - } - break; - } - if code_in_line { - line_count += 1; - } - } - - if line_count > self.max_lines { - span_lint( - cx, - TOO_MANY_LINES, - span, - &format!("this function has too many lines ({}/{})", line_count, self.max_lines), - ) - } - } - - fn check_raw_ptr( - cx: &LateContext<'tcx>, - unsafety: hir::Unsafety, - decl: &'tcx hir::FnDecl<'_>, - body: &'tcx hir::Body<'_>, - hir_id: hir::HirId, - ) { - let expr = &body.value; - if unsafety == hir::Unsafety::Normal && cx.access_levels.is_exported(hir_id) { - let raw_ptrs = iter_input_pats(decl, body) - .zip(decl.inputs.iter()) - .filter_map(|(arg, ty)| raw_ptr_arg(arg, ty)) - .collect::<FxHashSet<_>>(); - - if !raw_ptrs.is_empty() { - let typeck_results = cx.tcx.typeck_body(body.id()); - let mut v = DerefVisitor { - cx, - ptrs: raw_ptrs, - typeck_results, - }; - - intravisit::walk_expr(&mut v, expr); - } - } - } -} - -fn check_result_unit_err(cx: &LateContext<'_>, decl: &hir::FnDecl<'_>, item_span: Span, fn_header_span: Span) { - if_chain! { - if !in_external_macro(cx.sess(), item_span); - if let hir::FnRetTy::Return(ref ty) = decl.output; - let ty = hir_ty_to_ty(cx.tcx, ty); - if is_type_diagnostic_item(cx, ty, sym::result_type); - if let ty::Adt(_, substs) = ty.kind(); - let err_ty = substs.type_at(1); - if err_ty.is_unit(); - then { - span_lint_and_help( - cx, - RESULT_UNIT_ERR, - fn_header_span, - "this returns a `Result<_, ()>", - None, - "use a custom Error type instead", - ); - } - } -} - -fn check_needless_must_use( - cx: &LateContext<'_>, - decl: &hir::FnDecl<'_>, - item_id: hir::HirId, - item_span: Span, - fn_header_span: Span, - attr: &Attribute, -) { - if in_external_macro(cx.sess(), item_span) { - return; - } - if returns_unit(decl) { - span_lint_and_then( - cx, - MUST_USE_UNIT, - fn_header_span, - "this unit-returning function has a `#[must_use]` attribute", - |diag| { - diag.span_suggestion( - attr.span, - "remove the attribute", - "".into(), - Applicability::MachineApplicable, - ); - }, - ); - } else if !attr.is_value_str() && is_must_use_ty(cx, return_ty(cx, item_id)) { - span_lint_and_help( - cx, - DOUBLE_MUST_USE, - fn_header_span, - "this function has an empty `#[must_use]` attribute, but returns a type already marked as `#[must_use]`", - None, - "either add some descriptive text or remove the attribute", - ); - } -} - -fn check_must_use_candidate<'tcx>( - cx: &LateContext<'tcx>, - decl: &'tcx hir::FnDecl<'_>, - body: &'tcx hir::Body<'_>, - item_span: Span, - item_id: hir::HirId, - fn_span: Span, - msg: &str, -) { - if has_mutable_arg(cx, body) - || mutates_static(cx, body) - || in_external_macro(cx.sess(), item_span) - || returns_unit(decl) - || !cx.access_levels.is_exported(item_id) - || is_must_use_ty(cx, return_ty(cx, item_id)) - { - return; - } - span_lint_and_then(cx, MUST_USE_CANDIDATE, fn_span, msg, |diag| { - if let Some(snippet) = snippet_opt(cx, fn_span) { - diag.span_suggestion( - fn_span, - "add the attribute", - format!("#[must_use] {}", snippet), - Applicability::MachineApplicable, - ); - } - }); -} - -fn returns_unit(decl: &hir::FnDecl<'_>) -> bool { - match decl.output { - hir::FnRetTy::DefaultReturn(_) => true, - hir::FnRetTy::Return(ref ty) => match ty.kind { - hir::TyKind::Tup(ref tys) => tys.is_empty(), - hir::TyKind::Never => true, - _ => false, - }, - } -} - -fn has_mutable_arg(cx: &LateContext<'_>, body: &hir::Body<'_>) -> bool { - let mut tys = FxHashSet::default(); - body.params.iter().any(|param| is_mutable_pat(cx, ¶m.pat, &mut tys)) -} - -fn is_mutable_pat(cx: &LateContext<'_>, pat: &hir::Pat<'_>, tys: &mut FxHashSet<DefId>) -> bool { - if let hir::PatKind::Wild = pat.kind { - return false; // ignore `_` patterns - } - if cx.tcx.has_typeck_results(pat.hir_id.owner.to_def_id()) { - is_mutable_ty(cx, &cx.tcx.typeck(pat.hir_id.owner).pat_ty(pat), pat.span, tys) - } else { - false - } -} - -static KNOWN_WRAPPER_TYS: &[&[&str]] = &[&["alloc", "rc", "Rc"], &["std", "sync", "Arc"]]; - -fn is_mutable_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, span: Span, tys: &mut FxHashSet<DefId>) -> bool { - match *ty.kind() { - // primitive types are never mutable - ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str => false, - ty::Adt(ref adt, ref substs) => { - tys.insert(adt.did) && !ty.is_freeze(cx.tcx.at(span), cx.param_env) - || KNOWN_WRAPPER_TYS.iter().any(|path| match_def_path(cx, adt.did, path)) - && substs.types().any(|ty| is_mutable_ty(cx, ty, span, tys)) - }, - ty::Tuple(ref substs) => substs.types().any(|ty| is_mutable_ty(cx, ty, span, tys)), - ty::Array(ty, _) | ty::Slice(ty) => is_mutable_ty(cx, ty, span, tys), - ty::RawPtr(ty::TypeAndMut { ty, mutbl }) | ty::Ref(_, ty, mutbl) => { - mutbl == hir::Mutability::Mut || is_mutable_ty(cx, ty, span, tys) - }, - // calling something constitutes a side effect, so return true on all callables - // also never calls need not be used, so return true for them, too - _ => true, - } -} - -fn raw_ptr_arg(arg: &hir::Param<'_>, ty: &hir::Ty<'_>) -> Option<hir::HirId> { - if let (&hir::PatKind::Binding(_, id, _, _), &hir::TyKind::Ptr(_)) = (&arg.pat.kind, &ty.kind) { - Some(id) - } else { - None - } -} - -struct DerefVisitor<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - ptrs: FxHashSet<hir::HirId>, - typeck_results: &'a ty::TypeckResults<'tcx>, -} - -impl<'a, 'tcx> intravisit::Visitor<'tcx> for DerefVisitor<'a, 'tcx> { - type Map = Map<'tcx>; - - fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) { - match expr.kind { - hir::ExprKind::Call(ref f, args) => { - let ty = self.typeck_results.expr_ty(f); - - if type_is_unsafe_function(self.cx, ty) { - for arg in args { - self.check_arg(arg); - } - } - }, - hir::ExprKind::MethodCall(_, _, args, _) => { - let def_id = self.typeck_results.type_dependent_def_id(expr.hir_id).unwrap(); - let base_type = self.cx.tcx.type_of(def_id); - - if type_is_unsafe_function(self.cx, base_type) { - for arg in args { - self.check_arg(arg); - } - } - }, - hir::ExprKind::Unary(hir::UnOp::Deref, ref ptr) => self.check_arg(ptr), - _ => (), - } - - intravisit::walk_expr(self, expr); - } - - fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> { - intravisit::NestedVisitorMap::None - } -} - -impl<'a, 'tcx> DerefVisitor<'a, 'tcx> { - fn check_arg(&self, ptr: &hir::Expr<'_>) { - if let Some(id) = path_to_local(ptr) { - if self.ptrs.contains(&id) { - span_lint( - self.cx, - NOT_UNSAFE_PTR_ARG_DEREF, - ptr.span, - "this public function dereferences a raw pointer but is not marked `unsafe`", - ); - } - } - } -} - -struct StaticMutVisitor<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - mutates_static: bool, -} - -impl<'a, 'tcx> intravisit::Visitor<'tcx> for StaticMutVisitor<'a, 'tcx> { - type Map = Map<'tcx>; - - fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) { - use hir::ExprKind::{AddrOf, Assign, AssignOp, Call, MethodCall}; - - if self.mutates_static { - return; - } - match expr.kind { - Call(_, args) | MethodCall(_, _, args, _) => { - let mut tys = FxHashSet::default(); - for arg in args { - if self.cx.tcx.has_typeck_results(arg.hir_id.owner.to_def_id()) - && is_mutable_ty( - self.cx, - self.cx.tcx.typeck(arg.hir_id.owner).expr_ty(arg), - arg.span, - &mut tys, - ) - && is_mutated_static(arg) - { - self.mutates_static = true; - return; - } - tys.clear(); - } - }, - Assign(ref target, ..) | AssignOp(_, ref target, _) | AddrOf(_, hir::Mutability::Mut, ref target) => { - self.mutates_static |= is_mutated_static(target) - }, - _ => {}, - } - } - - fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> { - intravisit::NestedVisitorMap::None - } -} - -fn is_mutated_static(e: &hir::Expr<'_>) -> bool { - use hir::ExprKind::{Field, Index, Path}; - - match e.kind { - Path(QPath::Resolved(_, path)) => !matches!(path.res, Res::Local(_)), - Path(_) => true, - Field(ref inner, _) | Index(ref inner, _) => is_mutated_static(inner), - _ => false, - } -} - -fn mutates_static<'tcx>(cx: &LateContext<'tcx>, body: &'tcx hir::Body<'_>) -> bool { - let mut v = StaticMutVisitor { - cx, - mutates_static: false, - }; - intravisit::walk_expr(&mut v, &body.value); - v.mutates_static -} diff --git a/clippy_lints/src/functions/mod.rs b/clippy_lints/src/functions/mod.rs new file mode 100644 index 00000000000..2beb9bc94bf --- /dev/null +++ b/clippy_lints/src/functions/mod.rs @@ -0,0 +1,267 @@ +mod must_use; +mod not_unsafe_ptr_arg_deref; +mod result_unit_err; +mod too_many_arguments; +mod too_many_lines; + +use rustc_hir as hir; +use rustc_hir::intravisit; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::Span; + +declare_clippy_lint! { + /// **What it does:** Checks for functions with too many parameters. + /// + /// **Why is this bad?** Functions with lots of parameters are considered bad + /// style and reduce readability (“what does the 5th parameter mean?”). Consider + /// grouping some parameters into a new type. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// # struct Color; + /// fn foo(x: u32, y: u32, name: &str, c: Color, w: f32, h: f32, a: f32, b: f32) { + /// // .. + /// } + /// ``` + pub TOO_MANY_ARGUMENTS, + complexity, + "functions with too many arguments" +} + +declare_clippy_lint! { + /// **What it does:** Checks for functions with a large amount of lines. + /// + /// **Why is this bad?** Functions with a lot of lines are harder to understand + /// due to having to look at a larger amount of code to understand what the + /// function is doing. Consider splitting the body of the function into + /// multiple functions. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// fn im_too_long() { + /// println!(""); + /// // ... 100 more LoC + /// println!(""); + /// } + /// ``` + pub TOO_MANY_LINES, + pedantic, + "functions with too many lines" +} + +declare_clippy_lint! { + /// **What it does:** Checks for public functions that dereference raw pointer + /// arguments but are not marked `unsafe`. + /// + /// **Why is this bad?** The function should probably be marked `unsafe`, since + /// for an arbitrary raw pointer, there is no way of telling for sure if it is + /// valid. + /// + /// **Known problems:** + /// + /// * It does not check functions recursively so if the pointer is passed to a + /// private non-`unsafe` function which does the dereferencing, the lint won't + /// trigger. + /// * It only checks for arguments whose type are raw pointers, not raw pointers + /// got from an argument in some other way (`fn foo(bar: &[*const u8])` or + /// `some_argument.get_raw_ptr()`). + /// + /// **Example:** + /// ```rust,ignore + /// // Bad + /// pub fn foo(x: *const u8) { + /// println!("{}", unsafe { *x }); + /// } + /// + /// // Good + /// pub unsafe fn foo(x: *const u8) { + /// println!("{}", unsafe { *x }); + /// } + /// ``` + pub NOT_UNSAFE_PTR_ARG_DEREF, + correctness, + "public functions dereferencing raw pointer arguments but not marked `unsafe`" +} + +declare_clippy_lint! { + /// **What it does:** Checks for a [`#[must_use]`] attribute on + /// unit-returning functions and methods. + /// + /// [`#[must_use]`]: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute + /// + /// **Why is this bad?** Unit values are useless. The attribute is likely + /// a remnant of a refactoring that removed the return type. + /// + /// **Known problems:** None. + /// + /// **Examples:** + /// ```rust + /// #[must_use] + /// fn useless() { } + /// ``` + pub MUST_USE_UNIT, + style, + "`#[must_use]` attribute on a unit-returning function / method" +} + +declare_clippy_lint! { + /// **What it does:** Checks for a [`#[must_use]`] attribute without + /// further information on functions and methods that return a type already + /// marked as `#[must_use]`. + /// + /// [`#[must_use]`]: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute + /// + /// **Why is this bad?** The attribute isn't needed. Not using the result + /// will already be reported. Alternatively, one can add some text to the + /// attribute to improve the lint message. + /// + /// **Known problems:** None. + /// + /// **Examples:** + /// ```rust + /// #[must_use] + /// fn double_must_use() -> Result<(), ()> { + /// unimplemented!(); + /// } + /// ``` + pub DOUBLE_MUST_USE, + style, + "`#[must_use]` attribute on a `#[must_use]`-returning function / method" +} + +declare_clippy_lint! { + /// **What it does:** Checks for public functions that have no + /// [`#[must_use]`] attribute, but return something not already marked + /// must-use, have no mutable arg and mutate no statics. + /// + /// [`#[must_use]`]: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-must_use-attribute + /// + /// **Why is this bad?** Not bad at all, this lint just shows places where + /// you could add the attribute. + /// + /// **Known problems:** The lint only checks the arguments for mutable + /// types without looking if they are actually changed. On the other hand, + /// it also ignores a broad range of potentially interesting side effects, + /// because we cannot decide whether the programmer intends the function to + /// be called for the side effect or the result. Expect many false + /// positives. At least we don't lint if the result type is unit or already + /// `#[must_use]`. + /// + /// **Examples:** + /// ```rust + /// // this could be annotated with `#[must_use]`. + /// fn id<T>(t: T) -> T { t } + /// ``` + pub MUST_USE_CANDIDATE, + pedantic, + "function or method that could take a `#[must_use]` attribute" +} + +declare_clippy_lint! { + /// **What it does:** Checks for public functions that return a `Result` + /// with an `Err` type of `()`. It suggests using a custom type that + /// implements `std::error::Error`. + /// + /// **Why is this bad?** Unit does not implement `Error` and carries no + /// further information about what went wrong. + /// + /// **Known problems:** Of course, this lint assumes that `Result` is used + /// for a fallible operation (which is after all the intended use). However + /// code may opt to (mis)use it as a basic two-variant-enum. In that case, + /// the suggestion is misguided, and the code should use a custom enum + /// instead. + /// + /// **Examples:** + /// ```rust + /// pub fn read_u8() -> Result<u8, ()> { Err(()) } + /// ``` + /// should become + /// ```rust,should_panic + /// use std::fmt; + /// + /// #[derive(Debug)] + /// pub struct EndOfStream; + /// + /// impl fmt::Display for EndOfStream { + /// fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + /// write!(f, "End of Stream") + /// } + /// } + /// + /// impl std::error::Error for EndOfStream { } + /// + /// pub fn read_u8() -> Result<u8, EndOfStream> { Err(EndOfStream) } + ///# fn main() { + ///# read_u8().unwrap(); + ///# } + /// ``` + /// + /// Note that there are crates that simplify creating the error type, e.g. + /// [`thiserror`](https://docs.rs/thiserror). + pub RESULT_UNIT_ERR, + style, + "public function returning `Result` with an `Err` type of `()`" +} + +#[derive(Copy, Clone)] +pub struct Functions { + too_many_arguments_threshold: u64, + too_many_lines_threshold: u64, +} + +impl Functions { + pub fn new(too_many_arguments_threshold: u64, too_many_lines_threshold: u64) -> Self { + Self { + too_many_arguments_threshold, + too_many_lines_threshold, + } + } +} + +impl_lint_pass!(Functions => [ + TOO_MANY_ARGUMENTS, + TOO_MANY_LINES, + NOT_UNSAFE_PTR_ARG_DEREF, + MUST_USE_UNIT, + DOUBLE_MUST_USE, + MUST_USE_CANDIDATE, + RESULT_UNIT_ERR, +]); + +impl<'tcx> LateLintPass<'tcx> for Functions { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + kind: intravisit::FnKind<'tcx>, + decl: &'tcx hir::FnDecl<'_>, + body: &'tcx hir::Body<'_>, + span: Span, + hir_id: hir::HirId, + ) { + too_many_arguments::check_fn(cx, kind, decl, span, hir_id, self.too_many_arguments_threshold); + too_many_lines::check_fn(cx, span, body, self.too_many_lines_threshold); + not_unsafe_ptr_arg_deref::check_fn(cx, kind, decl, body, hir_id); + } + + 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); + } + + 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); + } + + 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); + } +} diff --git a/clippy_lints/src/functions/must_use.rs b/clippy_lints/src/functions/must_use.rs new file mode 100644 index 00000000000..3825699936f --- /dev/null +++ b/clippy_lints/src/functions/must_use.rs @@ -0,0 +1,272 @@ +use rustc_ast::ast::Attribute; +use rustc_data_structures::fx::FxHashSet; +use rustc_errors::Applicability; +use rustc_hir::{self as hir, def::Res, def_id::DefId, intravisit, QPath}; +use rustc_lint::{LateContext, LintContext}; +use rustc_middle::{ + hir::map::Map, + lint::in_external_macro, + ty::{self, Ty}, +}; +use rustc_span::Span; + +use clippy_utils::attrs::is_proc_macro; +use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_then}; +use clippy_utils::source::snippet_opt; +use clippy_utils::ty::is_must_use_ty; +use clippy_utils::{attr_by_name, match_def_path, must_use_attr, return_ty, trait_ref_of_method}; + +use super::{DOUBLE_MUST_USE, MUST_USE_CANDIDATE, MUST_USE_UNIT}; + +pub(super) fn check_item(cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { + let attrs = cx.tcx.hir().attrs(item.hir_id()); + let attr = must_use_attr(attrs); + if let hir::ItemKind::Fn(ref sig, ref _generics, ref body_id) = item.kind { + let is_public = cx.access_levels.is_exported(item.hir_id()); + let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); + if let Some(attr) = attr { + check_needless_must_use(cx, &sig.decl, item.hir_id(), item.span, fn_header_span, attr); + return; + } else if is_public && !is_proc_macro(cx.sess(), attrs) && attr_by_name(attrs, "no_mangle").is_none() { + check_must_use_candidate( + cx, + &sig.decl, + cx.tcx.hir().body(*body_id), + item.span, + item.hir_id(), + item.span.with_hi(sig.decl.output.span().hi()), + "this function could have a `#[must_use]` attribute", + ); + } + } +} + +pub(super) fn check_impl_item(cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) { + if let hir::ImplItemKind::Fn(ref sig, ref body_id) = item.kind { + let is_public = cx.access_levels.is_exported(item.hir_id()); + let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); + let attrs = cx.tcx.hir().attrs(item.hir_id()); + let attr = must_use_attr(attrs); + if let Some(attr) = attr { + check_needless_must_use(cx, &sig.decl, item.hir_id(), item.span, fn_header_span, attr); + } else if is_public && !is_proc_macro(cx.sess(), attrs) && trait_ref_of_method(cx, item.hir_id()).is_none() { + check_must_use_candidate( + cx, + &sig.decl, + cx.tcx.hir().body(*body_id), + item.span, + item.hir_id(), + item.span.with_hi(sig.decl.output.span().hi()), + "this method could have a `#[must_use]` attribute", + ); + } + } +} + +pub(super) fn check_trait_item(cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) { + if let hir::TraitItemKind::Fn(ref sig, ref eid) = item.kind { + let is_public = cx.access_levels.is_exported(item.hir_id()); + let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); + + let attrs = cx.tcx.hir().attrs(item.hir_id()); + let attr = must_use_attr(attrs); + if let Some(attr) = attr { + check_needless_must_use(cx, &sig.decl, item.hir_id(), item.span, fn_header_span, attr); + } else if let hir::TraitFn::Provided(eid) = *eid { + let body = cx.tcx.hir().body(eid); + if attr.is_none() && is_public && !is_proc_macro(cx.sess(), attrs) { + check_must_use_candidate( + cx, + &sig.decl, + body, + item.span, + item.hir_id(), + item.span.with_hi(sig.decl.output.span().hi()), + "this method could have a `#[must_use]` attribute", + ); + } + } + } +} + +fn check_needless_must_use( + cx: &LateContext<'_>, + decl: &hir::FnDecl<'_>, + item_id: hir::HirId, + item_span: Span, + fn_header_span: Span, + attr: &Attribute, +) { + if in_external_macro(cx.sess(), item_span) { + return; + } + if returns_unit(decl) { + span_lint_and_then( + cx, + MUST_USE_UNIT, + fn_header_span, + "this unit-returning function has a `#[must_use]` attribute", + |diag| { + diag.span_suggestion( + attr.span, + "remove the attribute", + "".into(), + Applicability::MachineApplicable, + ); + }, + ); + } else if !attr.is_value_str() && is_must_use_ty(cx, return_ty(cx, item_id)) { + span_lint_and_help( + cx, + DOUBLE_MUST_USE, + fn_header_span, + "this function has an empty `#[must_use]` attribute, but returns a type already marked as `#[must_use]`", + None, + "either add some descriptive text or remove the attribute", + ); + } +} + +fn check_must_use_candidate<'tcx>( + cx: &LateContext<'tcx>, + decl: &'tcx hir::FnDecl<'_>, + body: &'tcx hir::Body<'_>, + item_span: Span, + item_id: hir::HirId, + fn_span: Span, + msg: &str, +) { + if has_mutable_arg(cx, body) + || mutates_static(cx, body) + || in_external_macro(cx.sess(), item_span) + || returns_unit(decl) + || !cx.access_levels.is_exported(item_id) + || is_must_use_ty(cx, return_ty(cx, item_id)) + { + return; + } + span_lint_and_then(cx, MUST_USE_CANDIDATE, fn_span, msg, |diag| { + if let Some(snippet) = snippet_opt(cx, fn_span) { + diag.span_suggestion( + fn_span, + "add the attribute", + format!("#[must_use] {}", snippet), + Applicability::MachineApplicable, + ); + } + }); +} + +fn returns_unit(decl: &hir::FnDecl<'_>) -> bool { + match decl.output { + hir::FnRetTy::DefaultReturn(_) => true, + hir::FnRetTy::Return(ref ty) => match ty.kind { + hir::TyKind::Tup(ref tys) => tys.is_empty(), + hir::TyKind::Never => true, + _ => false, + }, + } +} + +fn has_mutable_arg(cx: &LateContext<'_>, body: &hir::Body<'_>) -> bool { + let mut tys = FxHashSet::default(); + body.params.iter().any(|param| is_mutable_pat(cx, ¶m.pat, &mut tys)) +} + +fn is_mutable_pat(cx: &LateContext<'_>, pat: &hir::Pat<'_>, tys: &mut FxHashSet<DefId>) -> bool { + if let hir::PatKind::Wild = pat.kind { + return false; // ignore `_` patterns + } + if cx.tcx.has_typeck_results(pat.hir_id.owner.to_def_id()) { + is_mutable_ty(cx, &cx.tcx.typeck(pat.hir_id.owner).pat_ty(pat), pat.span, tys) + } else { + false + } +} + +static KNOWN_WRAPPER_TYS: &[&[&str]] = &[&["alloc", "rc", "Rc"], &["std", "sync", "Arc"]]; + +fn is_mutable_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, span: Span, tys: &mut FxHashSet<DefId>) -> bool { + match *ty.kind() { + // primitive types are never mutable + ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str => false, + ty::Adt(ref adt, ref substs) => { + tys.insert(adt.did) && !ty.is_freeze(cx.tcx.at(span), cx.param_env) + || KNOWN_WRAPPER_TYS.iter().any(|path| match_def_path(cx, adt.did, path)) + && substs.types().any(|ty| is_mutable_ty(cx, ty, span, tys)) + }, + ty::Tuple(ref substs) => substs.types().any(|ty| is_mutable_ty(cx, ty, span, tys)), + ty::Array(ty, _) | ty::Slice(ty) => is_mutable_ty(cx, ty, span, tys), + ty::RawPtr(ty::TypeAndMut { ty, mutbl }) | ty::Ref(_, ty, mutbl) => { + mutbl == hir::Mutability::Mut || is_mutable_ty(cx, ty, span, tys) + }, + // calling something constitutes a side effect, so return true on all callables + // also never calls need not be used, so return true for them, too + _ => true, + } +} + +struct StaticMutVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + mutates_static: bool, +} + +impl<'a, 'tcx> intravisit::Visitor<'tcx> for StaticMutVisitor<'a, 'tcx> { + type Map = Map<'tcx>; + + fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) { + use hir::ExprKind::{AddrOf, Assign, AssignOp, Call, MethodCall}; + + if self.mutates_static { + return; + } + match expr.kind { + Call(_, args) | MethodCall(_, _, args, _) => { + let mut tys = FxHashSet::default(); + for arg in args { + if self.cx.tcx.has_typeck_results(arg.hir_id.owner.to_def_id()) + && is_mutable_ty( + self.cx, + self.cx.tcx.typeck(arg.hir_id.owner).expr_ty(arg), + arg.span, + &mut tys, + ) + && is_mutated_static(arg) + { + self.mutates_static = true; + return; + } + tys.clear(); + } + }, + Assign(ref target, ..) | AssignOp(_, ref target, _) | AddrOf(_, hir::Mutability::Mut, ref target) => { + self.mutates_static |= is_mutated_static(target) + }, + _ => {}, + } + } + + fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> { + intravisit::NestedVisitorMap::None + } +} + +fn is_mutated_static(e: &hir::Expr<'_>) -> bool { + use hir::ExprKind::{Field, Index, Path}; + + match e.kind { + Path(QPath::Resolved(_, path)) => !matches!(path.res, Res::Local(_)), + Path(_) => true, + Field(ref inner, _) | Index(ref inner, _) => is_mutated_static(inner), + _ => false, + } +} + +fn mutates_static<'tcx>(cx: &LateContext<'tcx>, body: &'tcx hir::Body<'_>) -> bool { + let mut v = StaticMutVisitor { + cx, + mutates_static: false, + }; + intravisit::walk_expr(&mut v, &body.value); + v.mutates_static +} diff --git a/clippy_lints/src/functions/not_unsafe_ptr_arg_deref.rs b/clippy_lints/src/functions/not_unsafe_ptr_arg_deref.rs new file mode 100644 index 00000000000..ac02b60a356 --- /dev/null +++ b/clippy_lints/src/functions/not_unsafe_ptr_arg_deref.rs @@ -0,0 +1,125 @@ +use rustc_data_structures::fx::FxHashSet; +use rustc_hir::{self as hir, intravisit}; +use rustc_lint::LateContext; +use rustc_middle::{hir::map::Map, ty}; + +use clippy_utils::diagnostics::span_lint; +use clippy_utils::ty::type_is_unsafe_function; +use clippy_utils::{iter_input_pats, path_to_local}; + +use super::NOT_UNSAFE_PTR_ARG_DEREF; + +pub(super) fn check_fn( + cx: &LateContext<'tcx>, + kind: intravisit::FnKind<'tcx>, + decl: &'tcx hir::FnDecl<'tcx>, + body: &'tcx hir::Body<'tcx>, + hir_id: hir::HirId, +) { + let unsafety = match kind { + intravisit::FnKind::ItemFn(_, _, hir::FnHeader { unsafety, .. }, _) => unsafety, + intravisit::FnKind::Method(_, sig, _) => sig.header.unsafety, + intravisit::FnKind::Closure => return, + }; + + check_raw_ptr(cx, unsafety, decl, body, hir_id); +} + +pub(super) fn check_trait_item(cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) { + if let hir::TraitItemKind::Fn(ref sig, hir::TraitFn::Provided(eid)) = item.kind { + let body = cx.tcx.hir().body(eid); + check_raw_ptr(cx, sig.header.unsafety, &sig.decl, body, item.hir_id()); + } +} + +fn check_raw_ptr( + cx: &LateContext<'tcx>, + unsafety: hir::Unsafety, + decl: &'tcx hir::FnDecl<'tcx>, + body: &'tcx hir::Body<'tcx>, + hir_id: hir::HirId, +) { + let expr = &body.value; + if unsafety == hir::Unsafety::Normal && cx.access_levels.is_exported(hir_id) { + let raw_ptrs = iter_input_pats(decl, body) + .zip(decl.inputs.iter()) + .filter_map(|(arg, ty)| raw_ptr_arg(arg, ty)) + .collect::<FxHashSet<_>>(); + + if !raw_ptrs.is_empty() { + let typeck_results = cx.tcx.typeck_body(body.id()); + let mut v = DerefVisitor { + cx, + ptrs: raw_ptrs, + typeck_results, + }; + + intravisit::walk_expr(&mut v, expr); + } + } +} + +fn raw_ptr_arg(arg: &hir::Param<'_>, ty: &hir::Ty<'_>) -> Option<hir::HirId> { + if let (&hir::PatKind::Binding(_, id, _, _), &hir::TyKind::Ptr(_)) = (&arg.pat.kind, &ty.kind) { + Some(id) + } else { + None + } +} + +struct DerefVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + ptrs: FxHashSet<hir::HirId>, + typeck_results: &'a ty::TypeckResults<'tcx>, +} + +impl<'a, 'tcx> intravisit::Visitor<'tcx> for DerefVisitor<'a, 'tcx> { + type Map = Map<'tcx>; + + fn visit_expr(&mut self, expr: &'tcx hir::Expr<'_>) { + match expr.kind { + hir::ExprKind::Call(ref f, args) => { + let ty = self.typeck_results.expr_ty(f); + + if type_is_unsafe_function(self.cx, ty) { + for arg in args { + self.check_arg(arg); + } + } + }, + hir::ExprKind::MethodCall(_, _, args, _) => { + let def_id = self.typeck_results.type_dependent_def_id(expr.hir_id).unwrap(); + let base_type = self.cx.tcx.type_of(def_id); + + if type_is_unsafe_function(self.cx, base_type) { + for arg in args { + self.check_arg(arg); + } + } + }, + hir::ExprKind::Unary(hir::UnOp::Deref, ref ptr) => self.check_arg(ptr), + _ => (), + } + + intravisit::walk_expr(self, expr); + } + + fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> { + intravisit::NestedVisitorMap::None + } +} + +impl<'a, 'tcx> DerefVisitor<'a, 'tcx> { + fn check_arg(&self, ptr: &hir::Expr<'_>) { + if let Some(id) = path_to_local(ptr) { + if self.ptrs.contains(&id) { + span_lint( + self.cx, + NOT_UNSAFE_PTR_ARG_DEREF, + ptr.span, + "this public function dereferences a raw pointer but is not marked `unsafe`", + ); + } + } + } +} diff --git a/clippy_lints/src/functions/result_unit_err.rs b/clippy_lints/src/functions/result_unit_err.rs new file mode 100644 index 00000000000..f71bfc690f6 --- /dev/null +++ b/clippy_lints/src/functions/result_unit_err.rs @@ -0,0 +1,66 @@ +use rustc_hir as hir; +use rustc_lint::{LateContext, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_middle::ty; +use rustc_span::{sym, Span}; +use rustc_typeck::hir_ty_to_ty; + +use if_chain::if_chain; + +use clippy_utils::diagnostics::span_lint_and_help; +use clippy_utils::trait_ref_of_method; +use clippy_utils::ty::is_type_diagnostic_item; + +use super::RESULT_UNIT_ERR; + +pub(super) fn check_item(cx: &LateContext<'tcx>, item: &'tcx hir::Item<'_>) { + if let hir::ItemKind::Fn(ref sig, ref _generics, _) = item.kind { + let is_public = cx.access_levels.is_exported(item.hir_id()); + let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); + if is_public { + check_result_unit_err(cx, &sig.decl, item.span, fn_header_span); + } + } +} + +pub(super) fn check_impl_item(cx: &LateContext<'tcx>, item: &'tcx hir::ImplItem<'_>) { + if let hir::ImplItemKind::Fn(ref sig, _) = item.kind { + let is_public = cx.access_levels.is_exported(item.hir_id()); + let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); + if is_public && trait_ref_of_method(cx, item.hir_id()).is_none() { + check_result_unit_err(cx, &sig.decl, item.span, fn_header_span); + } + } +} + +pub(super) fn check_trait_item(cx: &LateContext<'tcx>, item: &'tcx hir::TraitItem<'_>) { + if let hir::TraitItemKind::Fn(ref sig, _) = item.kind { + let is_public = cx.access_levels.is_exported(item.hir_id()); + let fn_header_span = item.span.with_hi(sig.decl.output.span().hi()); + if is_public { + check_result_unit_err(cx, &sig.decl, item.span, fn_header_span); + } + } +} + +fn check_result_unit_err(cx: &LateContext<'_>, decl: &hir::FnDecl<'_>, item_span: Span, fn_header_span: Span) { + if_chain! { + if !in_external_macro(cx.sess(), item_span); + if let hir::FnRetTy::Return(ref ty) = decl.output; + let ty = hir_ty_to_ty(cx.tcx, ty); + if is_type_diagnostic_item(cx, ty, sym::result_type); + if let ty::Adt(_, substs) = ty.kind(); + let err_ty = substs.type_at(1); + if err_ty.is_unit(); + then { + span_lint_and_help( + cx, + RESULT_UNIT_ERR, + fn_header_span, + "this returns a `Result<_, ()>", + None, + "use a custom Error type instead", + ); + } + } +} diff --git a/clippy_lints/src/functions/too_many_arguments.rs b/clippy_lints/src/functions/too_many_arguments.rs new file mode 100644 index 00000000000..62b1e6bd7ca --- /dev/null +++ b/clippy_lints/src/functions/too_many_arguments.rs @@ -0,0 +1,73 @@ +use rustc_hir::{self as hir, intravisit}; +use rustc_lint::LateContext; +use rustc_span::Span; +use rustc_target::spec::abi::Abi; + +use clippy_utils::diagnostics::span_lint; +use clippy_utils::is_trait_impl_item; + +use super::TOO_MANY_ARGUMENTS; + +pub(super) fn check_fn( + cx: &LateContext<'tcx>, + kind: intravisit::FnKind<'tcx>, + decl: &'tcx hir::FnDecl<'_>, + span: Span, + hir_id: hir::HirId, + too_many_arguments_threshold: u64, +) { + // don't warn for implementations, it's not their fault + if !is_trait_impl_item(cx, hir_id) { + // don't lint extern functions decls, it's not their fault either + match kind { + intravisit::FnKind::Method( + _, + &hir::FnSig { + header: hir::FnHeader { abi: Abi::Rust, .. }, + .. + }, + _, + ) + | intravisit::FnKind::ItemFn(_, _, hir::FnHeader { abi: Abi::Rust, .. }, _) => check_arg_number( + cx, + decl, + span.with_hi(decl.output.span().hi()), + too_many_arguments_threshold, + ), + _ => {}, + } + } +} + +pub(super) fn check_trait_item( + cx: &LateContext<'tcx>, + item: &'tcx hir::TraitItem<'_>, + too_many_arguments_threshold: u64, +) { + if let hir::TraitItemKind::Fn(ref sig, _) = item.kind { + // don't lint extern functions decls, it's not their fault + if sig.header.abi == Abi::Rust { + check_arg_number( + cx, + &sig.decl, + item.span.with_hi(sig.decl.output.span().hi()), + too_many_arguments_threshold, + ); + } + } +} + +fn check_arg_number(cx: &LateContext<'_>, decl: &hir::FnDecl<'_>, fn_span: Span, too_many_arguments_threshold: u64) { + let args = decl.inputs.len() as u64; + if args > too_many_arguments_threshold { + span_lint( + cx, + TOO_MANY_ARGUMENTS, + fn_span, + &format!( + "this function has too many arguments ({}/{})", + args, too_many_arguments_threshold + ), + ); + } +} diff --git a/clippy_lints/src/functions/too_many_lines.rs b/clippy_lints/src/functions/too_many_lines.rs new file mode 100644 index 00000000000..aa5494d5a7d --- /dev/null +++ b/clippy_lints/src/functions/too_many_lines.rs @@ -0,0 +1,68 @@ +use rustc_hir as hir; +use rustc_lint::{LateContext, LintContext}; +use rustc_middle::lint::in_external_macro; +use rustc_span::Span; + +use clippy_utils::diagnostics::span_lint; +use clippy_utils::source::snippet; + +use super::TOO_MANY_LINES; + +pub(super) fn check_fn(cx: &LateContext<'_>, span: Span, body: &'tcx hir::Body<'_>, too_many_lines_threshold: u64) { + if in_external_macro(cx.sess(), span) { + return; + } + + let code_snippet = snippet(cx, body.value.span, ".."); + let mut line_count: u64 = 0; + let mut in_comment = false; + let mut code_in_line; + + // Skip the surrounding function decl. + let start_brace_idx = code_snippet.find('{').map_or(0, |i| i + 1); + let end_brace_idx = code_snippet.rfind('}').unwrap_or_else(|| code_snippet.len()); + let function_lines = code_snippet[start_brace_idx..end_brace_idx].lines(); + + for mut line in function_lines { + code_in_line = false; + loop { + line = line.trim_start(); + if line.is_empty() { + break; + } + if in_comment { + if let Some(i) = line.find("*/") { + line = &line[i + 2..]; + in_comment = false; + continue; + } + } else { + let multi_idx = line.find("/*").unwrap_or_else(|| line.len()); + let single_idx = line.find("//").unwrap_or_else(|| line.len()); + code_in_line |= multi_idx > 0 && single_idx > 0; + // Implies multi_idx is below line.len() + if multi_idx < single_idx { + line = &line[multi_idx + 2..]; + in_comment = true; + continue; + } + } + break; + } + if code_in_line { + line_count += 1; + } + } + + if line_count > too_many_lines_threshold { + span_lint( + cx, + TOO_MANY_LINES, + span, + &format!( + "this function has too many lines ({}/{})", + line_count, too_many_lines_threshold + ), + ) + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f013613119c..3d3e57f7d2f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -1124,9 +1124,9 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| box new_without_default::NewWithoutDefault::default()); let blacklisted_names = conf.blacklisted_names.iter().cloned().collect::<FxHashSet<_>>(); store.register_late_pass(move || box blacklisted_name::BlacklistedName::new(blacklisted_names.clone())); - let too_many_arguments_threshold1 = conf.too_many_arguments_threshold; - let too_many_lines_threshold2 = conf.too_many_lines_threshold; - store.register_late_pass(move || box functions::Functions::new(too_many_arguments_threshold1, too_many_lines_threshold2)); + let too_many_arguments_threshold = conf.too_many_arguments_threshold; + let too_many_lines_threshold = conf.too_many_lines_threshold; + store.register_late_pass(move || box functions::Functions::new(too_many_arguments_threshold, too_many_lines_threshold)); let doc_valid_idents = conf.doc_valid_idents.iter().cloned().collect::<FxHashSet<_>>(); store.register_late_pass(move || box doc::DocMarkdown::new(doc_valid_idents.clone())); store.register_late_pass(|| box neg_multiply::NegMultiply); |