Merge remote-tracking branch 'origin/master' into clonedCollect

CHANGELOG.md

@@ -1,6 +1,8 @@
 # Change Log
 All notable changes to this project will be documented in this file.
 
+* New [`mut_from_ref`] lint
+
 ## 0.0.114 — 2017-02-08
 * Rustup to rustc 1.17.0-nightly (c49d10207 2017-02-07)
 * Tests are now ui tests (testing the exact output of rustc)
@@ -362,6 +364,7 @@ All notable changes to this project will be documented in this file.
 [`match_overlapping_arm`]: https://github.com/Manishearth/rust-clippy/wiki#match_overlapping_arm
 [`match_ref_pats`]: https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats
 [`match_same_arms`]: https://github.com/Manishearth/rust-clippy/wiki#match_same_arms
+[`match_wild_err_arm`]: https://github.com/Manishearth/rust-clippy/wiki#match_wild_err_arm
 [`mem_forget`]: https://github.com/Manishearth/rust-clippy/wiki#mem_forget
 [`min_max`]: https://github.com/Manishearth/rust-clippy/wiki#min_max
 [`misrefactored_assign_op`]: https://github.com/Manishearth/rust-clippy/wiki#misrefactored_assign_op
@@ -369,6 +372,7 @@ All notable changes to this project will be documented in this file.
 [`mixed_case_hex_literals`]: https://github.com/Manishearth/rust-clippy/wiki#mixed_case_hex_literals
 [`module_inception`]: https://github.com/Manishearth/rust-clippy/wiki#module_inception
 [`modulo_one`]: https://github.com/Manishearth/rust-clippy/wiki#modulo_one
+[`mut_from_ref`]: https://github.com/Manishearth/rust-clippy/wiki#mut_from_ref
 [`mut_mut`]: https://github.com/Manishearth/rust-clippy/wiki#mut_mut
 [`mutex_atomic`]: https://github.com/Manishearth/rust-clippy/wiki#mutex_atomic
 [`mutex_integer`]: https://github.com/Manishearth/rust-clippy/wiki#mutex_integer

README.md

@@ -180,7 +180,7 @@ transparently:
 
 ## Lints
 
-There are 186 lints included in this crate:
+There are 188 lints included in this crate:
 
 name                                                                                                                   | default | triggers on
 -----------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -271,6 +271,7 @@ name
 [match_overlapping_arm](https://github.com/Manishearth/rust-clippy/wiki#match_overlapping_arm)                         | warn    | a match with overlapping arms
 [match_ref_pats](https://github.com/Manishearth/rust-clippy/wiki#match_ref_pats)                                       | warn    | a match or `if let` with all arms prefixed with `&` instead of deref-ing the match expression
 [match_same_arms](https://github.com/Manishearth/rust-clippy/wiki#match_same_arms)                                     | warn    | `match` with identical arm bodies
+[match_wild_err_arm](https://github.com/Manishearth/rust-clippy/wiki#match_wild_err_arm)                               | warn    | a match with `Err(_)` arm and take drastic actions
 [mem_forget](https://github.com/Manishearth/rust-clippy/wiki#mem_forget)                                               | allow   | `mem::forget` usage on `Drop` types, likely to cause memory leaks
 [min_max](https://github.com/Manishearth/rust-clippy/wiki#min_max)                                                     | warn    | `min(_, max(_, _))` (or vice versa) with bounds clamping the result to a constant
 [misrefactored_assign_op](https://github.com/Manishearth/rust-clippy/wiki#misrefactored_assign_op)                     | warn    | having a variable on both sides of an assign op
@@ -278,6 +279,7 @@ name
 [mixed_case_hex_literals](https://github.com/Manishearth/rust-clippy/wiki#mixed_case_hex_literals)                     | warn    | hex literals whose letter digits are not consistently upper- or lowercased
 [module_inception](https://github.com/Manishearth/rust-clippy/wiki#module_inception)                                   | warn    | modules that have the same name as their parent module
 [modulo_one](https://github.com/Manishearth/rust-clippy/wiki#modulo_one)                                               | warn    | taking a number modulo 1, which always returns 0
+[mut_from_ref](https://github.com/Manishearth/rust-clippy/wiki#mut_from_ref)                                           | warn    | fns that create mutable refs from immutable ref args
 [mut_mut](https://github.com/Manishearth/rust-clippy/wiki#mut_mut)                                                     | allow   | usage of double-mut refs, e.g. `&mut &mut ...`
 [mutex_atomic](https://github.com/Manishearth/rust-clippy/wiki#mutex_atomic)                                           | warn    | using a mutex where an atomic value could be used instead
 [mutex_integer](https://github.com/Manishearth/rust-clippy/wiki#mutex_integer)                                         | allow   | using a mutex for an integer type

clippy_lints/src/lib.rs

@@ -15,6 +15,7 @@
 #![allow(needless_lifetimes)]
 
 extern crate syntax;
+extern crate syntax_pos;
 #[macro_use]
 extern crate rustc;
 extern crate rustc_data_structures;
@@ -412,6 +413,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         matches::MATCH_BOOL,
         matches::MATCH_OVERLAPPING_ARM,
         matches::MATCH_REF_PATS,
+        matches::MATCH_WILD_ERR_ARM,
         matches::SINGLE_MATCH,
         methods::CHARS_NEXT_CMP,
         methods::CLONE_DOUBLE_REF,
@@ -464,6 +466,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         precedence::PRECEDENCE,
         print::PRINT_WITH_NEWLINE,
         ptr::CMP_NULL,
+        ptr::MUT_FROM_REF,
         ptr::PTR_ARG,
         ranges::RANGE_STEP_BY_ZERO,
         ranges::RANGE_ZIP_WITH_LEN,

clippy_lints/src/matches.rs

@@ -10,7 +10,8 @@ use std::collections::Bound;
 use syntax::ast::LitKind;
 use syntax::codemap::Span;
 use utils::paths;
-use utils::{match_type, snippet, span_note_and_lint, span_lint_and_then, in_external_macro, expr_block};
+use utils::{match_type, snippet, span_note_and_lint, span_lint_and_then, in_external_macro, expr_block, walk_ptrs_ty,
+            is_expn_of};
 use utils::sugg::Sugg;
 
 /// **What it does:** Checks for matches with a single arm where an `if let`
@@ -121,6 +122,28 @@ declare_lint! {
     "a match with overlapping arms"
 }
 
+/// **What it does:** Checks for arm which matches all errors with `Err(_)`
+/// and take drastic actions like `panic!`.
+///
+/// **Why is this bad?** It is generally a bad practice, just like
+/// catching all exceptions in java with `catch(Exception)`
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// let x : Result(i32, &str) = Ok(3);
+/// match x {
+///     Ok(_) => println!("ok"),
+///     Err(_) => panic!("err"),
+/// }
+/// ```
+declare_lint! {
+    pub MATCH_WILD_ERR_ARM,
+    Warn,
+    "a match with `Err(_)` arm and take drastic actions"
+}
+
 #[allow(missing_copy_implementations)]
 pub struct MatchPass;
 
@@ -130,7 +153,8 @@ impl LintPass for MatchPass {
                     MATCH_REF_PATS,
                     MATCH_BOOL,
                     SINGLE_MATCH_ELSE,
-                    MATCH_OVERLAPPING_ARM)
+                    MATCH_OVERLAPPING_ARM,
+                    MATCH_WILD_ERR_ARM)
     }
 }
 
@@ -143,6 +167,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MatchPass {
             check_single_match(cx, ex, arms, expr);
             check_match_bool(cx, ex, arms, expr);
             check_overlapping_arms(cx, ex, arms);
+            check_wild_err_arm(cx, ex, arms);
         }
         if let ExprMatch(ref ex, ref arms, source) = expr.node {
             check_match_ref_pats(cx, ex, arms, source, expr);
@@ -322,6 +347,45 @@ fn check_overlapping_arms(cx: &LateContext, ex: &Expr, arms: &[Arm]) {
     }
 }
 
+fn check_wild_err_arm(cx: &LateContext, ex: &Expr, arms: &[Arm]) {
+    let ex_ty = walk_ptrs_ty(cx.tables.expr_ty(ex));
+    if match_type(cx, ex_ty, &paths::RESULT) {
+        for arm in arms {
+            if let PatKind::TupleStruct(ref path, ref inner, _) = arm.pats[0].node {
+                let path_str = print::to_string(print::NO_ANN, |s| s.print_qpath(path, false));
+                if_let_chain! {[
+                    path_str == "Err",
+                    inner.iter().any(|pat| pat.node == PatKind::Wild),
+                    let ExprBlock(ref block) = arm.body.node,
+                    is_panic_block(cx, block)
+                ], {
+                    // `Err(_)` arm with `panic!` found
+                    span_note_and_lint(cx,
+                                       MATCH_WILD_ERR_ARM,
+                                       arm.pats[0].span,
+                                       "Err(_) will match all errors, maybe not a good idea",
+                                       arm.pats[0].span,
+                                       "to remove this warning, match each error seperately \
+                                        or use unreachable macro");
+                }}
+            }
+        }
+    }
+}
+
+// If the block contains only a `panic!` macro (as expression or statement)
+fn is_panic_block(cx: &LateContext, block: &Block) -> bool {
+    match (&block.expr, block.stmts.len(), block.stmts.first()) {
+        (&Some(ref exp), 0, _) => {
+            is_expn_of(cx, exp.span, "panic").is_some() && is_expn_of(cx, exp.span, "unreachable").is_none()
+        },
+        (&None, 1, Some(stmt)) => {
+            is_expn_of(cx, stmt.span, "panic").is_some() && is_expn_of(cx, stmt.span, "unreachable").is_none()
+        },
+        _ => false,
+    }
+}
+
 fn check_match_ref_pats(cx: &LateContext, ex: &Expr, arms: &[Arm], source: MatchSource, expr: &Expr) {
     if has_only_ref_pats(arms) {
         if let ExprAddrOf(Mutability::MutImmutable, ref inner) = ex.node {

clippy_lints/src/ptr.rs

@@ -5,7 +5,9 @@ use rustc::hir::map::NodeItem;
 use rustc::lint::*;
 use rustc::ty;
 use syntax::ast::NodeId;
-use utils::{match_path, match_type, paths, span_lint};
+use syntax::codemap::Span;
+use syntax_pos::MultiSpan;
+use utils::{match_path, match_type, paths, span_lint, span_lint_and_then};
 
 /// **What it does:** This lint checks for function arguments of type `&String` or `&Vec` unless
 /// the references are mutable.
@@ -44,13 +46,32 @@ declare_lint! {
     "comparing a pointer to a null pointer, suggesting to use `.is_null()` instead."
 }
 
+/// **What it does:** This lint checks for functions that take immutable references and return
+/// mutable ones.
+///
+/// **Why is this bad?** This is trivially unsound, as one can create two mutable references
+/// from the same (immutable!) source. This [error](https://github.com/rust-lang/rust/issues/39465)
+/// actually lead to an interim Rust release 1.15.1.
+///
+/// **Known problems:** To be on the conservative side, if there's at least one mutable reference
+/// with the output lifetime, this lint will not trigger. In practice, this case is unlikely anyway.
+///
+/// **Example:**
+/// ```rust
+/// fn foo(&Foo) -> &mut Bar { .. }
+/// ```
+declare_lint! {
+    pub MUT_FROM_REF,
+    Warn,
+    "fns that create mutable refs from immutable ref args"
+}
 
 #[derive(Copy,Clone)]
 pub struct PointerPass;
 
 impl LintPass for PointerPass {
     fn get_lints(&self) -> LintArray {
-        lint_array!(PTR_ARG, CMP_NULL)
+        lint_array!(PTR_ARG, CMP_NULL, MUT_FROM_REF)
     }
 }
 
@@ -111,6 +132,37 @@ fn check_fn(cx: &LateContext, decl: &FnDecl, fn_id: NodeId) {
             }
         }
     }
+
+    if let FunctionRetTy::Return(ref ty) = decl.output {
+        if let Some((out, MutMutable, _)) = get_rptr_lm(ty) {
+            let mut immutables = vec![];
+            for (_, ref mutbl, ref argspan) in
+                decl.inputs
+                    .iter()
+                    .filter_map(|ty| get_rptr_lm(ty))
+                    .filter(|&(lt, _, _)| lt.name == out.name) {
+                if *mutbl == MutMutable {
+                    return;
+                }
+                immutables.push(*argspan);
+            }
+            if immutables.is_empty() {
+                return;
+            }
+            span_lint_and_then(cx, MUT_FROM_REF, ty.span, "mutable borrow from immutable input(s)", |db| {
+                let ms = MultiSpan::from_spans(immutables);
+                db.span_note(ms, "immutable borrow here");
+            });
+        }
+    }
+}
+
+fn get_rptr_lm(ty: &Ty) -> Option<(&Lifetime, Mutability, Span)> {
+    if let Ty_::TyRptr(ref lt, ref m) = ty.node {
+        Some((lt, m.mutbl, ty.span))
+    } else {
+        None
+    }
 }
 
 fn is_null_path(expr: &Expr) -> bool {

tests/ui/matches.rs

@@ -283,5 +283,53 @@ fn overlapping() {
     }
 }
 
+fn match_wild_err_arm() {
+    let x: Result<i32, &str> = Ok(3);
+
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_) => panic!("err")
+    }
+
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_) => {panic!()}
+    }
+
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_) => {panic!();}
+    }
+
+    // allowed when not with `panic!` block
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_) => println!("err")
+    }
+
+    // allowed when used with `unreachable!`
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_) => {unreachable!()}
+    }
+
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_) => unreachable!()
+    }
+
+    match x {
+        Ok(3) => println!("ok"),
+        Ok(_) => println!("ok"),
+        Err(_) => {unreachable!();}
+    }
+}
+
 fn main() {
 }

tests/ui/matches.stderr

@@ -388,5 +388,37 @@ note: overlaps with this
 275 |         0 ... 11 => println!("0 ... 11"),
     |         ^^^^^^^^
 
-error: aborting due to 23 previous errors
+error: Err(_) will match all errors, maybe not a good idea
+   --> $DIR/matches.rs:292:9
+    |
+292 |         Err(_) => panic!("err")
+    |         ^^^^^^
+    |
+    = note: #[deny(match_wild_err_arm)] implied by #[deny(clippy)]
+note: lint level defined here
+   --> $DIR/matches.rs:5:9
+    |
+5   | #![deny(clippy)]
+    |         ^^^^^^
+    = note: to remove this warning, match each error seperately or use unreachable macro
+
+error: Err(_) will match all errors, maybe not a good idea
+   --> $DIR/matches.rs:298:9
+    |
+298 |         Err(_) => {panic!()}
+    |         ^^^^^^
+    |
+    = note: #[deny(match_wild_err_arm)] implied by #[deny(clippy)]
+    = note: to remove this warning, match each error seperately or use unreachable macro
+
+error: Err(_) will match all errors, maybe not a good idea
+   --> $DIR/matches.rs:304:9
+    |
+304 |         Err(_) => {panic!();}
+    |         ^^^^^^
+    |
+    = note: #[deny(match_wild_err_arm)] implied by #[deny(clippy)]
+    = note: to remove this warning, match each error seperately or use unreachable macro
+
+error: aborting due to 26 previous errors
 

tests/ui/mut_from_ref.rs

@@ -0,0 +1,48 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+#![allow(unused)]
+#![deny(mut_from_ref)]
+
+struct Foo;
+
+impl Foo {
+    fn this_wont_hurt_a_bit(&self) -> &mut Foo {
+        unimplemented!()
+    }
+}
+
+trait Ouch {
+    fn ouch(x: &Foo) -> &mut Foo;
+}
+
+impl Ouch for Foo {
+    fn ouch(x: &Foo) -> &mut Foo {
+        unimplemented!()
+    }
+}
+
+fn fail(x: &u32) -> &mut u16 {
+    unimplemented!()
+}
+
+fn fail_lifetime<'a>(x: &'a u32, y: &mut u32) -> &'a mut u32 {
+    unimplemented!()
+}
+
+fn fail_double<'a, 'b>(x: &'a u32, y: &'a u32, z: &'b mut u32) -> &'a mut u32 {
+    unimplemented!()
+}
+
+// this is OK, because the result borrows y
+fn works<'a>(x: &u32, y: &'a mut u32) -> &'a mut u32 {
+    unimplemented!()
+}
+
+// this is also OK, because the result could borrow y
+fn also_works<'a>(x: &'a u32, y: &'a mut u32) -> &'a mut u32 {
+    unimplemented!()
+}
+
+fn main() {
+    //TODO
+}

tests/ui/mut_from_ref.stderr

@@ -0,0 +1,67 @@
+error: mutable borrow from immutable input(s)
+ --> $DIR/mut_from_ref.rs:9:39
+  |
+9 |     fn this_wont_hurt_a_bit(&self) -> &mut Foo {
+  |                                       ^^^^^^^^
+  |
+note: lint level defined here
+ --> $DIR/mut_from_ref.rs:4:9
+  |
+4 | #![deny(mut_from_ref)]
+  |         ^^^^^^^^^^^^
+note: immutable borrow here
+ --> $DIR/mut_from_ref.rs:9:29
+  |
+9 |     fn this_wont_hurt_a_bit(&self) -> &mut Foo {
+  |                             ^^^^^
+
+error: mutable borrow from immutable input(s)
+  --> $DIR/mut_from_ref.rs:15:25
+   |
+15 |     fn ouch(x: &Foo) -> &mut Foo;
+   |                         ^^^^^^^^
+   |
+note: immutable borrow here
+  --> $DIR/mut_from_ref.rs:15:16
+   |
+15 |     fn ouch(x: &Foo) -> &mut Foo;
+   |                ^^^^
+
+error: mutable borrow from immutable input(s)
+  --> $DIR/mut_from_ref.rs:24:21
+   |
+24 | fn fail(x: &u32) -> &mut u16 {
+   |                     ^^^^^^^^
+   |
+note: immutable borrow here
+  --> $DIR/mut_from_ref.rs:24:12
+   |
+24 | fn fail(x: &u32) -> &mut u16 {
+   |            ^^^^
+
+error: mutable borrow from immutable input(s)
+  --> $DIR/mut_from_ref.rs:28:50
+   |
+28 | fn fail_lifetime<'a>(x: &'a u32, y: &mut u32) -> &'a mut u32 {
+   |                                                  ^^^^^^^^^^^
+   |
+note: immutable borrow here
+  --> $DIR/mut_from_ref.rs:28:25
+   |
+28 | fn fail_lifetime<'a>(x: &'a u32, y: &mut u32) -> &'a mut u32 {
+   |                         ^^^^^^^
+
+error: mutable borrow from immutable input(s)
+  --> $DIR/mut_from_ref.rs:32:67
+   |
+32 | fn fail_double<'a, 'b>(x: &'a u32, y: &'a u32, z: &'b mut u32) -> &'a mut u32 {
+   |                                                                   ^^^^^^^^^^^
+   |
+note: immutable borrow here
+  --> $DIR/mut_from_ref.rs:32:27
+   |
+32 | fn fail_double<'a, 'b>(x: &'a u32, y: &'a u32, z: &'b mut u32) -> &'a mut u32 {
+   |                           ^^^^^^^     ^^^^^^^
+
+error: aborting due to 5 previous errors
+