We could reuse some of eq_op's machinery, we'd just have to make sure that no shadowing occurs in between.

Not sure what the heuristics for this should be. Will have to think.

It's easy to pass something like |a| f(a) when you could just pass f instead.

We explicitly say in the pointer guide that you usually shouldn't do this, return T instead and box it.

Not sure how to detect these, but it seems important.

Again, nicked from findbugs.

We can do this whenever there is no unusual flow in the first branch, which I think shouldn't be too difficult to detect.

Patterns to match:

x + 0, x - 0, x | 0, x << 0, x >> 0, x ^ 0, x * 1, x / 1, x & -1

(and vice versa, e.g. 0 + x)

Note that x & x and x | x are already handled by eq_op, so we won't include them here

This should either be .expect() or .unwrap_or_else(|| panic!()) (if they want the formatting panic provides). Ideally this would be generalised to any diverging function (fn(…) -> !) as well.

I believe we should warn about usage of unwrap() of optional types. unwrap() is essentially a pattern match with _ => panic!(), but there are often ways to continue execution of program w/o failing.

Today, it's common to find code like

extern crate term;

fn main() {
    let mut t = term::stdout().unwrap();

    t.fg(term::color::GREEN).unwrap();
    (write!(t, "hello, ")).unwrap();

    t.fg(term::color::RED).unwrap();
    (writeln!(t, "world!")).unwrap();

    t.reset().unwrap();
}

As one might learn pretty quickly, unwrap()ing a None returned by term::stdout() will lead to failure of task. This is, of course, specified in docs. But it's certainly not obvious that this is not a "safe" operation — it may lead to "crash". Not "hard" crash, because it will be handled by Rust runtime. It produces error about unwrapping None. But it's a crash nevertheless.

Warnings from compiler that .unwrap() is essentially a pattern match w/o "catch-all" case are missing here. We do get warnings when not all variants of enum are handled, so why skip it here?

unwrap() of Option<_> is a hack. It's convenient, I know — but it's the sort of convenience we disdain JavaScript and PHP for. I also understand that one is extremely likely to use unwrap() in closures because "heck, I only need to filter/map/reduce stuff, I don't want my closure to spend 5 lines!". Programmer uses it there and then forgets about it. Voila — safety is needlessly violated.

I think a programmer should be forced to actively discard any failure condition. It makes for explicit decision making about not handling an error.

In Rust, in it's present state, unwrap() is the easiest and simplest way to Get Things Done. This shortcut will be taken by many and many people who will then later complain about Rust not being secure despite claiming itself "preventing almost all crashes".

Because man that thing is useful.

Did you mean do use practically any other data structure?

Every repository I've seen that uses this for anything useful reimplements the internals to gain access to the raw links / raw nodes anyway, I doubt this will catch many/any false positives.

if x { if y { ... } } can be written as if x && y { ...}.

x == String::from_str("foo")

should be

x.as_slice() == "foo"

to avoid the allocation.

Regarding my failed attempt to fully solve #32 (which would require more typeck than external lints can currently acquire), I thought about the social contract around .len() and .is_empty(). While we don't have a Bounded trait that defines .len() and implements .is_empty(), we can still ask implementers to provide the latter.

There is some precedence to the use of is_empty: E.g. in Java, the Collection interface defines boolean isEmpty() with the usual meaning. C++'s STL defines empty() on all containers, Ocaml at least defines is_empty for stacks. Python's lists and Lua's tables are basically arrays, so they have no problem with using len(_).

In any event, we could add a lint that looks for traits which define .len() but do not define .is_empty() and ask the implementer to consider adding it. This would also remove the need for method lookup on that other lint – if most traits have .is_empty(), we won't need to care for the few remaining false positives, right?

A basic implementation would check_item for ItemTraits and see if the contained TraitItems fail to adhere to the social contract.

I wonder if we can catch cases like these where elision breaks things.

Basically, when the returned value has no relation to the inputs, lint that elision might not be wanted.

Exact heuristics would be tough to figure out, but I think it's doable (I've got a rough idea)

Ideally this should go into the compiler, but we can start it as a lint.

Here is the migration guide.

https://gist.github.com/kmcallister/c9ae9b977a7718376c7f

If you could create a branch for 1.0-alpha, I could try to make a pull request

Not a Rust-specific problem but I've seen it a lot in code from people who are new to programming in general.

Could generalize to find other forms that are better expressed by logical operators.

I just came across this warning:

src/helpers/adjust_header_level.rs:9:28: 9:71 warning: This expression mutably borrows a mutable reference. Consider reborrowing, #[warn(mut_mut)] on by default
src/helpers/adjust_header_level.rs:9     let headline_pattern = regex!(r"^(?P<level>[#]+)\s(?P<title>.+)$");
                                                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/main.rs:1:1: 24:1 note: in expansion of regex!

This does not seem very useful. (I totally understand why this can be problematic to fix in clippy, though.)

The Box<Vec<T>> warning appears to be being triggered when I box a closure (necessary in this case to make the struct Sized).

src/native_types.rs:93:13: 93:65 note: Vec<T> is already on the heap, Box<Vec<T>> makes an extra allocation
src/native_types.rs:93   pub run : Box<Fn(Vec<HValue>) -> Vec<HValue> + 'static + Send>
                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Currently, bad_bit_mask ignores masks that do not make the result of the whole expression constant, but don't add any value, e.g. x|0 or x & -1 or even x | 1 > 2 (because the former are always equal to x and latter is equal to x > 2 – they're not wrong per se, but may be misleading.

So let's have BitMask implement a second lint type (that is Warn instead of Deny), which I'd call ineffective_bit_mask.

Someone writing 3.1415 should probably use some definition of PI. We should look up other constants in the standard library.

Following a discussion on reddit, this is almost always a bad idea, so much that requiring to explicitly allow the few false positives (where you really mean it) is acceptable.

E.g. let x : u8 = ...; x as u32 > 300 maybe check sign of casted previously unsigned values.

E.g. ceil(1f32)

I've seen people do this regularly, I guess it's flagged from the must_use lint and people use this to silence the warnings.

let _ = stream.write(..);

Recommendation:

// ideally pass the error up
try!(stream.write(..))
// or else, panic
stream.write(..).unwrap()
// or add the allow(unused_must_use) attribute

Check for (doomed) comparisons to std::f32::NAN or std::f64::NAN.

In this situation you can always replace it with finer-grained Cells, you'd only turn it off if you have some specific reason not to do that. In my experience this is usually a bug.

For most types, this can be rewritten as container.is_empty().

cc rust-lang/rust#23682

As we all know, floating point operations carry the risk of rounding errors, thus comparing a float with == is asking for trouble. Suggest abs(x - y) < epsilon (for some value of epsilon) instead.

Raise your hand if you've ever needed to do this. The only tricky part is recommending an alternative since if you are even trying to do this something is probably badly wrong.

I don't know whether this is actually feasible to do a lint for, but just use a Box here (if the box is too small Rust will recommend a plain T).

Some people (myself included) think that name shadowing makes code harder to reason about.

Now of course there is a continuum of shadowing operations:

• shadow_same: The name is re-bound to the same data (e.g. let mut x = &mut x; for mutable re-bind)
• shadow_reuse_change: The name is re-used in the bound expression (e.g. let x = x + 1;)
• shadow_foreign: The name is re-bound without even using the original value

We could create lints for all three groups, plus a shadow_reuse lint group that combines shadow_same and shadow_reuse_change and a shadow lint group that combines all three.

Now for those lints, we want to make the macro check more strict in that we do not check within macro expansions at all.

I think (but am open to discussion) that the shadow_foreign lint should be Warn by default, while the others should be Allow. This won't disturb people too much but still makes for fairly readable code. Organizations or projects can then opt to Deny all shadowing, or whatever suits them.

&Vec<T> should usually be &[T] to save one pointer.

Since rust-lang/rust#19741 is merged, str.into_string() is no longer available. The replacement seems to be std::borrow::ToOwned::to_owned(). The lint should be updated to this.

Did you mean to use try! ? :)

Warn on instances of _ << _ + _ (or similar cases involving seldom-used operators). While those are not wrong, they can become footguns for anyone trying to reason about the code.

Suggest parenthesizing the operation.

Where:

• Small means "three words or fewer" (a la Vec)
• Box is not required for monomorphization
• T is not actually a generic (that is, your code doesn't have to work with potentially differently sized Ts).

Note that HashMap is five words by default, so it actually shouldn't qualify for this lint (IMO) but it would subsume the Vec lint.

I have seen this a couple of times, don't know how easy it is to detect (given the variations) but the person wants is x.iter().enumerate().

This might have to wait a while, and it might be really hard / impossible to detect, but for a simple loop like this:

let x = something_with_Slice<uint>_implemented;
for i in range(0, n) {
  /*every instance of x is called as x[i], where i is the same immutable uint (*not* MutSlice etc.) */
}

you should just use an iterator.

Things like

let mut x;

if p {
    x = 0u;
} else {
    x = 1u;
}

// no more mutation of x

which should be rewritten as

let x = if p { 0u } else { 1u };

Use .keys() and .values() instead of for (_, value) in hashmap.iter() or for (key, _) in hashmap.iter()

If we're just accessing fields and methods, pass by reference might make more sense. Maybe.

I just read https://internals.rust-lang.org/t/two-little-proposals/2148 and @Xirdus' first proposal sounds like a good addition to clippy:

Make it a warning (or even an error) to declare a variable of type (), except when it's due to generic type resolution. This is pretty useless, but some people new to Rust might do it by mistake, for example by saving the result of assignment. Also would make finding errors like unwanted semicolon easier in some cases.

Maybe you already know this, but it couldn't find an issue. (I'm compiling this project but it shouldn't matter.)

$ cargo run --features "dev" --release
   Compiling clippy v0.0.4 (https://github.com/Manishearth/rust-clippy.git#62f0fe0c)
/Users/pascal/.multirust/toolchains/nightly/cargo/git/checkouts/rust-clippy-2d80226e8d82d2cc/master/src/ptr_arg.rs:30:11: 30:39 error: this pattern has 5 fields, but the corresponding variant has 6 fields [E0023]
/Users/pascal/.multirust/toolchains/nightly/cargo/git/checkouts/rust-clippy-2d80226e8d82d2cc/master/src/ptr_arg.rs:30       if let &ItemFn(ref decl, _, _, _, _) = &item.node {
                                                                                                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
note: in expansion of if let expansion
/Users/pascal/.multirust/toolchains/nightly/cargo/git/checkouts/rust-clippy-2d80226e8d82d2cc/master/src/ptr_arg.rs:30:3: 32:4 note: expansion site
/Users/pascal/.multirust/toolchains/nightly/cargo/git/checkouts/rust-clippy-2d80226e8d82d2cc/master/src/ptr_arg.rs:30:11: 30:39 help: run `rustc --explain E0023` to see a detailed explanation
error: aborting due to previous error
Could not compile `clippy`.

To learn more, run the command again with --verbose.

$ rustc --version --verbose
rustc 1.2.0-nightly (7c4eedc21 2015-05-25) (built 2015-05-24)
binary: rustc
commit-hash: 7c4eedc21e609446671cc838d08473421917c353
commit-date: 2015-05-25
build-date: 2015-05-24
host: x86_64-apple-darwin
release: 1.2.0-nightly

This snippet is warned with warning: This if statement can be collapsed. Try: if a == 0 && b == 0, #[warn(collapsible_if)] on by default. Putting a semicolon after the second if block removes the warning, but doesn't seem like it should be necessary.

    let a = 0;
    let b = 0;
    if a == 0 {
        print!("a is zero");
        if b == 0 {
            println!("a and b is zero");
        }
    }

When i want to use this i get the following error:

clippy-0.0.4/src/bit_mask.rs:153:32: 153:80 error: this function takes 2 parameters but 3 parameters were supplied [E0061]
clippy-0.0.4/src/bit_mask.rs:153             .and_then(|def_id| lookup_const_by_id(cx.tcx, def_id, Option::None))

There are a few patterns we can search for (while true { ... } without break or return, method calling itself outside of if or closure, or within if true.

This is one of the cases where constant folding would enable us to implement more powerful checks.

(Analogous to the bad_bit_mask lint, we can detect min(x, max(y, _)) where x < y (this will always return x) or vice versa.

We should steal the string lint from hyperium/hyper#190 :)

to_string() is slower than into_string() since it creates a Formatter and all.