-use std::convert::TryFrom;
-
-use crate::utils::{iter_input_pats, snippet, snippet_opt, span_lint, type_is_unsafe_function};
+use crate::utils::{
+ attrs::is_proc_macro, iter_input_pats, match_def_path, qpath_res, return_ty, snippet, snippet_opt,
+ span_help_and_lint, span_lint, span_lint_and_then, type_is_unsafe_function,
+};
use matches::matches;
-use rustc::hir;
-use rustc::hir::def::Res;
-use rustc::hir::intravisit;
+use rustc::hir::{self, def::Res, def_id::DefId, intravisit};
use rustc::lint::{in_external_macro, LateContext, LateLintPass, LintArray, LintContext, LintPass};
-use rustc::ty;
+use rustc::ty::{self, Ty};
use rustc::{declare_tool_lint, impl_lint_pass};
use rustc_data_structures::fx::FxHashSet;
+use rustc_errors::Applicability;
use rustc_target::spec::abi::Abi;
-use syntax::source_map::{BytePos, Span};
+use syntax::ast::Attribute;
+use syntax::source_map::Span;
declare_clippy_lint! {
/// **What it does:** Checks for functions with too many parameters.
"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"
+}
+
#[derive(Copy, Clone)]
pub struct Functions {
threshold: u64,
}
}
-impl_lint_pass!(Functions => [TOO_MANY_ARGUMENTS, TOO_MANY_LINES, NOT_UNSAFE_PTR_ARG_DEREF]);
+impl_lint_pass!(Functions => [
+ TOO_MANY_ARGUMENTS,
+ TOO_MANY_LINES,
+ NOT_UNSAFE_PTR_ARG_DEREF,
+ MUST_USE_UNIT,
+ DOUBLE_MUST_USE,
+ MUST_USE_CANDIDATE,
+]);
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Functions {
fn check_fn(
hir_id: hir::HirId,
) {
let is_impl = if let Some(hir::Node::Item(item)) = cx.tcx.hir().find(cx.tcx.hir().get_parent_node(hir_id)) {
- matches!(item.node, hir::ItemKind::Impl(_, _, _, _, Some(_), _, _))
+ matches!(item.kind, hir::ItemKind::Impl(_, _, _, _, Some(_), _, _))
} else {
false
};
_,
)
| hir::intravisit::FnKind::ItemFn(_, _, hir::FnHeader { abi: Abi::Rust, .. }, _, _) => {
- self.check_arg_number(cx, decl, span)
+ 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_raw_ptr(cx, unsafety, decl, body, hir_id);
self.check_line_number(cx, span, body);
}
+ fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item) {
+ let attr = must_use_attr(&item.attrs);
+ if let hir::ItemKind::Fn(ref decl, ref _header, ref _generics, ref body_id) = item.kind {
+ if let Some(attr) = attr {
+ let fn_header_span = item.span.with_hi(decl.output.span().hi());
+ check_needless_must_use(cx, decl, item.hir_id, item.span, fn_header_span, attr);
+ return;
+ }
+ if cx.access_levels.is_exported(item.hir_id) && !is_proc_macro(&item.attrs) {
+ check_must_use_candidate(
+ cx,
+ decl,
+ cx.tcx.hir().body(*body_id),
+ item.span,
+ item.hir_id,
+ item.span.with_hi(decl.output.span().hi()),
+ "this function could have a `#[must_use]` attribute",
+ );
+ }
+ }
+ }
+
+ fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::ImplItem) {
+ if let hir::ImplItemKind::Method(ref sig, ref body_id) = item.kind {
+ let attr = must_use_attr(&item.attrs);
+ if let Some(attr) = attr {
+ let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
+ check_needless_must_use(cx, &sig.decl, item.hir_id, item.span, fn_header_span, attr);
+ } else if cx.access_levels.is_exported(item.hir_id) && !is_proc_macro(&item.attrs) {
+ 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<'a, 'tcx>, item: &'tcx hir::TraitItem) {
- if let hir::TraitItemKind::Method(ref sig, ref eid) = item.node {
+ if let hir::TraitItemKind::Method(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);
+ self.check_arg_number(cx, &sig.decl, item.span.with_hi(sig.decl.output.span().hi()));
}
+ let attr = must_use_attr(&item.attrs);
+ if let Some(attr) = attr {
+ let fn_header_span = item.span.with_hi(sig.decl.output.span().hi());
+ check_needless_must_use(cx, &sig.decl, item.hir_id, item.span, fn_header_span, attr);
+ }
if let hir::TraitMethod::Provided(eid) = *eid {
let body = cx.tcx.hir().body(eid);
- self.check_raw_ptr(cx, sig.header.unsafety, &sig.decl, body, item.hir_id);
+ Self::check_raw_ptr(cx, sig.header.unsafety, &sig.decl, body, item.hir_id);
+
+ if attr.is_none() && cx.access_levels.is_exported(item.hir_id) && !is_proc_macro(&item.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<'a, 'tcx> Functions {
- fn check_arg_number(self, cx: &LateContext<'_, '_>, decl: &hir::FnDecl, span: Span) {
- // Remove the function body from the span. We can't use `SourceMap::def_span` because the
- // argument list might span multiple lines.
- let span = if let Some(snippet) = snippet_opt(cx, span) {
- let snippet = snippet.split('{').nth(0).unwrap_or("").trim_end();
- if snippet.is_empty() {
- span
- } else {
- span.with_hi(BytePos(span.lo().0 + u32::try_from(snippet.len()).unwrap()))
- }
- } else {
- span
- };
-
+ 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,
- span,
+ fn_span,
&format!("this function has too many arguments ({}/{})", args, self.threshold),
);
}
}
fn check_raw_ptr(
- self,
cx: &LateContext<'a, 'tcx>,
unsafety: hir::Unsafety,
decl: &'tcx hir::FnDecl,
}
}
+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",
+ |db| {
+ db.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_help_and_lint(
+ cx,
+ DOUBLE_MUST_USE,
+ fn_header_span,
+ "this function has an empty `#[must_use]` attribute, but returns a type already marked as `#[must_use]`",
+ "either add some descriptive text or remove the attribute",
+ );
+ }
+}
+
+fn check_must_use_candidate<'a, 'tcx>(
+ cx: &LateContext<'a, '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)
+ || is_must_use_ty(cx, return_ty(cx, item_id))
+ {
+ return;
+ }
+ span_lint_and_then(cx, MUST_USE_CANDIDATE, fn_span, msg, |db| {
+ if let Some(snippet) = snippet_opt(cx, fn_span) {
+ db.span_suggestion(
+ fn_span,
+ "add the attribute",
+ format!("#[must_use] {}", snippet),
+ Applicability::MachineApplicable,
+ );
+ }
+ });
+}
+
+fn must_use_attr(attrs: &[Attribute]) -> Option<&Attribute> {
+ attrs.iter().find(|attr| {
+ attr.ident().map_or(false, |ident| {
+ let ident: &str = &ident.as_str();
+ "must_use" == ident
+ })
+ })
+}
+
+fn returns_unit(decl: &hir::FnDecl) -> bool {
+ match decl.output {
+ hir::FunctionRetTy::DefaultReturn(_) => true,
+ hir::FunctionRetTy::Return(ref ty) => match ty.kind {
+ hir::TyKind::Tup(ref tys) => tys.is_empty(),
+ hir::TyKind::Never => true,
+ _ => false,
+ },
+ }
+}
+
+fn is_must_use_ty<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>) -> bool {
+ use ty::TyKind::*;
+ match ty.kind {
+ Adt(ref adt, _) => must_use_attr(&cx.tcx.get_attrs(adt.did)).is_some(),
+ Foreign(ref did) => must_use_attr(&cx.tcx.get_attrs(*did)).is_some(),
+ Slice(ref ty) | Array(ref ty, _) | RawPtr(ty::TypeAndMut { ref ty, .. }) | Ref(_, ref ty, _) => {
+ // for the Array case we don't need to care for the len == 0 case
+ // because we don't want to lint functions returning empty arrays
+ is_must_use_ty(cx, *ty)
+ },
+ Tuple(ref substs) => substs.types().any(|ty| is_must_use_ty(cx, ty)),
+ Opaque(ref def_id, _) => {
+ for (predicate, _) in cx.tcx.predicates_of(*def_id).predicates {
+ if let ty::Predicate::Trait(ref poly_trait_predicate) = predicate {
+ if must_use_attr(&cx.tcx.get_attrs(poly_trait_predicate.skip_binder().trait_ref.def_id)).is_some() {
+ return true;
+ }
+ }
+ }
+ false
+ },
+ Dynamic(binder, _) => {
+ for predicate in binder.skip_binder().iter() {
+ if let ty::ExistentialPredicate::Trait(ref trait_ref) = predicate {
+ if must_use_attr(&cx.tcx.get_attrs(trait_ref.def_id)).is_some() {
+ return true;
+ }
+ }
+ }
+ false
+ },
+ _ => 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
+ }
+ let def_id = pat.hir_id.owner_def_id();
+ if cx.tcx.has_typeck_tables(def_id) {
+ is_mutable_ty(cx, &cx.tcx.typeck_tables_of(def_id).pat_ty(pat), pat.span, tys)
+ } else {
+ false
+ }
+}
+
+static KNOWN_WRAPPER_TYS: &[&[&str]] = &[&["alloc", "rc", "Rc"], &["std", "sync", "Arc"]];
+
+fn is_mutable_ty<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: Ty<'tcx>, span: Span, tys: &mut FxHashSet<DefId>) -> bool {
+ use ty::TyKind::*;
+ match ty.kind {
+ // primitive types are never mutable
+ Bool | Char | Int(_) | Uint(_) | Float(_) | Str => false,
+ Adt(ref adt, ref substs) => {
+ tys.insert(adt.did) && !ty.is_freeze(cx.tcx, cx.param_env, span)
+ || KNOWN_WRAPPER_TYS.iter().any(|path| match_def_path(cx, adt.did, path))
+ && substs.types().any(|ty| is_mutable_ty(cx, ty, span, tys))
+ },
+ Tuple(ref substs) => substs.types().any(|ty| is_mutable_ty(cx, ty, span, tys)),
+ Array(ty, _) | Slice(ty) => is_mutable_ty(cx, ty, span, tys),
+ RawPtr(ty::TypeAndMut { ty, mutbl }) | Ref(_, ty, mutbl) => {
+ mutbl == hir::Mutability::MutMutable || 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.node, &ty.node) {
+ if let (&hir::PatKind::Binding(_, id, _, _), &hir::TyKind::Ptr(_)) = (&arg.pat.kind, &ty.kind) {
Some(id)
} else {
None
tables: &'a ty::TypeckTables<'tcx>,
}
-impl<'a, 'tcx> hir::intravisit::Visitor<'tcx> for DerefVisitor<'a, 'tcx> {
+impl<'a, 'tcx> intravisit::Visitor<'tcx> for DerefVisitor<'a, 'tcx> {
fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
- match expr.node {
+ match expr.kind {
hir::ExprKind::Call(ref f, ref args) => {
let ty = self.tables.expr_ty(f);
_ => (),
}
- hir::intravisit::walk_expr(self, expr);
+ intravisit::walk_expr(self, expr);
}
+
fn nested_visit_map<'this>(&'this mut self) -> intravisit::NestedVisitorMap<'this, 'tcx> {
intravisit::NestedVisitorMap::None
}
impl<'a, 'tcx> DerefVisitor<'a, 'tcx> {
fn check_arg(&self, ptr: &hir::Expr) {
- if let hir::ExprKind::Path(ref qpath) = ptr.node {
- if let Res::Local(id) = self.cx.tables.qpath_res(qpath, ptr.hir_id) {
+ if let hir::ExprKind::Path(ref qpath) = ptr.kind {
+ if let Res::Local(id) = qpath_res(self.cx, qpath, ptr.hir_id) {
if self.ptrs.contains(&id) {
span_lint(
self.cx,
}
}
}
+
+struct StaticMutVisitor<'a, 'tcx> {
+ cx: &'a LateContext<'a, 'tcx>,
+ mutates_static: bool,
+}
+
+impl<'a, 'tcx> intravisit::Visitor<'tcx> for StaticMutVisitor<'a, 'tcx> {
+ fn visit_expr(&mut self, expr: &'tcx hir::Expr) {
+ use hir::ExprKind::*;
+
+ if self.mutates_static {
+ return;
+ }
+ match expr.kind {
+ Call(_, ref args) | MethodCall(_, _, ref args) => {
+ let mut tys = FxHashSet::default();
+ for arg in args {
+ let def_id = arg.hir_id.owner_def_id();
+ if self.cx.tcx.has_typeck_tables(def_id)
+ && is_mutable_ty(
+ self.cx,
+ self.cx.tcx.typeck_tables_of(def_id).expr_ty(arg),
+ arg.span,
+ &mut tys,
+ )
+ && is_mutated_static(self.cx, arg)
+ {
+ self.mutates_static = true;
+ return;
+ }
+ tys.clear();
+ }
+ },
+ Assign(ref target, _) | AssignOp(_, ref target, _) | AddrOf(hir::Mutability::MutMutable, ref target) => {
+ self.mutates_static |= is_mutated_static(self.cx, target)
+ },
+ _ => {},
+ }
+ }
+
+ fn nested_visit_map<'this>(&'this mut self) -> intravisit::NestedVisitorMap<'this, 'tcx> {
+ intravisit::NestedVisitorMap::None
+ }
+}
+
+fn is_mutated_static(cx: &LateContext<'_, '_>, e: &hir::Expr) -> bool {
+ use hir::ExprKind::*;
+
+ match e.kind {
+ Path(ref qpath) => {
+ if let Res::Local(_) = qpath_res(cx, qpath, e.hir_id) {
+ false
+ } else {
+ true
+ }
+ },
+ Field(ref inner, _) | Index(ref inner, _) => is_mutated_static(cx, inner),
+ _ => false,
+ }
+}
+
+fn mutates_static<'a, 'tcx>(cx: &'a LateContext<'a, 'tcx>, body: &'tcx hir::Body) -> bool {
+ let mut v = StaticMutVisitor {
+ cx,
+ mutates_static: false,
+ };
+ intravisit::walk_expr(&mut v, &body.value);
+ v.mutates_static
+}
projects / rust.git / blobdiff