nordic-dev.net/rust
nordic-dev.net/rust
2
0
Fork 0
mirror of https://github.com/rust-lang/rust.git synced 2024-12-19 03:54:40 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #492 from Manishearth/wiki

Browse Source 
added wiki comments + wiki-generating python script
This commit is contained in:
Manish Goregaokar 2015-12-11 09:20:19 -05:00
commit 6aa656a910
28 changed files with 668 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
src/approx_const.rs
View File

@ -6,6 +6,13 @@ use syntax::ast::Lit_::*;
use syntax::ast::Lit;
use syntax::ast::FloatTy::*;
/// **What it does:** This lint checks for floating point literals that approximate constants which are defined in [`std::f32::consts`](https://doc.rust-lang.org/stable/std/f32/consts/#constants) or [`std::f64::consts`](https://doc.rust-lang.org/stable/std/f64/consts/#constants), respectively, suggesting to use the predefined constant. This lint is `Warn` by default.
///
/// **Why is this bad?** Usually, the definition in the standard library is more precise than what people come up with. If you find that your definition is actually more precise, please [file a Rust issue](https://github.com/rust-lang/rust/issues).
///
/// **Known problems:** If you happen to have a value that is within 1/8192 of a known constant, but is not *and should not* be the same, this lint will report your value anyway. We have not yet noticed any false positives in code we tested clippy with (this includes servo), but YMMV.
///
/// **Example:** `let x = 3.14;`
declare_lint! {
pub APPROX_CONSTANT,
Warn,

13
src/attrs.rs
View File

@ -8,6 +8,19 @@ use syntax::attr::*;
use syntax::ast::{Attribute, MetaList, MetaWord};
use utils::{in_macro, match_path, span_lint};
/// **What it does:** This lint warns on items annotated with `#[inline(always)]`, unless the annotated function is empty or simply panics.
///
/// **Why is this bad?** While there are valid uses of this annotation (and once you know when to use it, by all means `allow` this lint), it's a common newbie-mistake to pepper one's code with it.
///
/// As a rule of thumb, before slapping `#[inline(always)]` on a function, measure if that additional function call really affects your runtime profile sufficiently to make up for the increase in compile time.
///
/// **Known problems:** False positives, big time. This lint is meant to be deactivated by everyone doing serious performance work. This means having done the measurement.
///
/// **Example:**
/// ```
/// #[inline(always)]
/// fn not_quite_hot_code(..) { ... }
/// ```
declare_lint! { pub INLINE_ALWAYS, Warn,
"`#[inline(always)]` is a bad idea in most cases" }

35
src/bit_mask.rs
View File

@ -8,6 +8,27 @@ use syntax::ast::Lit_::*;
use utils::span_lint;
/// **What it does:** This lint checks for incompatible bit masks in comparisons. It is `Warn` by default.
///
/// The formula for detecting if an expression of the type `_ <bit_op> m <cmp_op> c` (where `<bit_op>`
/// is one of {`&`, '|'} and `<cmp_op>` is one of {`!=`, `>=`, `>`, `!=`, `>=`, `>`}) can be determined from the following table:
///
/// |Comparison |Bit-Op|Example |is always|Formula |
/// |------------|------|------------|---------|----------------------|
/// |`==` or `!=`| `&` |`x & 2 == 3`|`false` |`c & m != c` |
/// |`<` or `>=`| `&` |`x & 2 < 3` |`true` |`m < c` |
/// |`>` or `<=`| `&` |`x & 1 > 1` |`false` |`m <= c` |
/// |`==` or `!=`| `|` |`x | 1 == 0`|`false` |`c | m != c` |
/// |`<` or `>=`| `|` |`x | 1 < 1` |`false` |`m >= c` |
/// |`<=` or `>` | `|` |`x | 1 > 0` |`true` |`m > c` |
///
/// **Why is this bad?** If the bits that the comparison cares about are always set to zero or one by the bit mask, the comparison is constant `true` or `false` (depending on mask, compared value, and operators).
///
/// So the code is actively misleading, and the only reason someone would write this intentionally is to win an underhanded Rust contest or create a test-case for this lint.
///
/// **Known problems:** None
///
/// **Example:** `x & 1 == 2` (also see table above)
declare_lint! {
pub BAD_BIT_MASK,
Warn,
@ -15,6 +36,20 @@ declare_lint! {
(because in the example `select` containing bits that `mask` doesn't have)"
}
/// **What it does:** This lint checks for bit masks in comparisons which can be removed without changing the outcome. The basic structure can be seen in the following table:
///
/// |Comparison|Bit-Op |Example |equals |
/// |----------|---------|-----------|-------|
/// |`>` / `<=`|`|` / `^`|`x | 2 > 3`|`x > 3`|
/// |`<` / `>=`|`|` / `^`|`x ^ 1 < 4`|`x < 4`|
///
/// This lint is `Warn` by default.
///
/// **Why is this bad?** Not equally evil as [`bad_bit_mask`](#bad_bit_mask), but still a bit misleading, because the bit mask is ineffective.
///
/// **Known problems:** False negatives: This lint will only match instances where we have figured out the math (which is for a power-of-two compared value). This means things like `x | 1 >= 7` (which would be better written as `x >= 6`) will not be reported (but bit masks like this are fairly uncommon).
///
/// **Example:** `x | 1 > 3` (also see table above)
declare_lint! {
pub INEFFECTIVE_BIT_MASK,
Warn,

7
src/collapsible_if.rs
View File

@ -18,6 +18,13 @@ use syntax::codemap::Spanned;
use utils::{in_macro, span_help_and_lint, snippet, snippet_block};
/// **What it does:** This lint checks for nested `if`-statements which can be collapsed by `&&`-combining their conditions. It is `Warn` by default.
///
/// **Why is this bad?** Each `if`-statement adds one level of nesting, which makes code look more complex than it really is.
///
/// **Known problems:** None
///
/// **Example:** `if x { if y { .. } }`
declare_lint! {
pub COLLAPSIBLE_IF,
Warn,

7
src/eq_op.rs
View File

@ -6,6 +6,13 @@ use syntax::ptr::P;
use consts::constant;
use utils::span_lint;
/// **What it does:** This lint checks for equal operands to comparisons and bitwise binary operators (`&`, `|` and `^`). It is `Warn` by default.
///
/// **Why is this bad?** This is usually just a typo.
///
/// **Known problems:** False negatives: We had some false positives regarding calls (notably [racer](https://github.com/phildawes/racer) had one instance of `x.pop() && x.pop()`), so we removed matching any function or method calls. We may introduce a whitelist of known pure functions in the future.
///
/// **Example:** `x + 1 == x + 1`
declare_lint! {
pub EQ_OP,
Warn,

7
src/eta_reduction.rs
View File

@ -9,6 +9,13 @@ use utils::{snippet_opt, span_lint_and_then, is_adjusted};
pub struct EtaPass;
/// **What it does:** This lint checks for closures which just call another function where the function can be called directly. `unsafe` functions or calls where types get adjusted are ignored. It is `Warn` by default.
///
/// **Why is this bad?** Needlessly creating a closure just costs heap space and adds code for no benefit.
///
/// **Known problems:** None
///
/// **Example:** `xs.map(|x| foo(x))` where `foo(_)` is a plain function that takes the exact argument type of `x`.
declare_lint!(pub REDUNDANT_CLOSURE, Warn,
"using redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`)");

7
src/identity_op.rs
View File

@ -6,6 +6,13 @@ use consts::{constant_simple, is_negative};
use consts::Constant::ConstantInt;
use utils::{span_lint, snippet, in_macro};
/// **What it does:** This lint checks for identity operations, e.g. `x + 0`. It is `Warn` by default.
///
/// **Why is this bad?** This code can be removed without changing the meaning. So it just obscures what's going on. Delete it mercilessly.
///
/// **Known problems:** None
///
/// **Example:** `x / 1 + 0 * 1 - 0 | 0`
declare_lint! { pub IDENTITY_OP, Warn,
"using identity operations, e.g. `x + 0` or `y / 1`" }

19
src/len_zero.rs
View File

@ -11,10 +11,29 @@ use syntax::ast::Lit;
use utils::{get_item_name, snippet, span_lint, walk_ptrs_ty};
/// **What it does:** This lint checks for getting the length of something via `.len()` just to compare to zero, and suggests using `.is_empty()` where applicable. It is `Warn` by default.
///
/// **Why is this bad?** Some structures can answer `.is_empty()` much faster than calculating their length. So it is good to get into the habit of using `.is_empty()`, and having it is cheap. Besides, it makes the intent clearer than a comparison.
///
/// **Known problems:** None
///
/// **Example:** `if x.len() == 0 { .. }`
declare_lint!(pub LEN_ZERO, Warn,
"checking `.len() == 0` or `.len() > 0` (or similar) when `.is_empty()` \
could be used instead");
/// **What it does:** This lint checks for items that implement `.len()` but not `.is_empty()`. It is `Warn` by default.
///
/// **Why is this bad?** It is good custom to have both methods, because for some data structures, asking about the length will be a costly operation, whereas `.is_empty()` can usually answer in constant time. Also it used to lead to false positives on the [`len_zero`](#len_zero) lint – currently that lint will ignore such entities.
///
/// **Known problems:** None
///
/// **Example:**
/// ```
/// impl X {
/// fn len(&self) -> usize { .. }
/// }
/// ```
declare_lint!(pub LEN_WITHOUT_IS_EMPTY, Warn,
"traits and impls that have `.len()` but not `.is_empty()`");

7
src/lifetimes.rs
View File

@ -8,6 +8,13 @@ use std::collections::{HashSet, HashMap};
use utils::{in_external_macro, span_lint};
/// **What it does:** This lint checks for lifetime annotations which can be removed by relying on lifetime elision. It is `Warn` by default.
///
/// **Why is this bad?** The additional lifetimes make the code look more complicated, while there is nothing out of the ordinary going on. Removing them leads to more readable code.
///
/// **Known problems:** Potential false negatives: we bail out if the function has a `where` clause where lifetimes are mentioned.
///
/// **Example:** `fn in_and_out<'a>(x: &'a u8, y: u8) -> &'a u8 { x }`
declare_lint!(pub NEEDLESS_LIFETIMES, Warn,
"using explicit lifetimes for references in function arguments when elision rules \
would allow omitting them");

75
src/loops.rs
View File

@ -15,28 +15,103 @@ use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type,
get_enclosing_block};
use utils::{VEC_PATH, LL_PATH};
/// **What it does:** This lint checks for looping over the range of `0..len` of some collection just to get the values by index. It is `Warn` by default.
///
/// **Why is this bad?** Just iterating the collection itself makes the intent more clear and is probably faster.
///
/// **Known problems:** None
///
/// **Example:**
/// ```
/// for i in 0..vec.len() {
/// println!("{}", vec[i]);
/// }
/// ```
declare_lint!{ pub NEEDLESS_RANGE_LOOP, Warn,
"for-looping over a range of indices where an iterator over items would do" }
/// **What it does:** This lint checks for loops on `x.iter()` where `&x` will do, and suggest the latter. It is `Warn` by default.
///
/// **Why is this bad?** Readability.
///
/// **Known problems:** False negatives. We currently only warn on some known types.
///
/// **Example:** `for x in y.iter() { .. }` (where y is a `Vec` or slice)
declare_lint!{ pub EXPLICIT_ITER_LOOP, Warn,
"for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do" }
/// **What it does:** This lint checks for loops on `x.next()`. It is `Warn` by default.
///
/// **Why is this bad?** `next()` returns either `Some(value)` if there was a value, or `None` otherwise. The insidious thing is that `Option<_>` implements `IntoIterator`, so that possibly one value will be iterated, leading to some hard to find bugs. No one will want to write such code [except to win an Underhanded Rust Contest](https://www.reddit.com/r/rust/comments/3hb0wm/underhanded_rust_contest/cu5yuhr).
///
/// **Known problems:** None
///
/// **Example:** `for x in y.next() { .. }`
declare_lint!{ pub ITER_NEXT_LOOP, Warn,
"for-looping over `_.next()` which is probably not intended" }
/// **What it does:** This lint detects `loop + match` combinations that are easier written as a `while let` loop.
///
/// **Why is this bad?** The `while let` loop is usually shorter and more readable
///
/// **Known problems:** Sometimes the wrong binding is displayed (#383)
///
/// **Example:**
///
/// ```
/// loop {
/// let x = match y {
/// Some(x) => x,
/// None => break,
/// }
/// // .. do something with x
/// }
/// // is easier written as
/// while let Some(x) = y {
/// // .. do something with x
/// }
/// ```
declare_lint!{ pub WHILE_LET_LOOP, Warn,
"`loop { if let { ... } else break }` can be written as a `while let` loop" }
/// **What it does:** This lint checks for using `collect()` on an iterator without using the result. It is `Warn` by default.
///
/// **Why is this bad?** It is more idiomatic to use a `for` loop over the iterator instead.
///
/// **Known problems:** None
///
/// **Example:** `vec.iter().map(|x| /* some operation returning () */).collect::<Vec<_>>();`
declare_lint!{ pub UNUSED_COLLECT, Warn,
"`collect()`ing an iterator without using the result; this is usually better \
written as a for loop" }
/// **What it does:** This lint checks for loops over ranges `x..y` where both `x` and `y` are constant and `x` is greater or equal to `y`, unless the range is reversed or has a negative `.step_by(_)`.
///
/// **Why is it bad?** Such loops will either be skipped or loop until wrap-around (in debug code, this may `panic!()`). Both options are probably not intended.
///
/// **Known problems:** The lint cannot catch loops over dynamically defined ranges. Doing this would require simulating all possible inputs and code paths through the program, which would be complex and error-prone.
///
/// **Examples**: `for x in 5..10-5 { .. }` (oops, stray `-`)
declare_lint!{ pub REVERSE_RANGE_LOOP, Warn,
"Iterating over an empty range, such as `10..0` or `5..5`" }
/// **What it does:** This lint checks `for` loops over slices with an explicit counter and suggests the use of `.enumerate()`. It is `Warn` by default.
///
/// **Why is it bad?** Not only is the version using `.enumerate()` more readable, the compiler is able to remove bounds checks which can lead to faster code in some instances.
///
/// **Known problems:** None.
///
/// **Example:** `for i in 0..v.len() { foo(v[i]); }` or `for i in 0..v.len() { bar(i, v[i]); }`
declare_lint!{ pub EXPLICIT_COUNTER_LOOP, Warn,
"for-looping with an explicit counter when `_.enumerate()` would do" }
/// **What it does:** This lint checks for empty `loop` expressions. It is `Warn` by default.
///
/// **Why is this bad?** Those busy loops burn CPU cycles without doing anything. Think of the environment and either block on something or at least make the thread sleep for some microseconds.
///
/// **Known problems:** None
///
/// **Example:** `loop {}`
declare_lint!{ pub EMPTY_LOOP, Warn, "empty `loop {}` detected" }
declare_lint!{ pub WHILE_LET_ON_ITERATOR, Warn, "using a while-let loop instead of a for loop on an iterator" }

43
src/matches.rs
View File

@ -6,12 +6,55 @@ use syntax::codemap::Span;
use utils::{snippet, span_lint, span_help_and_lint, in_external_macro, expr_block};
/// **What it does:** This lint checks for matches with a single arm where an `if let` will usually suffice. It is `Warn` by default.
///
/// **Why is this bad?** Just readability – `if let` nests less than a `match`.
///
/// **Known problems:** None
///
/// **Example:**
/// ```
/// match x {
/// Some(ref foo) -> bar(foo),
/// _ => ()
/// }
/// ```
declare_lint!(pub SINGLE_MATCH, Warn,
"a match statement with a single nontrivial arm (i.e, where the other arm \
is `_ => {}`) is used; recommends `if let` instead");
/// **What it does:** This lint checks for matches where all arms match a reference, suggesting to remove the reference and deref the matched expression instead. It is `Warn` by default.
///
/// **Why is this bad?** It just makes the code less readable. That reference destructuring adds nothing to the code.
///
/// **Known problems:** None
///
/// **Example:**
///
/// ```
/// match x {
/// &A(ref y) => foo(y),
/// &B => bar(),
/// _ => frob(&x),
/// }
/// ```
declare_lint!(pub MATCH_REF_PATS, Warn,
"a match has all arms prefixed with `&`; the match expression can be \
dereferenced instead");
/// **What it does:** This lint checks for matches where match expression is a `bool`. It suggests to replace the expression with an `if...else` block. It is `Warn` by default.
///
/// **Why is this bad?** It makes the code less readable.
///
/// **Known problems:** None
///
/// **Example:**
///
/// ```
/// let condition: bool = true;
/// match condition {
/// true => foo(),
/// false => bar(),
/// }
/// ```
declare_lint!(pub MATCH_BOOL, Warn,
"a match on boolean expression; recommends `if..else` block instead");

76
src/methods.rs
View File

@ -15,19 +15,95 @@ use self::OutType::*;
#[derive(Clone)]
pub struct MethodsPass;
/// **What it does:** This lint checks for `.unwrap()` calls on `Option`s. It is `Allow` by default.
///
/// **Why is this bad?** Usually it is better to handle the `None` case, or to at least call `.expect(_)` with a more helpful message. Still, for a lot of quick-and-dirty code, `unwrap` is a good choice, which is why this lint is `Allow` by default.
///
/// **Known problems:** None
///
/// **Example:** `x.unwrap()`
declare_lint!(pub OPTION_UNWRAP_USED, Allow,
"using `Option.unwrap()`, which should at least get a better message using `expect()`");
/// **What it does:** This lint checks for `.unwrap()` calls on `Result`s. It is `Allow` by default.
///
/// **Why is this bad?** `result.unwrap()` will let the thread panic on `Err` values. Normally, you want to implement more sophisticated error handling, and propagate errors upwards with `try!`.
///
/// Even if you want to panic on errors, not all `Error`s implement good messages on display. Therefore it may be beneficial to look at the places where they may get displayed. Activate this lint to do just that.
///
/// **Known problems:** None
///
/// **Example:** `x.unwrap()`
declare_lint!(pub RESULT_UNWRAP_USED, Allow,
"using `Result.unwrap()`, which might be better handled");
/// **What it does:** This lint checks for `.to_string()` method calls on values of type `&str`. It is `Warn` by default.
///
/// **Why is this bad?** This uses the whole formatting machinery just to clone a string. Using `.to_owned()` is lighter on resources. You can also consider using a [`Cow<'a, str>`](http://doc.rust-lang.org/std/borrow/enum.Cow.html) instead in some cases.
///
/// **Known problems:** None
///
/// **Example:** `s.to_string()` where `s: &str`
declare_lint!(pub STR_TO_STRING, Warn,
"using `to_string()` on a str, which should be `to_owned()`");
/// **What it does:** This lint checks for `.to_string()` method calls on values of type `String`. It is `Warn` by default.
///
/// **Why is this bad?** As our string is already owned, this whole operation is basically a no-op, but still creates a clone of the string (which, if really wanted, should be done with `.clone()`).
///
/// **Known problems:** None
///
/// **Example:** `s.to_string()` where `s: String`
declare_lint!(pub STRING_TO_STRING, Warn,
"calling `String.to_string()` which is a no-op");
/// **What it does:** This lint checks for methods that should live in a trait implementation of a `std` trait (see [llogiq's blog post](http://llogiq.github.io/2015/07/30/traits.html) for further information) instead of an inherent implementation. It is `Warn` by default.
///
/// **Why is this bad?** Implementing the traits improve ergonomics for users of the code, often with very little cost. Also people seeing a `mul(..)` method may expect `*` to work equally, so you should have good reason to disappoint them.
///
/// **Known problems:** None
///
/// **Example:**
/// ```
/// struct X;
/// impl X {
/// fn add(&self, other: &X) -> X { .. }
/// }
/// ```
declare_lint!(pub SHOULD_IMPLEMENT_TRAIT, Warn,
"defining a method that should be implementing a std trait");
/// **What it does:** This lint checks for methods with certain name prefixes and `Warn`s (by default) if the prefix doesn't match how self is taken. The actual rules are:
///
/// |Prefix |`self` taken |
/// |-------|--------------------|
/// |`as_` |`&self` or &mut self|
/// |`from_`| none |
/// |`into_`|`self` |
/// |`is_` |`&self` or none |
/// |`to_` |`&self` |
///
/// **Why is this bad?** Consistency breeds readability. If you follow the conventions, your users won't be surprised that they e.g. need to supply a mutable reference to a `as_`.. function.
///
/// **Known problems:** None
///
/// **Example**
///
/// ```
/// impl X {
/// fn as_str(self) -> &str { .. }
/// }
/// ```
declare_lint!(pub WRONG_SELF_CONVENTION, Warn,
"defining a method named with an established prefix (like \"into_\") that takes \
`self` with the wrong convention");
/// **What it does:** This is the same as [`wrong_self_convention`](#wrong_self_convention), but for public items. This lint is `Allow` by default.
///
/// **Why is this bad?** See [`wrong_self_convention`](#wrong_self_convention).
///
/// **Known problems:** Actually *renaming* the function may break clients if the function is part of the public interface. In that case, be mindful of the stability guarantees you've given your users.
///
/// **Example:**
/// ```
/// impl X {
/// pub fn as_str(self) -> &str { .. }
/// }
/// ```
declare_lint!(pub WRONG_PUB_SELF_CONVENTION, Allow,
"defining a public method named with an established prefix (like \"into_\") that takes \
`self` with the wrong convention");

7
src/minmax.rs
View File

@ -8,6 +8,13 @@ use consts::{Constant, constant_simple};
use utils::{match_def_path, span_lint};
use self::MinMax::{Min, Max};
/// **What it does:** This lint checks for expressions where `std::cmp::min` and `max` are used to clamp values, but switched so that the result is constant.
///
/// **Why is this bad?** This is in all probability not the intended outcome. At the least it hurts readability of the code.
///
/// **Known problems:** None
///
/// **Example:** `min(0, max(100, x))` will always be equal to `0`. Probably the author meant to clamp the value between 0 and 100, but has erroneously swapped `min` and `max`.
declare_lint!(pub MIN_MAX, Warn,
"`min(_, max(_, _))` (or vice versa) with bounds clamping the result \
to a constant");

50
src/misc.rs
View File

@ -13,6 +13,15 @@ use rustc::middle::const_eval::EvalHint::ExprTypeChecked;
use utils::{get_item_name, match_path, snippet, span_lint, walk_ptrs_ty, is_integer_literal};
use utils::span_help_and_lint;
/// **What it does:** This lint checks for function arguments and let bindings denoted as `ref`. It is `Warn` by default.
///
/// **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:** `fn foo(ref x: u8) -> bool { .. }`
declare_lint!(pub TOPLEVEL_REF_ARG, Warn,
"An entire binding was declared as `ref`, in a function argument (`fn foo(ref x: Bar)`), \
or a `let` statement (`let ref x = foo()`). In such cases, it is preferred to take \
@ -68,6 +77,13 @@ impl LateLintPass for TopLevelRefPass {
}
}
/// **What it does:** This lint checks for comparisons to NAN. It is `Deny` by default.
///
/// **Why is this bad?** NAN does not compare meaningfully to anything – not even itself – so those comparisons are simply wrong.
///
/// **Known problems:** None
///
/// **Example:** `x == NAN`
declare_lint!(pub CMP_NAN, Deny,
"comparisons to NAN (which will always return false, which is probably not intended)");
@ -102,6 +118,13 @@ fn check_nan(cx: &LateContext, path: &Path, span: Span) {
});
}
/// **What it does:** This lint 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). It is `Warn` by default.
///
/// **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:** `y == 1.23f64`
declare_lint!(pub FLOAT_CMP, Warn,
"using `==` or `!=` on float values (as floating-point operations \
usually involve rounding errors, it is always better to check for approximate \
@ -155,6 +178,13 @@ fn is_float(cx: &LateContext, expr: &Expr) -> bool {
}
}
/// **What it does:** This lint checks for conversions to owned values just for the sake of a comparison. It is `Warn` by default.
///
/// **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:** `x.to_owned() == y`
declare_lint!(pub CMP_OWNED, Warn,
"creating owned instances for comparing with others, e.g. `x == \"foo\".to_string()`");
@ -221,6 +251,13 @@ fn is_str_arg(cx: &LateContext, args: &[P<Expr>]) -> bool {
walk_ptrs_ty(cx.tcx.expr_ty(&args[0])).sty { true } else { false }
}
/// **What it does:** This lint checks for getting the remainder of a division by one. It is `Warn` by default.
///
/// **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:** `x % 1`
declare_lint!(pub MODULO_ONE, Warn, "taking a number modulo 1, which always returns 0");
#[derive(Copy,Clone)]
@ -244,6 +281,19 @@ impl LateLintPass for ModuloOne {
}
}
/// **What it does:** This lint 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**:
/// ```
/// match v {
/// Some(x) => (),
/// y @ _ => (), // easier written as `y`,
/// }
/// ```
declare_lint!(pub REDUNDANT_PATTERN, Warn, "using `name @ _` in a pattern");
#[derive(Copy,Clone)]

7
src/mut_mut.rs
View File

@ -4,6 +4,13 @@ use rustc::middle::ty::{TypeAndMut, TyRef};
use utils::{in_external_macro, span_lint};
/// **What it does:** This lint checks for instances of `mut mut` references. It is `Warn` by default.
///
/// **Why is this bad?** Multiple `mut`s don't add anything meaningful to the source.
///
/// **Known problems:** None
///
/// **Example:** `let x = &mut &mut y;`
declare_lint!(pub MUT_MUT, Allow,
"usage of double-mut refs, e.g. `&mut &mut ...` (either copy'n'paste error, \
or shows a fundamental misunderstanding of references)");

7
src/mut_reference.rs
View File

@ -4,6 +4,13 @@ use utils::span_lint;
use rustc::middle::ty::{TypeAndMut, TypeVariants, MethodCall, TyS};
use syntax::ptr::P;
/// **What it does:** This lint detects giving a mutable reference to a function that only requires an immutable reference.
///
/// **Why is this bad?** The immutable reference rules out all other references to the value. Also the code misleads about the intent of the call site.
///
/// **Known problems:** None
///
/// **Example** `my_vec.push(&mut value)`
declare_lint! {
pub UNNECESSARY_MUT_PASSED,
Warn,

7
src/needless_bool.rs
View File

@ -9,6 +9,13 @@ use syntax::ast::Lit_::*;
use utils::{span_lint, snippet};
/// **What it does:** This lint checks for expressions of the form `if c { true } else { false }` (or vice versa) and suggest using the condition directly. It is `Warn` by default.
///
/// **Why is this bad?** Redundant code.
///
/// **Known problems:** Maybe false positives: Sometimes, the two branches are painstakingly documented (which we of course do not detect), so they *may* have some value. Even then, the documentation can be rewritten to match the shorter code.
///
/// **Example:** `if x { false } else { true }`
declare_lint! {
pub NEEDLESS_BOOL,
Warn,

7
src/open_options.rs
View File

@ -4,6 +4,13 @@ use utils::{walk_ptrs_ty_depth, match_type, span_lint, OPEN_OPTIONS_PATH};
use syntax::codemap::{Span, Spanned};
use syntax::ast::Lit_::LitBool;
/// **What it does:** This lint checks for duplicate open options as well as combinations that make no sense. It is `Warn` by default.
///
/// **Why is this bad?** In the best case, the code will be harder to read than necessary. I don't know the worst case.
///
/// **Known problems:** None
///
/// **Example:** `OpenOptions::new().read(true).truncate(true)`
declare_lint! {
pub NONSENSICAL_OPEN_OPTIONS,
Warn,

11
src/precedence.rs
View File

@ -5,6 +5,17 @@ use syntax::ast_util::binop_to_string;
use utils::{span_lint, snippet};
/// **What it does:** This lint checks for operations where precedence may be unclear and `Warn`'s about them by default, suggesting to add parentheses. Currently it catches the following:
/// * mixed usage of arithmetic and bit shifting/combining operators without parentheses
/// * a "negative" numeric literal (which is really a unary `-` followed by a numeric literal) followed by a method call
///
/// **Why is this bad?** Because not everyone knows the precedence of those operators by heart, so expressions like these may trip others trying to reason about the code.
///
/// **Known problems:** None
///
/// **Examples:**
/// * `1 << 2 + 3` equals 32, while `(1 << 2) + 3` equals 7
/// * `-1i32.abs()` equals -1, while `(-1i32).abs()` equals 1
declare_lint!(pub PRECEDENCE, Warn,
"catches operations where precedence may be unclear. See the wiki for a \
list of cases caught");

7
src/ptr_arg.rs
View File

@ -10,6 +10,13 @@ use rustc::front::map::Node;
use utils::{span_lint, match_type};
use utils::{STRING_PATH, VEC_PATH};
/// **What it does:** This lint checks for function arguments of type `&String` or `&Vec` unless the references are mutable. It is `Warn` by default.
///
/// **Why is this bad?** Requiring the argument to be of the specific size makes the function less useful for no benefit; slices in the form of `&[T]` or `&str` usually suffice and can be obtained from other types, too.
///
/// **Known problems:** None
///
/// **Example:** `fn foo(&Vec<u32>) { .. }`
declare_lint! {
pub PTR_ARG,
Warn,

14
src/ranges.rs
View File

@ -3,10 +3,24 @@ use rustc_front::hir::*;
use syntax::codemap::Spanned;
use utils::{is_integer_literal, match_type, snippet};
/// **What it does:** This lint checks for iterating over ranges with a `.step_by(0)`, which never terminates. It is `Warn` by default.
///
/// **Why is this bad?** This very much looks like an oversight, since with `loop { .. }` there is an obvious better way to endlessly loop.
///
/// **Known problems:** None
///
/// **Example:** `for x in (5..5).step_by(0) { .. }`
declare_lint! {
pub RANGE_STEP_BY_ZERO, Warn,
"using Range::step_by(0), which produces an infinite iterator"
}
/// **What it does:** This lint checks for zipping a collection with the range of `0.._.len()`. It is `Warn` by default.
///
/// **Why is this bad?** The code is better expressed with `.enumerate()`.
///
/// **Known problems:** None
///
/// **Example:** `x.iter().zip(0..x.len())`
declare_lint! {
pub RANGE_ZIP_WITH_LEN, Warn,
"zipping iterator with a range when enumerate() would do"

14
src/returns.rs
View File

@ -6,8 +6,22 @@ use syntax::visit::FnKind;
use utils::{span_lint, snippet, match_path_ast, in_external_macro};
/// **What it does:** This lint checks for return statements at the end of a block. It is `Warn` by default.
///
/// **Why is this bad?** Removing the `return` and semicolon will make the code more rusty.
///
/// **Known problems:** None
///
/// **Example:** `fn foo(x: usize) { return x; }`
declare_lint!(pub NEEDLESS_RETURN, Warn,
"using a return statement like `return expr;` where an expression would suffice");
/// **What it does:** This lint checks for `let`-bindings, which are subsequently returned. It is `Warn` by default.
///
/// **Why is this bad?** It is just extraneous code. Remove it to make your code more rusty.
///
/// **Known problems:** None
///
/// **Example:** `{ let x = ..; x }`
declare_lint!(pub LET_AND_RETURN, Warn,
"creating a let-binding and then immediately returning it like `let x = expr; x` at \
the end of a block");

21
src/shadow.rs
View File

@ -9,11 +9,32 @@ use rustc::middle::def::Def::{DefVariant, DefStruct};
use utils::{is_from_for_desugar, in_external_macro, snippet, span_lint, span_note_and_lint};
/// **What it does:** This lint checks for bindings that shadow other bindings already in scope, while just changing reference level or mutability. It is `Allow` by default.
///
/// **Why is this bad?** Not much, in fact it's a very common pattern in Rust code. Still, some may opt to avoid it in their code base, they can set this lint to `Warn`.
///
/// **Known problems:** This lint, as the other shadowing related lints, currently only catches very simple patterns.
///
/// **Example:** `let x = &x;`
declare_lint!(pub SHADOW_SAME, Allow,
"rebinding a name to itself, e.g. `let mut x = &mut x`");
/// **What it does:** This lint checks for bindings that shadow other bindings already in scope, while reusing the original value. It is `Allow` by default.
///
/// **Why is this bad?** Not too much, in fact it's a common pattern in Rust code. Still, some argue that name shadowing like this hurts readability, because a value may be bound to different things depending on position in the code.
///
/// **Known problems:** This lint, as the other shadowing related lints, currently only catches very simple patterns.
///
/// **Example:** `let x = x + 1;`
declare_lint!(pub SHADOW_REUSE, Allow,
"rebinding a name to an expression that re-uses the original value, e.g. \
`let x = x + 1`");
/// **What it does:** This lint checks for bindings that shadow other bindings already in scope, either without a initialization or with one that does not even use the original value. This lint is `Warn` by default.
///
/// **Why is this bad?** Name shadowing can hurt readability, especially in large code bases, because it is easy to lose track of the active binding at any place in the code. This can be alleviated by either giving more specific names to bindings ore introducing more scopes to contain the bindings.
///
/// **Known problems:** This lint, as the other shadowing related lints, currently only catches very simple patterns.
///
/// **Example:** `let x = y; let x = z; // shadows the earlier binding`
declare_lint!(pub SHADOW_UNRELATED, Allow,
"The name is re-bound without even using the original value");

26
src/strings.rs
View File

@ -11,12 +11,38 @@ use eq_op::is_exp_equal;
use utils::{match_type, span_lint, walk_ptrs_ty, get_parent_expr};
use utils::STRING_PATH;
/// **What it does:** This lint matches code of the form `x = x + y` (without `let`!)
///
/// **Why is this bad?** Because this expression needs another copy as opposed to `x.push_str(y)` (in practice LLVM will usually elide it, though). Despite [llogiq](https://github.com/llogiq)'s reservations, this lint also is `allow` by default, as some people opine that it's more readable.
///
/// **Known problems:** None. Well apart from the lint being `allow` by default. :smile:
///
/// **Example:**
///
/// ```
/// let mut x = "Hello".to_owned();
/// x = x + ", World";
/// ```
declare_lint! {
pub STRING_ADD_ASSIGN,
Allow,
"using `x = x + ..` where x is a `String`; suggests using `push_str()` instead"
}
/// **What it does:** The `string_add` lint matches all instances of `x + _` where `x` is of type `String`, but only if [`string_add_assign`](#string_add_assign) does *not* match. It is `Allow` by default.
///
/// **Why is this bad?** It's not bad in and of itself. However, this particular `Add` implementation is asymmetric (the other operand need not be `String`, but `x` does), while addition as mathematically defined is symmetric, also the `String::push_str(_)` function is a perfectly good replacement. Therefore some dislike it and wish not to have it in their code.
///
/// That said, other people think that String addition, having a long tradition in other languages is actually fine, which is why we decided to make this particular lint `allow` by default.
///
/// **Known problems:** None
///
/// **Example:**
///
/// ```
/// let x = "Hello".to_owned();
/// x + ", World"
/// ```
declare_lint! {
pub STRING_ADD,
Allow,

69
src/types.rs
View File

@ -17,8 +17,26 @@ use utils::{LL_PATH, VEC_PATH};
#[allow(missing_copy_implementations)]
pub struct TypePass;
/// **What it does:** This lint checks for use of `Box<Vec<_>>` anywhere in the code.
///
/// **Why is this bad?** `Vec` already keeps its contents in a separate area on the heap. So if you `Box` it, you just add another level of indirection without any benefit whatsoever.
///
/// **Known problems:** None
///
/// **Example:** `struct X { values: Box<Vec<Foo>> }`
declare_lint!(pub BOX_VEC, Warn,
"usage of `Box<Vec<T>>`, vector elements are already on the heap");
/// **What it does:** This lint checks for usage of any `LinkedList`, suggesting to use a `Vec` or `RingBuf`.
///
/// **Why is this bad?** Gankro says:
///
/// >The TL;DR of `LinkedList` is that it's built on a massive amount of pointers and indirection. It wastes memory, it has terrible cache locality, and is all-around slow. `RingBuf`, while "only" amortized for push/pop, should be faster in the general case for almost every possible workload, and isn't even amortized at all if you can predict the capacity you need.
/// >
/// > `LinkedList`s are only really good if you're doing a lot of merging or splitting of lists. This is because they can just mangle some pointers instead of actually copying the data. Even if you're doing a lot of insertion in the middle of the list, `RingBuf` can still be better because of how expensive it is to seek to the middle of a `LinkedList`.
///
/// **Known problems:** False positives – the instances where using a `LinkedList` makes sense are few and far between, but they can still happen.
///
/// **Example:** `let x = LinkedList::new();`
declare_lint!(pub LINKEDLIST, Warn,
"usage of LinkedList, usually a vector is faster, or a more specialized data \
structure like a VecDeque");
@ -53,6 +71,13 @@ impl LateLintPass for TypePass {
#[allow(missing_copy_implementations)]
pub struct LetPass;
/// **What it does:** This lint checks for binding a unit value. It is `Warn` by default.
///
/// **Why is this bad?** A unit value cannot usefully be used anywhere. So binding one is kind of pointless.
///
/// **Known problems:** None
///
/// **Example:** `let x = { 1; };`
declare_lint!(pub LET_UNIT_VALUE, Warn,
"creating a let binding to a value of unit type, which usually can't be used afterwards");
@ -82,6 +107,13 @@ impl LateLintPass for LetPass {
}
}
/// **What it does:** This lint checks for comparisons to unit. It is `Warn` by default.
///
/// **Why is this bad?** Unit is always equal to itself, and thus is just a clumsily written constant. Mostly this happens when someone accidentally adds semicolons at the end of the operands.
///
/// **Known problems:** None
///
/// **Example:** `if { foo(); } == { bar(); } { baz(); }` is equal to `{ foo(); bar(); baz(); }`
declare_lint!(pub UNIT_CMP, Warn,
"comparing unit values (which is always `true` or `false`, respectively)");
@ -115,12 +147,42 @@ impl LateLintPass for UnitCmp {
pub struct CastPass;
/// **What it does:** This lint checks for casts from any numerical to a float type where the receiving type cannot store all values from the original type without rounding errors. This possible rounding is to be expected, so this lint is `Allow` by default.
///
/// Basically, this warns on casting any integer with 32 or more bits to `f32` or any 64-bit integer to `f64`.
///
/// **Why is this bad?** It's not bad at all. But in some applications it can be helpful to know where precision loss can take place. This lint can help find those places in the code.
///
/// **Known problems:** None
///
/// **Example:** `let x = u64::MAX; x as f64`
declare_lint!(pub CAST_PRECISION_LOSS, Allow,
"casts that cause loss of precision, e.g `x as f32` where `x: u64`");
/// **What it does:** This lint checks for casts from a signed to an unsigned numerical type. In this case, negative values wrap around to large positive values, which can be quite surprising in practice. However, as the cast works as defined, this lint is `Allow` by default.
///
/// **Why is this bad?** Possibly surprising results. You can activate this lint as a one-time check to see where numerical wrapping can arise.
///
/// **Known problems:** None
///
/// **Example:** `let y : i8 = -1; y as u64` will return 18446744073709551615
declare_lint!(pub CAST_SIGN_LOSS, Allow,
"casts from signed types to unsigned types, e.g `x as u32` where `x: i32`");
/// **What it does:** This lint checks for on casts between numerical types that may truncate large values. This is expected behavior, so the cast is `Allow` by default.
///
/// **Why is this bad?** In some problem domains, it is good practice to avoid truncation. This lint can be activated to help assess where additional checks could be beneficial.
///
/// **Known problems:** None
///
/// **Example:** `fn as_u8(x: u64) -> u8 { x as u8 }`
declare_lint!(pub CAST_POSSIBLE_TRUNCATION, Allow,
"casts that may cause truncation of the value, e.g `x as u8` where `x: u32`, or `x as i32` where `x: f32`");
/// **What it does:** This lint checks for casts from an unsigned type to a signed type of the same size. Performing such a cast is a 'no-op' for the compiler, i.e. nothing is changed at the bit level, and the binary representation of the value is reinterpreted. This can cause wrapping if the value is too big for the target signed type. However, the cast works as defined, so this lint is `Allow` by default.
///
/// **Why is this bad?** While such a cast is not bad in itself, the results can be surprising when this is not the intended behavior, as demonstrated by the example below.
///
/// **Known problems:** None
///
/// **Example:** `u32::MAX as i32` will yield a value of `-1`.
declare_lint!(pub CAST_POSSIBLE_WRAP, Allow,
"casts that may cause wrapping around the value, e.g `x as i32` where `x: u32` and `x > i32::MAX`");
@ -262,6 +324,13 @@ impl LateLintPass for CastPass {
}
}
/// **What it does:** This lint checks for types used in structs, parameters and `let` declarations above a certain complexity threshold. It is `Warn` by default.
///
/// **Why is this bad?** Too complex types make the code less readable. Consider using a `type` definition to simplify them.
///
/// **Known problems:** None
///
/// **Example:** `struct Foo { inner: Rc<Vec<Vec<Box<(u32, u32, u32, u32)>>>> }`
declare_lint!(pub TYPE_COMPLEXITY, Warn,
"usage of very complex types; recommends factoring out parts into `type` definitions");

21
src/unicode.rs
View File

@ -8,11 +8,32 @@ use unicode_normalization::UnicodeNormalization;
use utils::{snippet, span_help_and_lint};
/// **What it does:** This lint checks for the unicode zero-width space in the code. It is `Warn` by default.
///
/// **Why is this bad?** Having an invisible character in the code makes for all sorts of April fools, but otherwise is very much frowned upon.
///
/// **Known problems:** None
///
/// **Example:** You don't see it, but there may be a zero-width space somewhere in this text.
declare_lint!{ pub ZERO_WIDTH_SPACE, Deny,
"using a zero-width space in a string literal, which is confusing" }
/// **What it does:** This lint checks for non-ascii characters in string literals. It is `Allow` by default.
///
/// **Why is this bad?** Yeah, we know, the 90's called and wanted their charset back. Even so, there still are editors and other programs out there that don't work well with unicode. So if the code is meant to be used internationally, on multiple operating systems, or has other portability requirements, activating this lint could be useful.
///
/// **Known problems:** None
///
/// **Example:** `let x = "Hä?"`
declare_lint!{ pub NON_ASCII_LITERAL, Allow,
"using any literal non-ASCII chars in a string literal; suggests \
using the \\u escape instead" }
/// **What it does:** This lint checks for string literals that contain unicode in a form that is not equal to its [NFC-recomposition](http://www.unicode.org/reports/tr15/#Norm_Forms). This lint is `Allow` by default.
///
/// **Why is this bad?** If such a string is compared to another, the results may be surprising.
///
/// **Known problems** None
///
/// **Example:** You may not see it, but "à" and "à" aren't the same string. The former when escaped is actually "a\u{300}" while the latter is "\u{e0}".
declare_lint!{ pub UNICODE_NOT_NFC, Allow,
"using a unicode literal not in NFC normal form (see \
http://www.unicode.org/reports/tr15/ for further information)" }

7
src/zero_div_zero.rs
View File

@ -9,6 +9,13 @@ use consts::{Constant, constant_simple, FloatWidth};
/// 0.0/0.0 with std::f32::NaN or std::f64::NaN, depending on the precision.
pub struct ZeroDivZeroPass;
/// **What it does:** This lint checks for `0.0 / 0.0`
///
/// **Why is this bad?** It's less readable than `std::f32::NAN` or `std::f64::NAN`
///
/// **Known problems:** None
///
/// **Example** `0.0f32 / 0.0`
declare_lint!(pub ZERO_DIVIDED_BY_ZERO, Warn,
"usage of `0.0 / 0.0` to obtain NaN instead of std::f32::NaN or std::f64::NaN");

90
util/update_wiki.py Executable file
View File

@ -0,0 +1,90 @@
#!/usr/bin/env python
# Generate the wiki Home.md page from the contained doc comments
# requires the checked out wiki in ../rust-clippy.wiki/
# with -c option, print a warning and set exit status 1 if the file would be changed.
import os, re, sys
def parse_path(p="src"):
d = {}
for f in os.listdir(p):
if f.endswith(".rs"):
parse_file(d, os.path.join(p, f))
return d
START = 0
LINT = 1
def parse_file(d, f):
last_comment = []
comment = True
lint = None
with open(f) as rs:
for line in rs:
if comment:
if line.startswith("///"):
if line.startswith("/// "):
last_comment.append(line[4:])
else:
last_comment.append(line[3:])
elif line.startswith("declare_lint!"):
comment = False
else:
last_comment = []
if not comment:
l = line.strip()
m = re.search(r"pub\s+([A-Z_]+)", l)
if m:
print "found %s in %s" % (m.group(1).lower(), f)
d[m.group(1).lower()] = last_comment
last_comment = []
comment = True
if "}" in l:
print "Warning: Missing Lint-Name in", f
comment = True
PREFIX = """Welcome to the rust-clippy wiki!
Here we aim to collect further explanations on the lints clippy provides. So without further ado:
"""
WARNING = """
# A word of warning
Clippy works as a *plugin* to the compiler, which means using an unstable internal API. We have gotten quite good at keeping pace with the API evolution, but the consequence is that clippy absolutely needs to be compiled with the version of `rustc` it will run on, otherwise you will get strange errors of missing symbols."""
def write_wiki_page(d, f):
keys = d.keys()
keys.sort()
with open(f, "w") as w:
w.write(PREFIX)
for k in keys:
w.write("[`%s`](#%s)\n" % (k, k))
w.write(WARNING)
for k in keys:
w.write("\n# `%s`\n\n%s" % (k, "".join(d[k])))
def check_wiki_page(d, f):
errors = []
with open(f) as w:
for line in w:
m = re.match("# `([a-z_]+)`", line)
if m:
v = d.pop(m.group(1), "()")
if v == "()":
errors.append("Missing wiki entry: " + m.group(1))
keys = d.keys()
keys.sort()
for k in keys:
errors.append("Spurious wiki entry: " + k)
if errors:
print "\n".join(errors)
sys.exit(1)
if __name__ == "__main__":
d = parse_path()
if "-c" in sys.argv:
check_wiki_page(d, "../rust-clippy.wiki/Home.md")
else:
write_wiki_page(d, "../rust-clippy.wiki/Home.md")