Implement a lint mode to deal with unused 'mut' variables

7 files changed, 164 insertions, 21 deletions

diff --git a/src/librustc/middle/borrowck/check_loans.rs b/src/librustc/middle/borrowck/check_loans.rs
index 4766fe1fb94..6f3075717ed 100644
--- a/src/librustc/middle/borrowck/check_loans.rs
+++ b/src/librustc/middle/borrowck/check_loans.rs
@@ -367,7 +367,18 @@ pub impl CheckLoanCtxt {
             // are only assigned once
         } else {
             match cmt.mutbl {
-                McDeclared | McInherited => { /*ok*/ }
+                McDeclared | McInherited => {
+                    // Ok, but if this loan is a mutable loan, then mark the
+                    // loan path (if it exists) as being used. This is similar
+                    // to the check performed in loan.rs in issue_loan(). This
+                    // type of use of mutable is different from issuing a loan,
+                    // however.
+                    for cmt.lp.each |lp| {
+                        for lp.node_id().each |&id| {
+                            self.tcx().used_mut_nodes.insert(id);
+                        }
+                    }
+                }
                 McReadOnly | McImmutable => {
                     self.bccx.span_err(
                         ex.span,
diff --git a/src/librustc/middle/borrowck/loan.rs b/src/librustc/middle/borrowck/loan.rs
index 15189a552fb..3c37b5dae64 100644
--- a/src/librustc/middle/borrowck/loan.rs
+++ b/src/librustc/middle/borrowck/loan.rs
@@ -51,7 +51,7 @@ use middle::mem_categorization::{cat_arg, cat_binding, cat_discr, cat_comp};
 use middle::mem_categorization::{cat_deref, cat_discr, cat_local, cat_self};
 use middle::mem_categorization::{cat_special, cat_stack_upvar, cmt};
 use middle::mem_categorization::{comp_field, comp_index, comp_variant};
-use middle::mem_categorization::{gc_ptr, region_ptr};
+use middle::mem_categorization::{gc_ptr, region_ptr, lp_local, lp_arg};
 use middle::ty;
 use util::common::indenter;
 
@@ -274,7 +274,17 @@ pub impl LoanContext {
         if !owns_lent_data ||
             self.bccx.is_subregion_of(self.scope_region, scope_ub)
         {
-            if loan_kind.is_take() && !cmt.mutbl.is_mutable() {
+            if cmt.mutbl.is_mutable() {
+                // If this loan is a mutable loan, then mark the loan path (if
+                // it exists) as being used. This is similar to the check
+                // performed in check_loans.rs in check_assignment(), but this
+                // is for a different purpose of having the 'mut' qualifier.
+                for cmt.lp.each |lp| {
+                    for lp.node_id().each |&id| {
+                        self.tcx().used_mut_nodes.insert(id);
+                    }
+                }
+            } else if loan_kind.is_take() {
                 // We do not allow non-mutable data to be "taken"
                 // under any circumstances.
                 return Err(bckerr {
diff --git a/src/librustc/middle/lint.rs b/src/librustc/middle/lint.rs
index 9087afe5969..bda97f4f530 100644
--- a/src/librustc/middle/lint.rs
+++ b/src/librustc/middle/lint.rs
@@ -13,6 +13,7 @@ use core::prelude::*;
 use driver::session::Session;
 use driver::session;
 use middle::ty;
+use middle::pat_util;
 use util::ppaux::{ty_to_str};
 
 use core::hashmap::HashMap;
@@ -86,6 +87,7 @@ pub enum lint {
 
     unused_variable,
     dead_assignment,
+    unused_mut,
 }
 
 pub fn level_to_str(lv: level) -> &'static str {
@@ -277,6 +279,13 @@ pub fn get_lint_dict() -> LintDict {
             desc: "detect assignments that will never be read",
             default: warn
         }),
+
+        (~"unused_mut",
+         LintSpec {
+            lint: unused_mut,
+            desc: "detect mut variables which don't need to be mutable",
+            default: warn
+        }),
     ];
     let mut map = HashMap::new();
     do vec::consume(v) |_, (k, v)| {
@@ -499,6 +508,7 @@ fn check_item(i: @ast::item, cx: ty::ctxt) {
     check_item_deprecated_mutable_fields(cx, i);
     check_item_deprecated_drop(cx, i);
     check_item_unused_unsafe(cx, i);
+    check_item_unused_mut(cx, i);
 }
 
 // Take a visitor, and modify it so that it will not proceed past subitems.
@@ -954,6 +964,53 @@ fn check_item_unused_unsafe(cx: ty::ctxt, it: @ast::item) {
     visit::visit_item(it, (), visit);
 }
 
+fn check_item_unused_mut(tcx: ty::ctxt, it: @ast::item) {
+    let check_pat: @fn(@ast::pat) = |p| {
+        let mut used = false;
+        let mut bindings = 0;
+        do pat_util::pat_bindings(tcx.def_map, p) |_, id, _, _| {
+            used = used || tcx.used_mut_nodes.contains(&id);
+            bindings += 1;
+        }
+        if !used {
+            let msg = if bindings == 1 {
+                ~"variable does not need to be mutable"
+            } else {
+                ~"variables do not need to be mutable"
+            };
+            tcx.sess.span_lint(unused_mut, p.id, it.id, p.span, msg);
+        }
+    };
+
+    let visit_fn_decl: @fn(&ast::fn_decl) = |fd| {
+        for fd.inputs.each |arg| {
+            if arg.is_mutbl {
+                check_pat(arg.pat);
+            }
+        }
+    };
+
+    let visit = item_stopping_visitor(
+        visit::mk_simple_visitor(@visit::SimpleVisitor {
+            visit_local: |l| {
+                if l.node.is_mutbl {
+                    check_pat(l.node.pat);
+                }
+            },
+            visit_fn: |_, fd, _, _, _| visit_fn_decl(fd),
+            visit_ty_method: |tm| visit_fn_decl(&tm.decl),
+            visit_struct_method: |sm| visit_fn_decl(&sm.decl),
+            visit_trait_method: |tm| {
+                match *tm {
+                    ast::required(ref tm) => visit_fn_decl(&tm.decl),
+                    ast::provided(m) => visit_fn_decl(&m.decl),
+                }
+            },
+            .. *visit::default_simple_visitor()
+        }));
+    visit::visit_item(it, (), visit);
+}
+
 fn check_fn(tcx: ty::ctxt, fk: &visit::fn_kind, decl: &ast::fn_decl,
             _body: &ast::blk, span: span, id: ast::node_id) {
     debug!("lint check_fn fk=%? id=%?", fk, id);
diff --git a/src/librustc/middle/liveness.rs b/src/librustc/middle/liveness.rs
index 3136d7bf4e4..2b36ce4ce03 100644
--- a/src/librustc/middle/liveness.rs
+++ b/src/librustc/middle/liveness.rs
@@ -1516,9 +1516,8 @@ fn check_local(local: @local, self: @Liveness, vt: vt<@Liveness>) {
 
         // Initializer:
         self.warn_about_unused_or_dead_vars_in_pat(local.node.pat);
-        if !local.node.is_mutbl {
-            self.check_for_reassignments_in_pat(local.node.pat);
-        }
+        self.check_for_reassignments_in_pat(local.node.pat,
+                                            local.node.is_mutbl);
       }
       None => {
 
@@ -1702,12 +1701,15 @@ pub impl Liveness {
         match expr.node {
           expr_path(_) => {
             match *self.tcx.def_map.get(&expr.id) {
-              def_local(nid, false) => {
-                // Assignment to an immutable variable or argument:
-                // only legal if there is no later assignment.
+              def_local(nid, mutbl) => {
+                // Assignment to an immutable variable or argument: only legal
+                // if there is no later assignment. If this local is actually
+                // mutable, then check for a reassignment to flag the mutability
+                // as being used.
                 let ln = self.live_node(expr.id, expr.span);
                 let var = self.variable(nid, expr.span);
-                self.check_for_reassignment(ln, var, expr.span);
+                self.check_for_reassignment(ln, var, expr.span,
+                                            if mutbl {Some(nid)} else {None});
                 self.warn_about_dead_assign(expr.span, expr.id, ln, var);
               }
               def => {
@@ -1731,23 +1733,28 @@ pub impl Liveness {
        }
     }
 
-    fn check_for_reassignments_in_pat(@self, pat: @pat) {
-        do self.pat_bindings(pat) |ln, var, sp, _id| {
-            self.check_for_reassignment(ln, var, sp);
+    fn check_for_reassignments_in_pat(@self, pat: @pat, mutbl: bool) {
+        do self.pat_bindings(pat) |ln, var, sp, id| {
+            self.check_for_reassignment(ln, var, sp,
+                                        if mutbl {Some(id)} else {None});
         }
     }
 
     fn check_for_reassignment(@self, ln: LiveNode, var: Variable,
-                              orig_span: span) {
+                              orig_span: span, mutbl: Option<node_id>) {
         match self.assigned_on_exit(ln, var) {
           Some(ExprNode(span)) => {
-            self.tcx.sess.span_err(
-                span,
-                ~"re-assignment of immutable variable");
-
-            self.tcx.sess.span_note(
-                orig_span,
-                ~"prior assignment occurs here");
+            match mutbl {
+              Some(id) => { self.tcx.used_mut_nodes.insert(id); }
+              None => {
+                self.tcx.sess.span_err(
+                    span,
+                    ~"re-assignment of immutable variable");
+                self.tcx.sess.span_note(
+                    orig_span,
+                    ~"prior assignment occurs here");
+              }
+            }
           }
           Some(lnk) => {
             self.tcx.sess.span_bug(
diff --git a/src/librustc/middle/mem_categorization.rs b/src/librustc/middle/mem_categorization.rs
index 6473cb8e8e0..51e6860432a 100644
--- a/src/librustc/middle/mem_categorization.rs
+++ b/src/librustc/middle/mem_categorization.rs
@@ -351,6 +351,16 @@ pub impl MutabilityCategory {
     }
 }
 
+pub impl loan_path {
+    fn node_id(&self) -> Option<ast::node_id> {
+        match *self {
+            lp_local(id) | lp_arg(id) => Some(id),
+            lp_deref(lp, _) | lp_comp(lp, _) => lp.node_id(),
+            lp_self => None
+        }
+    }
+}
+
 pub impl mem_categorization_ctxt {
     fn cat_expr(&self, expr: @ast::expr) -> cmt {
         match self.tcx.adjustments.find(&expr.id) {
diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs
index 95d0fa984ae..be362904dc4 100644
--- a/src/librustc/middle/ty.rs
+++ b/src/librustc/middle/ty.rs
@@ -304,6 +304,11 @@ struct ctxt_ {
     // Set of used unsafe nodes (functions or blocks). Unsafe nodes not
     // present in this set can be warned about.
     used_unsafe: @mut HashSet<ast::node_id>,
+
+    // Set of nodes which mark locals as mutable which end up getting used at
+    // some point. Local variable definitions not in this set can be warned
+    // about.
+    used_mut_nodes: @mut HashSet<ast::node_id>,
 }
 
 pub enum tbox_flag {
@@ -933,6 +938,7 @@ pub fn mk_ctxt(s: session::Session,
         destructors: @mut HashSet::new(),
         trait_impls: @mut HashMap::new(),
         used_unsafe: @mut HashSet::new(),
+        used_mut_nodes: @mut HashSet::new(),
      }
 }
 
diff --git a/src/test/compile-fail/unused-mut-variables.rs b/src/test/compile-fail/unused-mut-variables.rs
new file mode 100644
index 00000000000..d1223cd8893
--- /dev/null
+++ b/src/test/compile-fail/unused-mut-variables.rs
@@ -0,0 +1,42 @@
+// Copyright 2013 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Exercise the unused_mut attribute in some positive and negative cases
+
+#[allow(dead_assignment)];
+#[allow(unused_variable)];
+#[deny(unused_mut)];
+
+fn main() {
+    // negative cases
+    let mut a = 3; //~ ERROR: variable does not need to be mutable
+    let mut a = 2, b = 3; //~ ERROR: variable does not need to be mutable
+                          //~^ ERROR: variable does not need to be mutable
+    let mut a = ~[3]; //~ ERROR: variable does not need to be mutable
+
+    // positive cases
+    let mut a = 2;
+    a = 3;
+    let mut a = ~[];
+    a.push(3);
+    let mut a = ~[];
+    do callback {
+        a.push(3);
+    }
+}
+
+fn callback(f: &fn()) {}
+
+// make sure the lint attribute can be turned off
+#[allow(unused_mut)]
+fn foo(mut a: int) {
+    let mut a = 3;
+    let mut b = ~[2];
+}