Skip to content

Commit d1985c9

Browse files
committed
Implement a lint mode to deal with unused 'mut' variables
1 parent 0e017ab commit d1985c9

File tree

7 files changed

+164
-21
lines changed

7 files changed

+164
-21
lines changed

src/librustc/middle/borrowck/check_loans.rs

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -367,7 +367,18 @@ pub impl CheckLoanCtxt {
367367
// are only assigned once
368368
} else {
369369
match cmt.mutbl {
370-
McDeclared | McInherited => { /*ok*/ }
370+
McDeclared | McInherited => {
371+
// Ok, but if this loan is a mutable loan, then mark the
372+
// loan path (if it exists) as being used. This is similar
373+
// to the check performed in loan.rs in issue_loan(). This
374+
// type of use of mutable is different from issuing a loan,
375+
// however.
376+
for cmt.lp.each |lp| {
377+
for lp.node_id().each |&id| {
378+
self.tcx().used_mut_nodes.insert(id);
379+
}
380+
}
381+
}
371382
McReadOnly | McImmutable => {
372383
self.bccx.span_err(
373384
ex.span,

src/librustc/middle/borrowck/loan.rs

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ use middle::mem_categorization::{cat_arg, cat_binding, cat_discr, cat_comp};
5151
use middle::mem_categorization::{cat_deref, cat_discr, cat_local, cat_self};
5252
use middle::mem_categorization::{cat_special, cat_stack_upvar, cmt};
5353
use middle::mem_categorization::{comp_field, comp_index, comp_variant};
54-
use middle::mem_categorization::{gc_ptr, region_ptr};
54+
use middle::mem_categorization::{gc_ptr, region_ptr, lp_local, lp_arg};
5555
use middle::ty;
5656
use util::common::indenter;
5757

@@ -274,7 +274,17 @@ pub impl LoanContext {
274274
if !owns_lent_data ||
275275
self.bccx.is_subregion_of(self.scope_region, scope_ub)
276276
{
277-
if loan_kind.is_take() && !cmt.mutbl.is_mutable() {
277+
if cmt.mutbl.is_mutable() {
278+
// If this loan is a mutable loan, then mark the loan path (if
279+
// it exists) as being used. This is similar to the check
280+
// performed in check_loans.rs in check_assignment(), but this
281+
// is for a different purpose of having the 'mut' qualifier.
282+
for cmt.lp.each |lp| {
283+
for lp.node_id().each |&id| {
284+
self.tcx().used_mut_nodes.insert(id);
285+
}
286+
}
287+
} else if loan_kind.is_take() {
278288
// We do not allow non-mutable data to be "taken"
279289
// under any circumstances.
280290
return Err(bckerr {

src/librustc/middle/lint.rs

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ use core::prelude::*;
1313
use driver::session::Session;
1414
use driver::session;
1515
use middle::ty;
16+
use middle::pat_util;
1617
use util::ppaux::{ty_to_str};
1718

1819
use core::hashmap::HashMap;
@@ -86,6 +87,7 @@ pub enum lint {
8687

8788
unused_variable,
8889
dead_assignment,
90+
unused_mut,
8991
}
9092

9193
pub fn level_to_str(lv: level) -> &'static str {
@@ -277,6 +279,13 @@ pub fn get_lint_dict() -> LintDict {
277279
desc: "detect assignments that will never be read",
278280
default: warn
279281
}),
282+
283+
(~"unused_mut",
284+
LintSpec {
285+
lint: unused_mut,
286+
desc: "detect mut variables which don't need to be mutable",
287+
default: warn
288+
}),
280289
];
281290
let mut map = HashMap::new();
282291
do vec::consume(v) |_, (k, v)| {
@@ -499,6 +508,7 @@ fn check_item(i: @ast::item, cx: ty::ctxt) {
499508
check_item_deprecated_mutable_fields(cx, i);
500509
check_item_deprecated_drop(cx, i);
501510
check_item_unused_unsafe(cx, i);
511+
check_item_unused_mut(cx, i);
502512
}
503513

504514
// 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) {
954964
visit::visit_item(it, (), visit);
955965
}
956966

967+
fn check_item_unused_mut(tcx: ty::ctxt, it: @ast::item) {
968+
let check_pat: @fn(@ast::pat) = |p| {
969+
let mut used = false;
970+
let mut bindings = 0;
971+
do pat_util::pat_bindings(tcx.def_map, p) |_, id, _, _| {
972+
used = used || tcx.used_mut_nodes.contains(&id);
973+
bindings += 1;
974+
}
975+
if !used {
976+
let msg = if bindings == 1 {
977+
~"variable does not need to be mutable"
978+
} else {
979+
~"variables do not need to be mutable"
980+
};
981+
tcx.sess.span_lint(unused_mut, p.id, it.id, p.span, msg);
982+
}
983+
};
984+
985+
let visit_fn_decl: @fn(&ast::fn_decl) = |fd| {
986+
for fd.inputs.each |arg| {
987+
if arg.is_mutbl {
988+
check_pat(arg.pat);
989+
}
990+
}
991+
};
992+
993+
let visit = item_stopping_visitor(
994+
visit::mk_simple_visitor(@visit::SimpleVisitor {
995+
visit_local: |l| {
996+
if l.node.is_mutbl {
997+
check_pat(l.node.pat);
998+
}
999+
},
1000+
visit_fn: |_, fd, _, _, _| visit_fn_decl(fd),
1001+
visit_ty_method: |tm| visit_fn_decl(&tm.decl),
1002+
visit_struct_method: |sm| visit_fn_decl(&sm.decl),
1003+
visit_trait_method: |tm| {
1004+
match *tm {
1005+
ast::required(ref tm) => visit_fn_decl(&tm.decl),
1006+
ast::provided(m) => visit_fn_decl(&m.decl),
1007+
}
1008+
},
1009+
.. *visit::default_simple_visitor()
1010+
}));
1011+
visit::visit_item(it, (), visit);
1012+
}
1013+
9571014
fn check_fn(tcx: ty::ctxt, fk: &visit::fn_kind, decl: &ast::fn_decl,
9581015
_body: &ast::blk, span: span, id: ast::node_id) {
9591016
debug!("lint check_fn fk=%? id=%?", fk, id);

src/librustc/middle/liveness.rs

Lines changed: 25 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1516,9 +1516,8 @@ fn check_local(local: @local, self: @Liveness, vt: vt<@Liveness>) {
15161516

15171517
// Initializer:
15181518
self.warn_about_unused_or_dead_vars_in_pat(local.node.pat);
1519-
if !local.node.is_mutbl {
1520-
self.check_for_reassignments_in_pat(local.node.pat);
1521-
}
1519+
self.check_for_reassignments_in_pat(local.node.pat,
1520+
local.node.is_mutbl);
15221521
}
15231522
None => {
15241523

@@ -1702,12 +1701,15 @@ pub impl Liveness {
17021701
match expr.node {
17031702
expr_path(_) => {
17041703
match *self.tcx.def_map.get(&expr.id) {
1705-
def_local(nid, false) => {
1706-
// Assignment to an immutable variable or argument:
1707-
// only legal if there is no later assignment.
1704+
def_local(nid, mutbl) => {
1705+
// Assignment to an immutable variable or argument: only legal
1706+
// if there is no later assignment. If this local is actually
1707+
// mutable, then check for a reassignment to flag the mutability
1708+
// as being used.
17081709
let ln = self.live_node(expr.id, expr.span);
17091710
let var = self.variable(nid, expr.span);
1710-
self.check_for_reassignment(ln, var, expr.span);
1711+
self.check_for_reassignment(ln, var, expr.span,
1712+
if mutbl {Some(nid)} else {None});
17111713
self.warn_about_dead_assign(expr.span, expr.id, ln, var);
17121714
}
17131715
def => {
@@ -1731,23 +1733,28 @@ pub impl Liveness {
17311733
}
17321734
}
17331735

1734-
fn check_for_reassignments_in_pat(@self, pat: @pat) {
1735-
do self.pat_bindings(pat) |ln, var, sp, _id| {
1736-
self.check_for_reassignment(ln, var, sp);
1736+
fn check_for_reassignments_in_pat(@self, pat: @pat, mutbl: bool) {
1737+
do self.pat_bindings(pat) |ln, var, sp, id| {
1738+
self.check_for_reassignment(ln, var, sp,
1739+
if mutbl {Some(id)} else {None});
17371740
}
17381741
}
17391742

17401743
fn check_for_reassignment(@self, ln: LiveNode, var: Variable,
1741-
orig_span: span) {
1744+
orig_span: span, mutbl: Option<node_id>) {
17421745
match self.assigned_on_exit(ln, var) {
17431746
Some(ExprNode(span)) => {
1744-
self.tcx.sess.span_err(
1745-
span,
1746-
~"re-assignment of immutable variable");
1747-
1748-
self.tcx.sess.span_note(
1749-
orig_span,
1750-
~"prior assignment occurs here");
1747+
match mutbl {
1748+
Some(id) => { self.tcx.used_mut_nodes.insert(id); }
1749+
None => {
1750+
self.tcx.sess.span_err(
1751+
span,
1752+
~"re-assignment of immutable variable");
1753+
self.tcx.sess.span_note(
1754+
orig_span,
1755+
~"prior assignment occurs here");
1756+
}
1757+
}
17511758
}
17521759
Some(lnk) => {
17531760
self.tcx.sess.span_bug(

src/librustc/middle/mem_categorization.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -351,6 +351,16 @@ pub impl MutabilityCategory {
351351
}
352352
}
353353

354+
pub impl loan_path {
355+
fn node_id(&self) -> Option<ast::node_id> {
356+
match *self {
357+
lp_local(id) | lp_arg(id) => Some(id),
358+
lp_deref(lp, _) | lp_comp(lp, _) => lp.node_id(),
359+
lp_self => None
360+
}
361+
}
362+
}
363+
354364
pub impl mem_categorization_ctxt {
355365
fn cat_expr(&self, expr: @ast::expr) -> cmt {
356366
match self.tcx.adjustments.find(&expr.id) {

src/librustc/middle/ty.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,11 @@ struct ctxt_ {
304304
// Set of used unsafe nodes (functions or blocks). Unsafe nodes not
305305
// present in this set can be warned about.
306306
used_unsafe: @mut HashSet<ast::node_id>,
307+
308+
// Set of nodes which mark locals as mutable which end up getting used at
309+
// some point. Local variable definitions not in this set can be warned
310+
// about.
311+
used_mut_nodes: @mut HashSet<ast::node_id>,
307312
}
308313

309314
pub enum tbox_flag {
@@ -933,6 +938,7 @@ pub fn mk_ctxt(s: session::Session,
933938
destructors: @mut HashSet::new(),
934939
trait_impls: @mut HashMap::new(),
935940
used_unsafe: @mut HashSet::new(),
941+
used_mut_nodes: @mut HashSet::new(),
936942
}
937943
}
938944

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
// Copyright 2013 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// Exercise the unused_mut attribute in some positive and negative cases
12+
13+
#[allow(dead_assignment)];
14+
#[allow(unused_variable)];
15+
#[deny(unused_mut)];
16+
17+
fn main() {
18+
// negative cases
19+
let mut a = 3; //~ ERROR: variable does not need to be mutable
20+
let mut a = 2, b = 3; //~ ERROR: variable does not need to be mutable
21+
//~^ ERROR: variable does not need to be mutable
22+
let mut a = ~[3]; //~ ERROR: variable does not need to be mutable
23+
24+
// positive cases
25+
let mut a = 2;
26+
a = 3;
27+
let mut a = ~[];
28+
a.push(3);
29+
let mut a = ~[];
30+
do callback {
31+
a.push(3);
32+
}
33+
}
34+
35+
fn callback(f: &fn()) {}
36+
37+
// make sure the lint attribute can be turned off
38+
#[allow(unused_mut)]
39+
fn foo(mut a: int) {
40+
let mut a = 3;
41+
let mut b = ~[2];
42+
}

0 commit comments

Comments
 (0)