clippy 0.0.31

A bunch of helpful lints to avoid common pitfalls in Rust
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
337
338
339
340
341
342
343
344
345
346
347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380
381
382
383
384
385
386
387
388
389
390
391
392
393
394
395
396
397
398
399
400
401
402
403
404
405
406
407
408
409
410
411
412
413
414
415
416
417
418
419
420
421
422
423
424
425
426
427
428
429
430
431
432
433
434
435
436
437
438
439
440
441
442
443
444
445
446
447
448
449
450
451
452
453
454
455
456
457
458
459
460
461
462
463
464
465
466
use rustc_front::hir::*;
use rustc::lint::*;
use rustc::middle::ty;
use rustc::middle::subst::{Subst, TypeSpace};
use std::iter;
use std::borrow::Cow;

use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, walk_ptrs_ty_depth,
    walk_ptrs_ty};
use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH};

use self::SelfKind::*;
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");

/// **What it does:** This lint `Warn`s on using `ok().expect(..)`.
///
/// **Why is this bad?** Because you usually call `expect()` on the `Result` directly to get a good error message.
///
/// **Known problems:** None.
///
/// **Example:** `x.ok().expect("why did I do this again?")`
declare_lint!(pub OK_EXPECT, Warn,
              "using `ok().expect()`, which gives worse error messages than \
               calling `expect` directly on the Result");

/// **What it does:** This lint `Warn`s on `_.map(_).unwrap_or(_)`.
///
/// **Why is this bad?** Readability, this can be written more concisely as `_.map_or(_, _)`.
///
/// **Known problems:** None.
///
/// **Example:** `x.map(|a| a + 1).unwrap_or(0)`
declare_lint!(pub OPTION_MAP_UNWRAP_OR, Warn,
              "using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as \
               `map_or(a, f)`)");

/// **What it does:** This lint `Warn`s on `_.map(_).unwrap_or_else(_)`.
///
/// **Why is this bad?** Readability, this can be written more concisely as `_.map_or_else(_, _)`.
///
/// **Known problems:** None.
///
/// **Example:** `x.map(|a| a + 1).unwrap_or_else(some_function)`
declare_lint!(pub OPTION_MAP_UNWRAP_OR_ELSE, Warn,
              "using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as \
               `map_or_else(g, f)`)");

impl LintPass for MethodsPass {
    fn get_lints(&self) -> LintArray {
        lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING,
                    SHOULD_IMPLEMENT_TRAIT, WRONG_SELF_CONVENTION, OK_EXPECT, OPTION_MAP_UNWRAP_OR,
                    OPTION_MAP_UNWRAP_OR_ELSE)
    }
}

impl LateLintPass for MethodsPass {
    fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {

        if let ExprMethodCall(ref name, _, ref args) = expr.node {
            let (obj_ty, ptr_depth) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0]));
            match &*name.node.as_str() {
                "unwrap" if match_type(cx, obj_ty, &OPTION_PATH) => {
                    span_lint(cx, OPTION_UNWRAP_USED, expr.span,
                              "used unwrap() on an Option value. If you don't want \
                               to handle the None case gracefully, consider using \
                               expect() to provide a better panic message");
                },
                "unwrap" if match_type(cx, obj_ty, &RESULT_PATH) => {
                    span_lint(cx, RESULT_UNWRAP_USED, expr.span,
                              "used unwrap() on a Result value. Graceful handling \
                               of Err values is preferred");
                },
                "to_string" if obj_ty.sty == ty::TyStr => {
                    let mut arg_str = snippet(cx, args[0].span, "_");
                    if ptr_depth > 1 {
                        arg_str = Cow::Owned(format!(
                            "({}{})",
                            iter::repeat('*').take(ptr_depth - 1).collect::<String>(),
                            arg_str));
                    }
                    span_lint(cx, STR_TO_STRING, expr.span, &format!(
                        "`{}.to_owned()` is faster", arg_str));
                },
                "to_string" if match_type(cx, obj_ty, &STRING_PATH) => {
                    span_lint(cx, STRING_TO_STRING, expr.span, "`String.to_string()` is a no-op; use \
                                                                `clone()` to make a copy");
                },
                "expect" => if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node {
                    if inner_name.node.as_str() == "ok"
                            && match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &RESULT_PATH) {
                        let result_type = cx.tcx.expr_ty(&inner_args[0]);
                        if let Some(error_type) = get_error_type(cx, result_type) {
                            if has_debug_impl(error_type, cx) {
                                span_lint(cx, OK_EXPECT, expr.span,
                                         "called `ok().expect()` on a Result \
                                          value. You can call `expect` directly \
                                          on the `Result`");
                            }
                        }
                    }
                },
                // check Option.map(_).unwrap_or(_)
                "unwrap_or" => if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node {
                    if inner_name.node.as_str() == "map"
                            && match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &OPTION_PATH) {
                        // lint message
                        let msg =
                            "called `map(f).unwrap_or(a)` on an Option value. This can be done \
                             more directly by calling `map_or(a, f)` instead";
                        // get args to map() and unwrap_or()
                        let map_arg = snippet(cx, inner_args[1].span, "..");
                        let unwrap_arg = snippet(cx, args[1].span, "..");
                        // lint, with note if neither arg is > 1 line and both map() and
                        // unwrap_or() have the same span
                        let multiline = map_arg.lines().count() > 1
                                        || unwrap_arg.lines().count() > 1;
                        let same_span = inner_args[1].span.expn_id == args[1].span.expn_id;
                        if same_span && !multiline {
                            span_note_and_lint(
                                cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span,
                                &format!("replace this with map_or({1}, {0})",
                                         map_arg, unwrap_arg)
                            );
                        }
                        else if same_span && multiline {
                            span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg);
                        };
                    }
                },
                // check Option.map(_).unwrap_or_else(_)
                "unwrap_or_else" => if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node {
                    if inner_name.node.as_str() == "map"
                            && match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &OPTION_PATH) {
                        // lint message
                        let msg =
                            "called `map(f).unwrap_or_else(g)` on an Option value. This can be \
                             done more directly by calling `map_or_else(g, f)` instead";
                        // get args to map() and unwrap_or_else()
                        let map_arg = snippet(cx, inner_args[1].span, "..");
                        let unwrap_arg = snippet(cx, args[1].span, "..");
                        // lint, with note if neither arg is > 1 line and both map() and
                        // unwrap_or_else() have the same span
                        let multiline = map_arg.lines().count() > 1
                                        || unwrap_arg.lines().count() > 1;
                        let same_span = inner_args[1].span.expn_id == args[1].span.expn_id;
                        if same_span && !multiline {
                            span_note_and_lint(
                                cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg, expr.span,
                                &format!("replace this with map_or_else({1}, {0})",
                                         map_arg, unwrap_arg)
                            );
                        }
                        else if same_span && multiline {
                            span_lint(cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg);
                        };
                    }
                },
                _ => {},
            }
        }
    }

    fn check_item(&mut self, cx: &LateContext, item: &Item) {
        if let ItemImpl(_, _, _, None, ref ty, ref items) = item.node {
            for implitem in items {
                let name = implitem.name;
                if let ImplItemKind::Method(ref sig, _) = implitem.node {
                    // check missing trait implementations
                    for &(method_name, n_args, self_kind, out_type, trait_name) in &TRAIT_METHODS {
                        if_let_chain! {
                            [
                                name.as_str() == method_name,
                                sig.decl.inputs.len() == n_args,
                                out_type.matches(&sig.decl.output),
                                self_kind.matches(&sig.explicit_self.node, false)
                            ], {
                                span_lint(cx, SHOULD_IMPLEMENT_TRAIT, implitem.span, &format!(
                                    "defining a method called `{}` on this type; consider implementing \
                                     the `{}` trait or choosing a less ambiguous name", name, trait_name));
                            }
                        }
                    }
                    // check conventions w.r.t. conversion method names and predicates
                    let is_copy = is_copy(cx, &ty, &item);
                    for &(prefix, self_kinds) in &CONVENTIONS {
                        if name.as_str().starts_with(prefix) &&
                                !self_kinds.iter().any(|k| k.matches(&sig.explicit_self.node, is_copy)) {
                            let lint = if item.vis == Visibility::Public {
                                WRONG_PUB_SELF_CONVENTION
                            } else {
                                WRONG_SELF_CONVENTION
                            };
                            span_lint(cx, lint, sig.explicit_self.span, &format!(
                                "methods called `{}*` usually take {}; consider choosing a less \
                                 ambiguous name", prefix,
                                &self_kinds.iter().map(|k| k.description()).collect::<Vec<_>>().join(" or ")));
                        }
                    }
                }
            }
        }
    }
}

// Given a `Result<T, E>` type, return its error type (`E`)
fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option<ty::Ty<'a>> {
    if !match_type(cx, ty, &RESULT_PATH) {
        return None;
    }
    if let ty::TyEnum(_, substs) = ty.sty {
        if let Some(err_ty) = substs.types.opt_get(TypeSpace, 1) {
            return Some(err_ty);
        }
    }
    None
}

// This checks whether a given type is known to implement Debug. It's
// conservative, i.e. it should not return false positives, but will return
// false negatives.
fn has_debug_impl<'a, 'b>(ty: ty::Ty<'a>, cx: &LateContext<'b, 'a>) -> bool {
    let no_ref_ty = walk_ptrs_ty(ty);
    let debug = match cx.tcx.lang_items.debug_trait() {
        Some(debug) => debug,
        None => return false
    };
    let debug_def = cx.tcx.lookup_trait_def(debug);
    let mut debug_impl_exists = false;
    debug_def.for_each_relevant_impl(cx.tcx, no_ref_ty, |d| {
        let self_ty = &cx.tcx.impl_trait_ref(d).and_then(|im| im.substs.self_ty());
        if let Some(self_ty) = *self_ty {
            if !self_ty.flags.get().contains(ty::TypeFlags::HAS_PARAMS) {
                debug_impl_exists = true;
            }
        }
    });
    debug_impl_exists
}

const CONVENTIONS: [(&'static str, &'static [SelfKind]); 5] = [
    ("into_", &[ValueSelf]),
    ("to_",   &[RefSelf]),
    ("as_",   &[RefSelf, RefMutSelf]),
    ("is_",   &[RefSelf, NoSelf]),
    ("from_", &[NoSelf]),
];

const TRAIT_METHODS: [(&'static str, usize, SelfKind, OutType, &'static str); 30] = [
    ("add",        2, ValueSelf,  AnyType,  "std::ops::Add"),
    ("sub",        2, ValueSelf,  AnyType,  "std::ops::Sub"),
    ("mul",        2, ValueSelf,  AnyType,  "std::ops::Mul"),
    ("div",        2, ValueSelf,  AnyType,  "std::ops::Div"),
    ("rem",        2, ValueSelf,  AnyType,  "std::ops::Rem"),
    ("shl",        2, ValueSelf,  AnyType,  "std::ops::Shl"),
    ("shr",        2, ValueSelf,  AnyType,  "std::ops::Shr"),
    ("bitand",     2, ValueSelf,  AnyType,  "std::ops::BitAnd"),
    ("bitor",      2, ValueSelf,  AnyType,  "std::ops::BitOr"),
    ("bitxor",     2, ValueSelf,  AnyType,  "std::ops::BitXor"),
    ("neg",        1, ValueSelf,  AnyType,  "std::ops::Neg"),
    ("not",        1, ValueSelf,  AnyType,  "std::ops::Not"),
    ("drop",       1, RefMutSelf, UnitType, "std::ops::Drop"),
    ("index",      2, RefSelf,    RefType,  "std::ops::Index"),
    ("index_mut",  2, RefMutSelf, RefType,  "std::ops::IndexMut"),
    ("deref",      1, RefSelf,    RefType,  "std::ops::Deref"),
    ("deref_mut",  1, RefMutSelf, RefType,  "std::ops::DerefMut"),
    ("clone",      1, RefSelf,    AnyType,  "std::clone::Clone"),
    ("borrow",     1, RefSelf,    RefType,  "std::borrow::Borrow"),
    ("borrow_mut", 1, RefMutSelf, RefType,  "std::borrow::BorrowMut"),
    ("as_ref",     1, RefSelf,    RefType,  "std::convert::AsRef"),
    ("as_mut",     1, RefMutSelf, RefType,  "std::convert::AsMut"),
    ("eq",         2, RefSelf,    BoolType, "std::cmp::PartialEq"),
    ("cmp",        2, RefSelf,    AnyType,  "std::cmp::Ord"),
    ("default",    0, NoSelf,     AnyType,  "std::default::Default"),
    ("hash",       2, RefSelf,    UnitType, "std::hash::Hash"),
    ("next",       1, RefMutSelf, AnyType,  "std::iter::Iterator"),
    ("into_iter",  1, ValueSelf,  AnyType,  "std::iter::IntoIterator"),
    ("from_iter",  1, NoSelf,     AnyType,  "std::iter::FromIterator"),
    ("from_str",   1, NoSelf,     AnyType,  "std::str::FromStr"),
];

#[derive(Clone, Copy)]
enum SelfKind {
    ValueSelf,
    RefSelf,
    RefMutSelf,
    NoSelf,
}

impl SelfKind {
    fn matches(&self, slf: &ExplicitSelf_, allow_value_for_ref: bool) -> bool {
        match (self, slf) {
            (&ValueSelf, &SelfValue(_)) => true,
            (&RefSelf, &SelfRegion(_, Mutability::MutImmutable, _)) => true,
            (&RefMutSelf, &SelfRegion(_, Mutability::MutMutable, _)) => true,
            (&RefSelf, &SelfValue(_)) => allow_value_for_ref,
            (&RefMutSelf, &SelfValue(_)) => allow_value_for_ref,
            (&NoSelf, &SelfStatic) => true,
            (_, &SelfExplicit(ref ty, _)) => self.matches_explicit_type(ty, allow_value_for_ref),
            _ => false
        }
    }

    fn matches_explicit_type(&self, ty: &Ty, allow_value_for_ref: bool) -> bool {
        match (self, &ty.node) {
            (&ValueSelf, &TyPath(..)) => true,
            (&RefSelf, &TyRptr(_, MutTy { mutbl: Mutability::MutImmutable, .. })) => true,
            (&RefMutSelf, &TyRptr(_, MutTy { mutbl: Mutability::MutMutable, .. })) => true,
            (&RefSelf, &TyPath(..)) => allow_value_for_ref,
            (&RefMutSelf, &TyPath(..)) => allow_value_for_ref,
            _ => false
        }
    }

    fn description(&self) -> &'static str {
        match *self {
            ValueSelf => "self by value",
            RefSelf => "self by reference",
            RefMutSelf => "self by mutable reference",
            NoSelf => "no self",
        }
    }
}

#[derive(Clone, Copy)]
enum OutType {
    UnitType,
    BoolType,
    AnyType,
    RefType,
}

impl OutType {
    fn matches(&self, ty: &FunctionRetTy) -> bool {
        match (self, ty) {
            (&UnitType, &DefaultReturn(_)) => true,
            (&UnitType, &Return(ref ty)) if ty.node == TyTup(vec![]) => true,
            (&BoolType, &Return(ref ty)) if is_bool(ty) => true,
            (&AnyType, &Return(ref ty)) if ty.node != TyTup(vec![])  => true,
            (&RefType, &Return(ref ty)) => {
                if let TyRptr(_, _) = ty.node { true } else { false }
            }
            _ => false
        }
    }
}

fn is_bool(ty: &Ty) -> bool {
    if let TyPath(None, ref p) = ty.node {
        if match_path(p, &["bool"]) {
            return true;
        }
    }
    false
}

fn is_copy(cx: &LateContext, ast_ty: &Ty, item: &Item) -> bool {
    match cx.tcx.ast_ty_to_ty_cache.borrow().get(&ast_ty.id) {
        None => false,
        Some(ty) => {
            let env = ty::ParameterEnvironment::for_item(cx.tcx, item.id);
            !ty.subst(cx.tcx, &env.free_substs).moves_by_default(&env, ast_ty.span)
        }
    }
}