-use crate::reexport::*;
-use matches::matches;
-use crate::rustc::hir::*;
-use crate::rustc::hir::intravisit::FnKind;
-use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
-use crate::rustc::{declare_tool_lint, lint_array};
use if_chain::if_chain;
-use crate::rustc::ty;
-use crate::syntax::source_map::{ExpnFormat, Span};
-use crate::utils::{get_item_name, get_parent_expr, implements_trait, in_constant, in_macro, is_integer_literal,
- iter_input_pats, last_path_segment, match_qpath, match_trait_method, paths, snippet, span_lint,
- span_lint_and_then, walk_ptrs_ty, SpanlessEq};
-use crate::utils::sugg::Sugg;
-use crate::syntax::ast::{LitKind, CRATE_NODE_ID};
+use matches::matches;
+use rustc::hir::intravisit::FnKind;
+use rustc::hir::*;
+use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
+use rustc::ty;
+use rustc::{declare_tool_lint, lint_array};
+use rustc_errors::Applicability;
+use syntax::ast::LitKind;
+use syntax::source_map::{ExpnFormat, Span};
+
use crate::consts::{constant, Constant};
-use crate::rustc_errors::Applicability;
-
-/// **What it does:** Checks for function arguments and let bindings denoted as
-/// `ref`.
-///
-/// **Why is this bad?** The `ref` declaration makes the function take an owned
-/// value, but turns the argument into a reference (which means that the value
-/// is destroyed when exiting the function). This adds not much value: either
-/// take a reference type, or take an owned value and create references in the
-/// body.
-///
-/// For let bindings, `let x = &foo;` is preferred over `let ref x = foo`. The
-/// type of `x` is more obvious with the former.
-///
-/// **Known problems:** If the argument is dereferenced within the function,
-/// removing the `ref` will lead to errors. This can be fixed by removing the
-/// dereferences, e.g. changing `*x` to `x` within the function.
-///
-/// **Example:**
-/// ```rust
-/// fn foo(ref x: u8) -> bool { .. }
-/// ```
+use crate::utils::sugg::Sugg;
+use crate::utils::{
+ get_item_name, get_parent_expr, implements_trait, in_constant, in_macro, is_integer_literal, iter_input_pats,
+ last_path_segment, match_qpath, match_trait_method, paths, snippet, span_lint, span_lint_and_then, walk_ptrs_ty,
+ SpanlessEq,
+};
+
declare_clippy_lint! {
+ /// **What it does:** Checks for function arguments and let bindings denoted as
+ /// `ref`.
+ ///
+ /// **Why is this bad?** The `ref` declaration makes the function take an owned
+ /// value, but turns the argument into a reference (which means that the value
+ /// is destroyed when exiting the function). This adds not much value: either
+ /// take a reference type, or take an owned value and create references in the
+ /// body.
+ ///
+ /// For let bindings, `let x = &foo;` is preferred over `let ref x = foo`. The
+ /// type of `x` is more obvious with the former.
+ ///
+ /// **Known problems:** If the argument is dereferenced within the function,
+ /// removing the `ref` will lead to errors. This can be fixed by removing the
+ /// dereferences, e.g., changing `*x` to `x` within the function.
+ ///
+ /// **Example:**
+ /// ```rust
+ /// fn foo(ref x: u8) -> bool {
+ /// ..
+ /// }
+ /// ```
pub TOPLEVEL_REF_ARG,
style,
"an entire binding declared as `ref`, in a function argument or a `let` statement"
}
-/// **What it does:** Checks for comparisons to NaN.
-///
-/// **Why is this bad?** NaN does not compare meaningfully to anything – not
-/// even itself – so those comparisons are simply wrong.
-///
-/// **Known problems:** None.
-///
-/// **Example:**
-/// ```rust
-/// x == NAN
-/// ```
declare_clippy_lint! {
+ /// **What it does:** Checks for comparisons to NaN.
+ ///
+ /// **Why is this bad?** NaN does not compare meaningfully to anything – not
+ /// even itself – so those comparisons are simply wrong.
+ ///
+ /// **Known problems:** None.
+ ///
+ /// **Example:**
+ /// ```ignore
+ /// x == NAN
+ /// ```
pub CMP_NAN,
correctness,
"comparisons to NAN, which will always return false, probably not intended"
}
-/// **What it does:** Checks for (in-)equality comparisons on floating-point
-/// values (apart from zero), except in functions called `*eq*` (which probably
-/// implement equality for a type involving floats).
-///
-/// **Why is this bad?** Floating point calculations are usually imprecise, so
-/// asking if two values are *exactly* equal is asking for trouble. For a good
-/// guide on what to do, see [the floating point
-/// guide](http://www.floating-point-gui.de/errors/comparison).
-///
-/// **Known problems:** None.
-///
-/// **Example:**
-/// ```rust
-/// y == 1.23f64
-/// y != x // where both are floats
-/// ```
declare_clippy_lint! {
+ /// **What it does:** Checks for (in-)equality comparisons on floating-point
+ /// values (apart from zero), except in functions called `*eq*` (which probably
+ /// implement equality for a type involving floats).
+ ///
+ /// **Why is this bad?** Floating point calculations are usually imprecise, so
+ /// asking if two values are *exactly* equal is asking for trouble. For a good
+ /// guide on what to do, see [the floating point
+ /// guide](http://www.floating-point-gui.de/errors/comparison).
+ ///
+ /// **Known problems:** None.
+ ///
+ /// **Example:**
+ /// ```ignore
+ /// y == 1.23f64
+ /// y != x // where both are floats
+ /// ```
pub FLOAT_CMP,
correctness,
"using `==` or `!=` on float values instead of comparing difference with an epsilon"
}
-/// **What it does:** Checks for conversions to owned values just for the sake
-/// of a comparison.
-///
-/// **Why is this bad?** The comparison can operate on a reference, so creating
-/// an owned value effectively throws it away directly afterwards, which is
-/// needlessly consuming code and heap space.
-///
-/// **Known problems:** None.
-///
-/// **Example:**
-/// ```rust
-/// x.to_owned() == y
-/// ```
declare_clippy_lint! {
+ /// **What it does:** Checks for conversions to owned values just for the sake
+ /// of a comparison.
+ ///
+ /// **Why is this bad?** The comparison can operate on a reference, so creating
+ /// an owned value effectively throws it away directly afterwards, which is
+ /// needlessly consuming code and heap space.
+ ///
+ /// **Known problems:** None.
+ ///
+ /// **Example:**
+ /// ```rust
+ /// x.to_owned() == y
+ /// ```
pub CMP_OWNED,
perf,
- "creating owned instances for comparing with others, e.g. `x == \"foo\".to_string()`"
+ "creating owned instances for comparing with others, e.g., `x == \"foo\".to_string()`"
}
-/// **What it does:** Checks for getting the remainder of a division by one.
-///
-/// **Why is this bad?** The result can only ever be zero. No one will write
-/// such code deliberately, unless trying to win an Underhanded Rust
-/// Contest. Even for that contest, it's probably a bad idea. Use something more
-/// underhanded.
-///
-/// **Known problems:** None.
-///
-/// **Example:**
-/// ```rust
-/// x % 1
-/// ```
declare_clippy_lint! {
+ /// **What it does:** Checks for getting the remainder of a division by one.
+ ///
+ /// **Why is this bad?** The result can only ever be zero. No one will write
+ /// such code deliberately, unless trying to win an Underhanded Rust
+ /// Contest. Even for that contest, it's probably a bad idea. Use something more
+ /// underhanded.
+ ///
+ /// **Known problems:** None.
+ ///
+ /// **Example:**
+ /// ```ignore
+ /// x % 1
+ /// ```
pub MODULO_ONE,
correctness,
"taking a number modulo 1, which always returns 0"
}
-/// **What it does:** Checks for patterns in the form `name @ _`.
-///
-/// **Why is this bad?** It's almost always more readable to just use direct
-/// bindings.
-///
-/// **Known problems:** None.
-///
-/// **Example:**
-/// ```rust
-/// match v {
-/// Some(x) => (),
-/// y @ _ => (), // easier written as `y`,
-/// }
-/// ```
declare_clippy_lint! {
+ /// **What it does:** Checks for patterns in the form `name @ _`.
+ ///
+ /// **Why is this bad?** It's almost always more readable to just use direct
+ /// bindings.
+ ///
+ /// **Known problems:** None.
+ ///
+ /// **Example:**
+ /// ```ignore
+ /// match v {
+ /// Some(x) => (),
+ /// y @ _ => (), // easier written as `y`,
+ /// }
+ /// ```
pub REDUNDANT_PATTERN,
style,
"using `name @ _` in a pattern"
}
-/// **What it does:** Checks for the use of bindings with a single leading
-/// underscore.
-///
-/// **Why is this bad?** A single leading underscore is usually used to indicate
-/// that a binding will not be used. Using such a binding breaks this
-/// expectation.
-///
-/// **Known problems:** The lint does not work properly with desugaring and
-/// macro, it has been allowed in the mean time.
-///
-/// **Example:**
-/// ```rust
-/// let _x = 0;
-/// let y = _x + 1; // Here we are using `_x`, even though it has a leading
-/// // underscore. We should rename `_x` to `x`
-/// ```
declare_clippy_lint! {
+ /// **What it does:** Checks for the use of bindings with a single leading
+ /// underscore.
+ ///
+ /// **Why is this bad?** A single leading underscore is usually used to indicate
+ /// that a binding will not be used. Using such a binding breaks this
+ /// expectation.
+ ///
+ /// **Known problems:** The lint does not work properly with desugaring and
+ /// macro, it has been allowed in the mean time.
+ ///
+ /// **Example:**
+ /// ```rust
+ /// let _x = 0;
+ /// let y = _x + 1; // Here we are using `_x`, even though it has a leading
+ /// // underscore. We should rename `_x` to `x`
+ /// ```
pub USED_UNDERSCORE_BINDING,
pedantic,
"using a binding which is prefixed with an underscore"
}
-/// **What it does:** Checks for the use of short circuit boolean conditions as
-/// a
-/// statement.
-///
-/// **Why is this bad?** Using a short circuit boolean condition as a statement
-/// may hide the fact that the second part is executed or not depending on the
-/// outcome of the first part.
-///
-/// **Known problems:** None.
-///
-/// **Example:**
-/// ```rust
-/// f() && g(); // We should write `if f() { g(); }`.
-/// ```
declare_clippy_lint! {
+ /// **What it does:** Checks for the use of short circuit boolean conditions as
+ /// a
+ /// statement.
+ ///
+ /// **Why is this bad?** Using a short circuit boolean condition as a statement
+ /// may hide the fact that the second part is executed or not depending on the
+ /// outcome of the first part.
+ ///
+ /// **Known problems:** None.
+ ///
+ /// **Example:**
+ /// ```rust
+ /// f() && g(); // We should write `if f() { g(); }`.
+ /// ```
pub SHORT_CIRCUIT_STATEMENT,
complexity,
"using a short circuit boolean condition as a statement"
}
-/// **What it does:** Catch casts from `0` to some pointer type
-///
-/// **Why is this bad?** This generally means `null` and is better expressed as
-/// {`std`, `core`}`::ptr::`{`null`, `null_mut`}.
-///
-/// **Known problems:** None.
-///
-/// **Example:**
-///
-/// ```rust
-/// 0 as *const u32
-/// ```
declare_clippy_lint! {
+ /// **What it does:** Catch casts from `0` to some pointer type
+ ///
+ /// **Why is this bad?** This generally means `null` and is better expressed as
+ /// {`std`, `core`}`::ptr::`{`null`, `null_mut`}.
+ ///
+ /// **Known problems:** None.
+ ///
+ /// **Example:**
+ ///
+ /// ```ignore
+ /// 0 as *const u32
+ /// ```
pub ZERO_PTR,
style,
"using 0 as *{const, mut} T"
}
-/// **What it does:** Checks for (in-)equality comparisons on floating-point
-/// value and constant, except in functions called `*eq*` (which probably
-/// implement equality for a type involving floats).
-///
-/// **Why is this bad?** Floating point calculations are usually imprecise, so
-/// asking if two values are *exactly* equal is asking for trouble. For a good
-/// guide on what to do, see [the floating point
-/// guide](http://www.floating-point-gui.de/errors/comparison).
-///
-/// **Known problems:** None.
-///
-/// **Example:**
-/// ```rust
-/// const ONE == 1.00f64
-/// x == ONE // where both are floats
-/// ```
declare_clippy_lint! {
+ /// **What it does:** Checks for (in-)equality comparisons on floating-point
+ /// value and constant, except in functions called `*eq*` (which probably
+ /// implement equality for a type involving floats).
+ ///
+ /// **Why is this bad?** Floating point calculations are usually imprecise, so
+ /// asking if two values are *exactly* equal is asking for trouble. For a good
+ /// guide on what to do, see [the floating point
+ /// guide](http://www.floating-point-gui.de/errors/comparison).
+ ///
+ /// **Known problems:** None.
+ ///
+ /// **Example:**
+ /// ```rust
+ /// const ONE = 1.00f64;
+ /// x == ONE // where both are floats
+ /// ```
pub FLOAT_CMP_CONST,
restriction,
"using `==` or `!=` on float constants instead of comparing difference with an epsilon"
FLOAT_CMP_CONST
)
}
+
+ fn name(&self) -> &'static str {
+ "MiscLints"
+ }
}
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
decl: &'tcx FnDecl,
body: &'tcx Body,
_: Span,
- _: NodeId,
+ _: HirId,
) {
if let FnKind::Closure(_) = k {
// Does not apply to closures
}
for arg in iter_input_pats(decl, body) {
match arg.pat.node {
- PatKind::Binding(BindingAnnotation::Ref, _, _, _) |
- PatKind::Binding(BindingAnnotation::RefMut, _, _, _) => {
+ PatKind::Binding(BindingAnnotation::Ref, ..) | PatKind::Binding(BindingAnnotation::RefMut, ..) => {
span_lint(
cx,
TOPLEVEL_REF_ARG,
fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, s: &'tcx Stmt) {
if_chain! {
- if let StmtKind::Decl(ref d, _) = s.node;
- if let DeclKind::Local(ref l) = d.node;
- if let PatKind::Binding(an, _, i, None) = l.pat.node;
+ if let StmtKind::Local(ref l) = s.node;
+ if let PatKind::Binding(an, .., i, None) = l.pat.node;
if let Some(ref init) = l.init;
then {
if an == BindingAnnotation::Ref || an == BindingAnnotation::RefMut {
l.pat.span,
"`ref` on an entire `let` pattern is discouraged, take a reference with `&` instead",
|db| {
- db.span_suggestion_with_applicability(
+ db.span_suggestion(
s.span,
"try",
format!(
tyopt=tyopt,
initref=initref,
),
- Applicability::Unspecified,
+ Applicability::MachineApplicable, // snippet
);
}
);
}
};
if_chain! {
- if let StmtKind::Semi(ref expr, _) = s.node;
+ if let StmtKind::Semi(ref expr) = s.node;
if let ExprKind::Binary(ref binop, ref a, ref b) = expr.node;
if binop.node == BinOpKind::And || binop.node == BinOpKind::Or;
if let Some(sugg) = Sugg::hir_opt(cx, a);
"boolean short circuit operator in statement may be clearer using an explicit test",
|db| {
let sugg = if binop.node == BinOpKind::Or { !sugg } else { sugg };
- db.span_suggestion_with_applicability(
- s.span,
+ db.span_suggestion(
+ s.span,
"replace it with",
format!(
"if {} {{ {}; }}",
- sugg,
+ sugg,
&snippet(cx, b.span, ".."),
),
- Applicability::Unspecified,
+ Applicability::MachineApplicable, // snippet
);
});
}
}
if let Some(name) = get_item_name(cx, expr) {
let name = name.as_str();
- if name == "eq" || name == "ne" || name == "is_nan" || name.starts_with("eq_")
+ if name == "eq"
+ || name == "ne"
+ || name == "is_nan"
+ || name.starts_with("eq_")
|| name.ends_with("_eq")
{
return;
let lhs = Sugg::hir(cx, left, "..");
let rhs = Sugg::hir(cx, right, "..");
- db.span_suggestion_with_applicability(
+ db.span_suggestion(
expr.span,
"consider comparing them within some error",
format!("({}).abs() < error", lhs - rhs),
- Applicability::Unspecified,
+ Applicability::MachineApplicable, // snippet
);
db.span_note(expr.span, "std::f32::EPSILON and std::f64::EPSILON are available.");
});
}
fn check_pat(&mut self, cx: &LateContext<'a, 'tcx>, pat: &'tcx Pat) {
- if let PatKind::Binding(_, _, ident, Some(ref right)) = pat.node {
+ if let PatKind::Binding(.., ident, Some(ref right)) = pat.node {
if let PatKind::Wild = right.node {
span_lint(
cx,
REDUNDANT_PATTERN,
pat.span,
- &format!("the `{} @ _` pattern can be written as just `{}`", ident.name, ident.name),
+ &format!(
+ "the `{} @ _` pattern can be written as just `{}`",
+ ident.name, ident.name
+ ),
);
}
}
}
fn check_nan(cx: &LateContext<'_, '_>, path: &Path, expr: &Expr) {
- if !in_constant(cx, expr.id) {
+ if !in_constant(cx, expr.hir_id) {
if let Some(seg) = path.segments.last() {
if seg.ident.name == "NAN" {
span_lint(
CMP_NAN,
expr.span,
"doomed comparison with NAN, use `std::{f32,f64}::is_nan()` instead",
- );
+ );
}
}
}
if let Some((_, res)) = constant(cx, cx.tables, expr) {
res
} else {
- false
+ false
}
}
return;
}
},
- ExprKind::Call(ref path, ref v) if v.len() == 1 => if let ExprKind::Path(ref path) = path.node {
- if match_qpath(path, &["String", "from_str"]) || match_qpath(path, &["String", "from"]) {
- (cx.tables.expr_ty_adjusted(&v[0]), snippet(cx, v[0].span, ".."))
+ ExprKind::Call(ref path, ref v) if v.len() == 1 => {
+ if let ExprKind::Path(ref path) = path.node {
+ if match_qpath(path, &["String", "from_str"]) || match_qpath(path, &["String", "from"]) {
+ (cx.tables.expr_ty_adjusted(&v[0]), snippet(cx, v[0].span, ".."))
+ } else {
+ return;
+ }
} else {
return;
}
- } else {
- return;
},
_ => return,
};
None => return,
};
- // *arg impls PartialEq<other>
- if !arg_ty
- .builtin_deref(true)
- .map_or(false, |tam| implements_trait(cx, tam.ty, partial_eq_trait_id, &[other_ty.into()]))
- // arg impls PartialEq<*other>
- && !other_ty
- .builtin_deref(true)
- .map_or(false, |tam| implements_trait(cx, arg_ty, partial_eq_trait_id, &[tam.ty.into()]))
- // arg impls PartialEq<other>
- && !implements_trait(cx, arg_ty, partial_eq_trait_id, &[other_ty.into()])
- {
+ let deref_arg_impl_partial_eq_other = arg_ty.builtin_deref(true).map_or(false, |tam| {
+ implements_trait(cx, tam.ty, partial_eq_trait_id, &[other_ty.into()])
+ });
+ let arg_impl_partial_eq_deref_other = other_ty.builtin_deref(true).map_or(false, |tam| {
+ implements_trait(cx, arg_ty, partial_eq_trait_id, &[tam.ty.into()])
+ });
+ let arg_impl_partial_eq_other = implements_trait(cx, arg_ty, partial_eq_trait_id, &[other_ty.into()]);
+
+ if !deref_arg_impl_partial_eq_other && !arg_impl_partial_eq_deref_other && !arg_impl_partial_eq_other {
return;
}
+ let other_gets_derefed = match other.node {
+ ExprKind::Unary(UnDeref, _) => true,
+ _ => false,
+ };
+
+ let lint_span = if other_gets_derefed {
+ expr.span.to(other.span)
+ } else {
+ expr.span
+ };
+
span_lint_and_then(
cx,
CMP_OWNED,
- expr.span,
+ lint_span,
"this creates an owned instance just for comparison",
|db| {
- // this is as good as our recursion check can get, we can't prove that the
- // current function is
- // called by
- // PartialEq::eq, but we can at least ensure that this code is not part of it
- let parent_fn = cx.tcx.hir.get_parent(expr.id);
- let parent_impl = cx.tcx.hir.get_parent(parent_fn);
- if parent_impl != CRATE_NODE_ID {
- if let Node::Item(item) = cx.tcx.hir.get(parent_impl) {
- if let ItemKind::Impl(.., Some(ref trait_ref), _, _) = item.node {
- if trait_ref.path.def.def_id() == partial_eq_trait_id {
- // we are implementing PartialEq, don't suggest not doing `to_owned`, otherwise
- // we go into
- // recursion
- db.span_label(expr.span, "try calling implementing the comparison without allocating");
- return;
- }
- }
- }
+ // This also catches `PartialEq` implementations that call `to_owned`.
+ if other_gets_derefed {
+ db.span_label(lint_span, "try implementing the comparison without allocating");
+ return;
}
- db.span_suggestion_with_applicability(
- expr.span,
+
+ let try_hint = if deref_arg_impl_partial_eq_other {
+ // suggest deref on the left
+ format!("*{}", snip)
+ } else {
+ // suggest dropping the to_owned on the left
+ snip.to_string()
+ };
+
+ db.span_suggestion(
+ lint_span,
"try",
- snip.to_string(),
- Applicability::Unspecified,
+ try_hint,
+ Applicability::MachineApplicable, // snippet
);
},
);
}
}
-/// Test whether an expression is in a macro expansion (e.g. something
-/// generated by
-/// `#[derive(...)`] or the like).
+/// Tests whether an expression is in a macro expansion (e.g., something
+/// generated by `#[derive(...)]` or the like).
fn in_attributes_expansion(expr: &Expr) -> bool {
expr.span
.ctxt()
.map_or(false, |info| matches!(info.format, ExpnFormat::MacroAttribute(_)))
}
-/// Test whether `def` is a variable defined outside a macro.
+/// Tests whether `def` is a variable defined outside a macro.
fn non_macro_local(cx: &LateContext<'_, '_>, def: &def::Def) -> bool {
match *def {
- def::Def::Local(id) | def::Def::Upvar(id, _, _) => !in_macro(cx.tcx.hir.span(id)),
+ def::Def::Local(id) | def::Def::Upvar(id, _, _) => !in_macro(cx.tcx.hir().span(id)),
_ => false,
}
}
if let ExprKind::Lit(ref lit) = e.node;
if let LitKind::Int(value, ..) = lit.node;
if value == 0;
- if !in_constant(cx, e.id);
+ if !in_constant(cx, e.hir_id);
then {
let msg = match mutbl {
Mutability::MutMutable => "`0 as *mut _` detected. Consider using `ptr::null_mut()`",
projects / rust.git / blobdiff