Skip to content

Commit

Permalink
extend liveness to treat bindings more like other variables
Browse files Browse the repository at this point in the history
This results in a lot of warnings in rustc.  I left them in because
many are bugs and we should fix our code, but Graydon asked that
I not touch every file in the codebase.
  • Loading branch information
nikomatsakis committed Aug 24, 2012
1 parent aa024ac commit e47d2f6
Show file tree
Hide file tree
Showing 7 changed files with 158 additions and 25 deletions.
4 changes: 3 additions & 1 deletion src/libsyntax/print/pprust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1450,8 +1450,10 @@ fn print_pat(s: ps, &&pat: @ast::pat) {
word_nbsp(s, ~"ref");
print_mutability(s, mutbl);
}
ast::bind_by_move => {
word_nbsp(s, ~"move");
}
ast::bind_by_implicit_ref |
ast::bind_by_move | // this is totally the wrong thing
ast::bind_by_value => {}
}
print_path(s, path, true);
Expand Down
20 changes: 18 additions & 2 deletions src/rustc/middle/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,11 @@ enum lint {
vecs_implicitly_copyable,
deprecated_mode,
deprecated_pattern,
non_camel_case_types
non_camel_case_types,

// FIXME(#3266)--make liveness warnings lintable
// unused_variable,
// dead_assignment
}

fn level_to_str(lv: level) -> ~str {
Expand Down Expand Up @@ -134,7 +138,19 @@ fn get_lint_dict() -> lint_dict {
(~"non_camel_case_types",
@{lint: non_camel_case_types,
desc: ~"types, variants and traits must have camel case names",
default: allow})
default: allow}),

/* FIXME(#3266)--make liveness warnings lintable
(~"unused_variable",
@{lint: unused_variable,
desc: ~"detect variables which are not used in any way",
default: warn}),
(~"dead_assignment",
@{lint: dead_assignment,
desc: ~"detect assignments that will never be read",
default: warn}),
*/
];
hash_from_strs(v)
}
Expand Down
116 changes: 96 additions & 20 deletions src/rustc/middle/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ fn check_crate(tcx: ty::ctxt,
let visitor = visit::mk_vt(@{
visit_fn: visit_fn,
visit_local: visit_local,
visit_expr: visit_expr
visit_expr: visit_expr,
visit_arm: visit_arm,
with *visit::default_visitor()
});

Expand Down Expand Up @@ -191,11 +192,17 @@ enum RelevantDef { RelevantVar(node_id), RelevantSelf }

type CaptureInfo = {ln: LiveNode, is_move: bool, rv: RelevantDef};

enum LocalKind {
FromMatch(binding_mode),
FromLetWithInitializer,
FromLetNoInitializer
}

struct LocalInfo {
id: node_id;
ident: ident;
is_mutbl: bool;
initialized: bool;
kind: LocalKind;
}

enum VarKind {
Expand All @@ -209,7 +216,11 @@ enum VarKind {
fn relevant_def(def: def) -> option<RelevantDef> {
match def {
def_self(_) => some(RelevantSelf),
def_arg(nid, _) | def_local(nid, _) => some(RelevantVar(nid)),

def_binding(nid, _) |
def_arg(nid, _) |
def_local(nid, _) => some(RelevantVar(nid)),

_ => none
}
}
Expand Down Expand Up @@ -329,7 +340,16 @@ impl IrMaps {
match vk {
Arg(id, name, by_move) |
Arg(id, name, by_copy) |
Local(LocalInfo {id:id, ident:name, _}) => {
Local(LocalInfo {id:id, ident:name,
kind: FromLetNoInitializer, _}) |
Local(LocalInfo {id:id, ident:name,
kind: FromLetWithInitializer, _}) |
Local(LocalInfo {id:id, ident:name,
kind: FromMatch(bind_by_value), _}) |
Local(LocalInfo {id:id, ident:name,
kind: FromMatch(bind_by_ref(_)), _}) |
Local(LocalInfo {id:id, ident:name,
kind: FromMatch(bind_by_move), _}) => {
let v = match self.last_use_map.find(expr_id) {
some(v) => v,
none => {
Expand All @@ -342,7 +362,8 @@ impl IrMaps {
(*v).push(id);
}
Arg(_, _, by_ref) | Arg(_, _, by_mutbl_ref) |
Arg(_, _, by_val) | Self | Field(_) | ImplicitRet => {
Arg(_, _, by_val) | Self | Field(_) | ImplicitRet |
Local(LocalInfo {kind: FromMatch(bind_by_implicit_ref), _}) => {
debug!("--but it is not owned");
}
}
Expand Down Expand Up @@ -396,7 +417,8 @@ fn visit_fn(fk: visit::fn_kind, decl: fn_decl, body: blk,
let check_vt = visit::mk_vt(@{
visit_fn: check_fn,
visit_local: check_local,
visit_expr: check_expr
visit_expr: check_expr,
visit_arm: check_arm,
with *visit::default_visitor()
});
check_vt.visit_block(body, lsets, check_vt);
Expand All @@ -419,16 +441,39 @@ fn visit_local(local: @local, &&self: @IrMaps, vt: vt<@IrMaps>) {
debug!("adding local variable %d", p_id);
let name = ast_util::path_to_ident(path);
self.add_live_node_for_node(p_id, VarDefNode(sp));
let kind = match local.node.init {
some(_) => FromLetWithInitializer,
none => FromLetNoInitializer
};
self.add_variable(Local(LocalInfo {
id: p_id,
ident: name,
is_mutbl: local.node.is_mutbl,
initialized: local.node.init.is_some()
id: p_id,
ident: name,
is_mutbl: local.node.is_mutbl,
kind: kind
}));
}
visit::visit_local(local, self, vt);
}

fn visit_arm(arm: arm, &&self: @IrMaps, vt: vt<@IrMaps>) {
let def_map = self.tcx.def_map;
for arm.pats.each |pat| {
do pat_util::pat_bindings(def_map, pat) |bm, p_id, sp, path| {
debug!("adding local variable %d from match with bm %?",
p_id, bm);
let name = ast_util::path_to_ident(path);
self.add_live_node_for_node(p_id, VarDefNode(sp));
self.add_variable(Local(LocalInfo {
id: p_id,
ident: name,
is_mutbl: false,
kind: FromMatch(bm)
}));
}
}
visit::visit_arm(arm, self, vt);
}

fn visit_expr(expr: @expr, &&self: @IrMaps, vt: vt<@IrMaps>) {
match expr.node {
// live nodes required for uses or definitions of variables:
Expand Down Expand Up @@ -612,6 +657,30 @@ impl Liveness {
}
}

fn arm_pats_bindings(pats: &[@pat], f: fn(LiveNode, Variable, span)) {
// only consider the first pattern; any later patterns must have
// the same bindings, and we also consider the first pattern to be
// the "authoratative" set of ids
if !pats.is_empty() {
self.pat_bindings(pats[0], f)
}
}

fn define_bindings_in_pat(pat: @pat, succ: LiveNode) -> LiveNode {
self.define_bindings_in_arm_pats([pat], succ)
}

fn define_bindings_in_arm_pats(pats: &[@pat],
succ: LiveNode) -> LiveNode {
let mut succ = succ;
do self.arm_pats_bindings(pats) |ln, var, _sp| {
self.init_from_succ(ln, succ);
self.define(ln, var);
succ = ln;
}
succ
}

fn idx(ln: LiveNode, var: Variable) -> uint {
*ln * self.ir.num_vars + *var
}
Expand Down Expand Up @@ -894,13 +963,8 @@ impl Liveness {
// once at the func header but otherwise equivalent.

let opt_init = local.node.init.map(|i| i.expr );
let mut succ = self.propagate_through_opt_expr(opt_init, succ);
do self.pat_bindings(local.node.pat) |ln, var, _sp| {
self.init_from_succ(ln, succ);
self.define(ln, var);
succ = ln;
}
succ
let succ = self.propagate_through_opt_expr(opt_init, succ);
self.define_bindings_in_pat(local.node.pat, succ)
}

fn propagate_through_exprs(exprs: ~[@expr],
Expand Down Expand Up @@ -1003,10 +1067,12 @@ impl Liveness {
self.init_empty(ln, succ);
let mut first_merge = true;
for arms.each |arm| {
let body_succ =
self.propagate_through_block(arm.body, succ);
let guard_succ =
self.propagate_through_opt_expr(arm.guard, body_succ);
let arm_succ =
self.propagate_through_opt_expr(
arm.guard,
self.propagate_through_block(arm.body, succ));
self.define_bindings_in_arm_pats(arm.pats, guard_succ);
self.merge_from_succ(ln, arm_succ, first_merge);
first_merge = false;
};
Expand Down Expand Up @@ -1409,6 +1475,13 @@ fn check_local(local: @local, &&self: @Liveness, vt: vt<@Liveness>) {
visit::visit_local(local, self, vt);
}

fn check_arm(arm: arm, &&self: @Liveness, vt: vt<@Liveness>) {
do self.arm_pats_bindings(arm.pats) |ln, var, sp| {
self.warn_about_unused(sp, ln, var);
}
visit::visit_arm(arm, self, vt);
}

fn check_expr(expr: @expr, &&self: @Liveness, vt: vt<@Liveness>) {
match expr.node {
expr_path(_) => {
Expand Down Expand Up @@ -1798,10 +1871,12 @@ impl @Liveness {
};

if is_assigned {
// FIXME(#3266)--make liveness warnings lintable
self.tcx.sess.span_warn(
sp, fmt!("variable `%s` is assigned to, \
but never used", name));
} else {
// FIXME(#3266)--make liveness warnings lintable
self.tcx.sess.span_warn(
sp, fmt!("unused variable: `%s`", name));
}
Expand All @@ -1814,6 +1889,7 @@ impl @Liveness {
fn warn_about_dead_assign(sp: span, ln: LiveNode, var: Variable) {
if self.live_on_exit(ln, var).is_none() {
for self.should_warn(var).each |name| {
// FIXME(#3266)--make liveness warnings lintable
self.tcx.sess.span_warn(
sp,
fmt!("value assigned to `%s` is never read", name));
Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/borrowck-issue-2657-1.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
fn main() {
let x = some(~1);
match x { //~ NOTE loan of immutable local variable granted here
some(ref y) => {
some(ref _y) => {
let _a <- x; //~ ERROR moving out of immutable local variable prohibited due to outstanding loan
}
_ => {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ fn main() {
// fact no outstanding loan of x!
x = some(0);
}
some(ref i) => {
some(ref _i) => {
x = some(1); //~ ERROR assigning to mutable local variable prohibited due to outstanding loan
}
}
Expand Down
13 changes: 13 additions & 0 deletions src/test/compile-fail/liveness-unused.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,24 @@ fn f3b() {
fn f4() {
match some(3) {
some(i) => {
//~^ WARNING unused variable: `i`
}
none => {}
}
}

enum tri {
a(int), b(int), c(int)
}

fn f4b() -> int {
match a(3) {
a(i) | b(i) | c(i) => {
i
}
}
}

// leave this in here just to trigger compile-fail:
struct r {
let x: ();
Expand Down
26 changes: 26 additions & 0 deletions src/test/run-pass/option-unwrap.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
struct dtor {
x: @mut int;

drop {
// abuse access to shared mutable state to write this code
*self.x -= 1;
}
}

fn unwrap<T>(+o: option<T>) -> T {
match move o {
some(move v) => v,
none => fail
}
}

fn main() {
let x = @mut 1;

{
let b = some(dtor { x:x });
let c = unwrap(b);
}

assert *x == 0;
}

1 comment on commit e47d2f6

@brson
Copy link
Contributor

@brson brson commented on e47d2f6 Aug 26, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome.

Please sign in to comment.