Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
_match.rs: prune sub-match tree too aggressively
The `_match.rs` takes advantage of passes prior to `trans` and
aggressively prunes the sub-match tree based on exact equality. When it
comes to literal or range, the strategy may lead to wrong result if
there's guard function or multiple patterns inside tuple.

Closes #12582.
Closes #13027.
  • Loading branch information
edwardw committed Mar 27, 2014
1 parent c83994e commit 4112941
Show file tree
Hide file tree
Showing 3 changed files with 280 additions and 41 deletions.
122 changes: 81 additions & 41 deletions src/librustc/middle/trans/_match.rs
Expand Up @@ -256,43 +256,23 @@ enum Opt {
vec_len(/* length */ uint, VecLenOpt, /*range of matches*/(uint, uint))
}

fn lit_to_expr(tcx: &ty::ctxt, a: &Lit) -> @ast::Expr {
match *a {
ExprLit(existing_a_expr) => existing_a_expr,
ConstLit(a_const) => const_eval::lookup_const_by_id(tcx, a_const).unwrap(),
UnitLikeStructLit(_) => fail!("lit_to_expr: unexpected struct lit"),
}
}

fn opt_eq(tcx: &ty::ctxt, a: &Opt, b: &Opt) -> bool {
match (a, b) {
(&lit(UnitLikeStructLit(a)), &lit(UnitLikeStructLit(b))) => a == b,
(&lit(a), &lit(b)) => {
match (a, b) {
(UnitLikeStructLit(a), UnitLikeStructLit(b)) => a == b,
_ => {
let a_expr;
match a {
ExprLit(existing_a_expr) => a_expr = existing_a_expr,
ConstLit(a_const) => {
let e = const_eval::lookup_const_by_id(tcx, a_const);
a_expr = e.unwrap();
}
UnitLikeStructLit(_) => {
fail!("UnitLikeStructLit should have been handled \
above")
}
}

let b_expr;
match b {
ExprLit(existing_b_expr) => b_expr = existing_b_expr,
ConstLit(b_const) => {
let e = const_eval::lookup_const_by_id(tcx, b_const);
b_expr = e.unwrap();
}
UnitLikeStructLit(_) => {
fail!("UnitLikeStructLit should have been handled \
above")
}
}

match const_eval::compare_lit_exprs(tcx, a_expr, b_expr) {
Some(val1) => val1 == 0,
None => fail!("compare_list_exprs: type mismatch"),
}
}
let a_expr = lit_to_expr(tcx, &a);
let b_expr = lit_to_expr(tcx, &b);
match const_eval::compare_lit_exprs(tcx, a_expr, b_expr) {
Some(val1) => val1 == 0,
None => fail!("compare_list_exprs: type mismatch"),
}
}
(&range(a1, a2), &range(b1, b2)) => {
Expand All @@ -310,6 +290,42 @@ fn opt_eq(tcx: &ty::ctxt, a: &Opt, b: &Opt) -> bool {
}
}

fn opt_overlap(tcx: &ty::ctxt, a: &Opt, b: &Opt) -> bool {
match (a, b) {
(&lit(a), &lit(b)) => {
let a_expr = lit_to_expr(tcx, &a);
let b_expr = lit_to_expr(tcx, &b);
match const_eval::compare_lit_exprs(tcx, a_expr, b_expr) {
Some(val1) => val1 == 0,
None => fail!("opt_overlap: type mismatch"),
}
}

(&range(a1, a2), &range(b1, b2)) => {
let m1 = const_eval::compare_lit_exprs(tcx, a1, b2);
let m2 = const_eval::compare_lit_exprs(tcx, b1, a2);
match (m1, m2) {
// two ranges [a1, a2] and [b1, b2] overlap iff:
// a1 <= b2 && b1 <= a2
(Some(val1), Some(val2)) => (val1 <= 0 && val2 <= 0),
_ => fail!("opt_overlap: type mismatch"),
}
}

(&range(a1, a2), &lit(b)) | (&lit(b), &range(a1, a2)) => {
let b_expr = lit_to_expr(tcx, &b);
let m1 = const_eval::compare_lit_exprs(tcx, a1, b_expr);
let m2 = const_eval::compare_lit_exprs(tcx, a2, b_expr);
match (m1, m2) {
// b is in range [a1, a2] iff a1 <= b and b <= a2
(Some(val1), Some(val2)) => (val1 <= 0 && 0 <= val2),
_ => fail!("opt_overlap: type mismatch"),
}
}
_ => fail!("opt_overlap: expect lit or range")
}
}

pub enum opt_result<'a> {
single_result(Result<'a>),
lower_bound(Result<'a>),
Expand Down Expand Up @@ -490,7 +506,7 @@ fn assert_is_binding_or_wild(bcx: &Block, p: @ast::Pat) {
}
}

type enter_pat<'a> = 'a |@ast::Pat| -> Option<Vec<@ast::Pat> >;
type enter_pat<'a> = 'a |@ast::Pat| -> Option<Vec<@ast::Pat>>;

fn enter_match<'r,'b>(
bcx: &'b Block<'b>,
Expand Down Expand Up @@ -632,16 +648,30 @@ fn enter_opt<'r,'b>(
let tcx = bcx.tcx();
let dummy = @ast::Pat {id: 0, node: ast::PatWild, span: DUMMY_SP};
let mut i = 0;
// By the virtue of fact that we are in `trans` already, `enter_opt` is able
// to prune sub-match tree aggressively based on exact equality. But when it
// comes to literal or range, that strategy may lead to wrong result if there
// are guard function or multiple patterns inside tuple; in that case, pruning
// based on the overlap of patterns is required.
//
// Ideally, when constructing the sub-match tree for certain arm, only those
// arms beneath it matter. But that isn't how algorithm works right now and
// all other arms are taken into consideration when computing `guarded` below.
// That is ok since each round of `compile_submatch` guarantees to trim one
// "column" of arm patterns and the algorithm will converge.
let guarded = m.iter().any(|x| x.data.arm.guard.is_some());
let multi_pats = m.len() > 0 && m[0].pats.len() > 1;
enter_match(bcx, tcx.def_map, m, col, val, |p| {
let answer = match p.node {
ast::PatEnum(..) |
ast::PatIdent(_, _, None) if pat_is_const(tcx.def_map, p) => {
let const_def = tcx.def_map.borrow().get_copy(&p.id);
let const_def_id = ast_util::def_id_of_def(const_def);
if opt_eq(tcx, &lit(ConstLit(const_def_id)), opt) {
Some(Vec::new())
} else {
None
let konst = lit(ConstLit(const_def_id));
match guarded || multi_pats {
false if opt_eq(tcx, &konst, opt) => Some(Vec::new()),
true if opt_overlap(tcx, &konst, opt) => Some(Vec::new()),
_ => None,
}
}
ast::PatEnum(_, ref subpats) => {
Expand All @@ -666,10 +696,20 @@ fn enter_opt<'r,'b>(
}
}
ast::PatLit(l) => {
if opt_eq(tcx, &lit(ExprLit(l)), opt) {Some(Vec::new())} else {None}
let lit_expr = lit(ExprLit(l));
match guarded || multi_pats {
false if opt_eq(tcx, &lit_expr, opt) => Some(Vec::new()),
true if opt_overlap(tcx, &lit_expr, opt) => Some(Vec::new()),
_ => None,
}
}
ast::PatRange(l1, l2) => {
if opt_eq(tcx, &range(l1, l2), opt) {Some(Vec::new())} else {None}
let rng = range(l1, l2);
match guarded || multi_pats {
false if opt_eq(tcx, &rng, opt) => Some(Vec::new()),
true if opt_overlap(tcx, &rng, opt) => Some(Vec::new()),
_ => None,
}
}
ast::PatStruct(_, ref field_pats, _) => {
if opt_eq(tcx, &variant_opt(bcx, p.id), opt) {
Expand Down
29 changes: 29 additions & 0 deletions src/test/run-pass/issue-12582.rs
@@ -0,0 +1,29 @@
// Copyright 2012-2014 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.

pub fn main() {
let x = 1;
let y = 2;

assert_eq!(3, match (x, y) {
(1, 1) => 1,
(2, 2) => 2,
(1..2, 2) => 3,
_ => 4,
});

// nested tuple
assert_eq!(3, match ((x, y),) {
((1, 1),) => 1,
((2, 2),) => 2,
((1..2, 2),) => 3,
_ => 4,
});
}
170 changes: 170 additions & 0 deletions src/test/run-pass/issue-13027.rs
@@ -0,0 +1,170 @@
// Copyright 2012-2014 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.

// Tests that match expression handles overlapped literal and range
// properly in the presence of guard function.

fn val() -> uint { 1 }

static CONST: uint = 1;

pub fn main() {
lit_shadow_range();
range_shadow_lit();
range_shadow_range();
multi_pats_shadow_lit();
multi_pats_shadow_range();
lit_shadow_multi_pats();
range_shadow_multi_pats();
}

fn lit_shadow_range() {
assert_eq!(2, match 1 {
1 if false => 1,
1..2 => 2,
_ => 3
});

let x = 0;
assert_eq!(2, match x+1 {
0 => 0,
1 if false => 1,
1..2 => 2,
_ => 3
});

assert_eq!(2, match val() {
1 if false => 1,
1..2 => 2,
_ => 3
});

assert_eq!(2, match CONST {
0 => 0,
1 if false => 1,
1..2 => 2,
_ => 3
});

// value is out of the range of second arm, should match wildcard pattern
assert_eq!(3, match 3 {
1 if false => 1,
1..2 => 2,
_ => 3
});
}

fn range_shadow_lit() {
assert_eq!(2, match 1 {
1..2 if false => 1,
1 => 2,
_ => 3
});

let x = 0;
assert_eq!(2, match x+1 {
0 => 0,
1..2 if false => 1,
1 => 2,
_ => 3
});

assert_eq!(2, match val() {
1..2 if false => 1,
1 => 2,
_ => 3
});

assert_eq!(2, match CONST {
0 => 0,
1..2 if false => 1,
1 => 2,
_ => 3
});

// ditto
assert_eq!(3, match 3 {
1..2 if false => 1,
1 => 2,
_ => 3
});
}

fn range_shadow_range() {
assert_eq!(2, match 1 {
0..2 if false => 1,
1..3 => 2,
_ => 3,
});

let x = 0;
assert_eq!(2, match x+1 {
100 => 0,
0..2 if false => 1,
1..3 => 2,
_ => 3,
});

assert_eq!(2, match val() {
0..2 if false => 1,
1..3 => 2,
_ => 3,
});

assert_eq!(2, match CONST {
100 => 0,
0..2 if false => 1,
1..3 => 2,
_ => 3,
});

// ditto
assert_eq!(3, match 5 {
0..2 if false => 1,
1..3 => 2,
_ => 3,
});
}

fn multi_pats_shadow_lit() {
assert_eq!(2, match 1 {
100 => 0,
0 | 1..10 if false => 1,
1 => 2,
_ => 3,
});
}

fn multi_pats_shadow_range() {
assert_eq!(2, match 1 {
100 => 0,
0 | 1..10 if false => 1,
1..3 => 2,
_ => 3,
});
}

fn lit_shadow_multi_pats() {
assert_eq!(2, match 1 {
100 => 0,
1 if false => 1,
0 | 1..10 => 2,
_ => 3,
});
}

fn range_shadow_multi_pats() {
assert_eq!(2, match 1 {
100 => 0,
1..3 if false => 1,
0 | 1..10 => 2,
_ => 3,
});
}

5 comments on commit 4112941

@bors
Copy link
Contributor

@bors bors commented on 4112941 Mar 27, 2014

Choose a reason for hiding this comment

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

saw approval from nikomatsakis
at edwardw@4112941

@bors
Copy link
Contributor

@bors bors commented on 4112941 Mar 27, 2014

Choose a reason for hiding this comment

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

merging edwardw/rust/match = 4112941 into auto

@bors
Copy link
Contributor

@bors bors commented on 4112941 Mar 27, 2014

Choose a reason for hiding this comment

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

edwardw/rust/match = 4112941 merged ok, testing candidate = e560db7

@bors
Copy link
Contributor

@bors bors commented on 4112941 Mar 27, 2014

Choose a reason for hiding this comment

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

@bors
Copy link
Contributor

@bors bors commented on 4112941 Mar 27, 2014

Choose a reason for hiding this comment

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

fast-forwarding master to auto = e560db7

Please sign in to comment.