Stop normalizing patterns

The check for whether a pat_ident is a variant or a binding
is simple and fast. Normalizing patterns again and again is
slow and error-prone (several places were forgetting to do it).

Author: marijnh

src/comp/middle/alias.rs

@@ -332,7 +332,7 @@ fn check_alt(cx: ctx, input: @ast::expr, arms: [ast::arm], sc: scope,
     for a: ast::arm in arms {
         let new_bs = sc.bs;
         let root_var = path_def_id(cx, root.ex);
-        let pat_id_map = pat_util::pat_id_map(cx.tcx, a.pats[0]);
+        let pat_id_map = pat_util::pat_id_map(cx.tcx.def_map, a.pats[0]);
         type info = {
             id: node_id,
             mutable unsafe_tys: [unsafe_ty],
@@ -596,13 +596,15 @@ fn pattern_roots(tcx: ty::ctxt, mutbl: option<unsafe_ty>, pat: @ast::pat)
     -> [pattern_root] {
     fn walk(tcx: ty::ctxt, mutbl: option<unsafe_ty>, pat: @ast::pat,
             &set: [pattern_root]) {
-        alt normalize_pat(tcx, pat).node {
-          ast::pat_wild | ast::pat_lit(_) | ast::pat_range(_, _) {}
-          ast::pat_ident(nm, sub) {
+        alt pat.node {
+          ast::pat_ident(nm, sub)
+          if !pat_util::pat_is_variant(tcx.def_map, pat) {
             set += [{id: pat.id, name: path_to_ident(nm), mutbl: mutbl,
                         span: pat.span}];
             alt sub { some(p) { walk(tcx, mutbl, p, set); } _ {} }
           }
+          ast::pat_wild | ast::pat_lit(_) | ast::pat_range(_, _) |
+          ast::pat_ident(_, _) {}
           ast::pat_enum(_, ps) | ast::pat_tup(ps) {
             for p in ps { walk(tcx, mutbl, p, set); }
           }

src/comp/middle/ast_map.rs

@@ -62,19 +62,25 @@ fn map_fn(fk: visit::fn_kind, decl: fn_decl, body: blk,
     visit::visit_fn(fk, decl, body, sp, id, cx, v);
 }
 
-fn map_local(loc: @local, cx: ctx, v: vt) {
-    pat_util::pat_bindings(loc.node.pat) {|p_id, _s, _p|
-        cx.map.insert(p_id, node_local(cx.local_id));
-        cx.local_id += 1u;
+fn number_pat(cx: ctx, pat: @pat) {
+    pat_util::walk_pat(pat) {|p|
+        alt p.node {
+          pat_ident(_, _) {
+            cx.map.insert(p.id, node_local(cx.local_id));
+            cx.local_id += 1u;
+          }
+          _ {}
+        }
     };
+}
+
+fn map_local(loc: @local, cx: ctx, v: vt) {
+    number_pat(cx, loc.node.pat);
     visit::visit_local(loc, cx, v);
 }
 
 fn map_arm(arm: arm, cx: ctx, v: vt) {
-    pat_util::pat_bindings(arm.pats[0]) {|p_id, _s, _p|
-        cx.map.insert(p_id, node_local(cx.local_id));
-        cx.local_id += 1u;
-    };
+    number_pat(cx, arm.pats[0]);
     visit::visit_arm(arm, cx, v);
 }
 

src/comp/middle/check_alt.rs

@@ -10,19 +10,18 @@ import middle::ty;
 import middle::ty::*;
 
 fn check_crate(tcx: ty::ctxt, crate: @crate) {
-    let v =
-        @{visit_expr: bind check_expr(tcx, _, _, _),
-          visit_local: bind check_local(tcx, _, _, _)
-             with *visit::default_visitor::<()>()};
-    visit::visit_crate(*crate, (), visit::mk_vt(v));
+    visit::visit_crate(*crate, (), visit::mk_vt(@{
+        visit_expr: bind check_expr(tcx, _, _, _),
+        visit_local: bind check_local(tcx, _, _, _)
+        with *visit::default_visitor::<()>()
+    }));
     tcx.sess.abort_if_errors();
 }
 
 fn check_expr(tcx: ty::ctxt, ex: @expr, &&s: (), v: visit::vt<()>) {
     visit::visit_expr(ex, s, v);
     alt ex.node {
       expr_alt(scrut, arms, mode) {
-        let arms = pat_util::normalize_arms(tcx, arms);
         check_arms(tcx, arms);
         /* Check for exhaustiveness */
         if mode == alt_exhaustive {
@@ -66,8 +65,6 @@ fn raw_pat(p: @pat) -> @pat {
     }
 }
 
-// Precondition: patterns have been normalized
-// (not checked statically yet)
 fn check_exhaustive(tcx: ty::ctxt, sp: span, pats: [@pat]) {
     if pats.len() == 0u {
         tcx.sess.span_err(sp, "non-exhaustive patterns");
@@ -216,11 +213,13 @@ fn pattern_supersedes(tcx: ty::ctxt, a: @pat, b: @pat) -> bool {
 
     alt a.node {
       pat_ident(_, some(p)) { pattern_supersedes(tcx, p, b) }
-      pat_wild | pat_ident(_, none) { true }
-      pat_lit(la) {
-        alt b.node {
-          pat_lit(lb) { lit_expr_eq(la, lb) }
-          _ { false }
+      pat_wild { true }
+      pat_ident(_, none) {
+        let opt_def_a = tcx.def_map.find(a.id);
+        alt opt_def_a {
+          some(def_variant(_, _)) { opt_def_a == tcx.def_map.find(b.id) }
+          // This is a binding
+          _ { true }
         }
       }
       pat_enum(va, suba) {
@@ -256,6 +255,12 @@ fn pattern_supersedes(tcx: ty::ctxt, a: @pat, b: @pat) -> bool {
           _ { pattern_supersedes(tcx, suba, b) }
         }
       }
+      pat_lit(la) {
+        alt b.node {
+          pat_lit(lb) { lit_expr_eq(la, lb) }
+          _ { false }
+        }
+      }
       pat_range(begina, enda) {
         alt b.node {
           pat_lit(lb) {
@@ -281,12 +286,19 @@ fn check_local(tcx: ty::ctxt, loc: @local, &&s: (), v: visit::vt<()>) {
 }
 
 fn is_refutable(tcx: ty::ctxt, pat: @pat) -> bool {
-    alt normalize_pat(tcx, pat).node {
+    alt tcx.def_map.find(pat.id) {
+      some(def_variant(enum_id, var_id)) {
+        if vec::len(*ty::enum_variants(tcx, enum_id)) != 1u { ret true; }
+      }
+      _ {}
+    }
+
+    alt pat.node {
       pat_box(sub) | pat_uniq(sub) | pat_ident(_, some(sub)) {
         is_refutable(tcx, sub)
       }
       pat_wild | pat_ident(_, none) { false }
-      pat_lit(_) { true }
+      pat_lit(_) | pat_range(_, _) { true }
       pat_rec(fields, _) {
         for it: field_pat in fields {
             if is_refutable(tcx, it.pat) { ret true; }
@@ -298,12 +310,9 @@ fn is_refutable(tcx: ty::ctxt, pat: @pat) -> bool {
         false
       }
       pat_enum(_, args) {
-        let vdef = variant_def_ids(tcx.def_map.get(pat.id));
-        if vec::len(*ty::enum_variants(tcx, vdef.enm)) != 1u { ret true; }
         for p: @pat in args { if is_refutable(tcx, p) { ret true; } }
         false
       }
-      pat_range(_, _) { true }
     }
 }
 

src/comp/middle/pat_util.rs

@@ -5,115 +5,66 @@ import syntax::fold;
 import syntax::fold::*;
 import syntax::codemap::span;
 
-export normalize_arms;
-export normalize_pat;
-export normalize_pat_def_map;
-export pat_binding_ids;
-export pat_bindings;
-export pat_id_map;
+export walk_pat;
+export pat_binding_ids, pat_bindings, pat_id_map;
+export pat_is_variant;
 export path_to_ident;
 
-fn normalize_pat_def_map(dm: resolve::def_map, p: @pat) -> @pat {
-  // have to do it the hard way b/c ast fold doesn't pass around
-  // node IDs. bother.
-  alt p.node {
-      pat_wild { p }
-      pat_ident(_, none) { normalize_one(dm, p) }
-      pat_ident(q, some(r)) {
-        @{node: pat_ident(q, some(normalize_pat_def_map(dm, r)))
-            with *p}
-      }
-      pat_enum(a_path, subs) {
-        @{node: pat_enum(a_path,
-           vec::map(subs, {|p| normalize_pat_def_map(dm, p)})) with *p}
-      }
-      pat_rec(field_pats, b) {
-        @{node: pat_rec(vec::map(field_pats,
-           {|fp| {pat: normalize_pat_def_map(dm, fp.pat) with fp}}), b)
-            with *p}
-      }
-      pat_tup(subs) {
-        @{node: pat_tup(vec::map(subs, {|p| normalize_pat_def_map(dm, p)}))
-            with *p}
-      }
-      pat_box(q) {
-        @{node: pat_box(normalize_pat_def_map(dm, q))
-            with *p}
-      }
-      pat_uniq(q) {
-        @{node: pat_uniq(normalize_pat_def_map(dm, q))
-            with *p}
-      }
-      pat_lit(_) { p }
-      pat_range(_,_) { p }
-    }
-}
-
-fn normalize_one(dm: resolve::def_map, p: @pat) -> @pat {
-    alt dm.find(p.id) {
-        some(d) {
-          alt p.node {
-              pat_ident(enum_path, _) { @{id: p.id,
-                    node: pat_enum(enum_path, []),
-                    span: p.span} }
-              _ { p }
-          }
-        }
-        none { p }
-    }
-}
-
-fn normalize_pat(tcx: ty::ctxt, p: @pat) -> @pat {
-  normalize_pat_def_map(tcx.def_map, p)
-}
-
-fn normalize_arms(tcx: ty::ctxt, arms:[arm]) -> [arm] {
-      vec::map(arms, {|a|
-            {pats:
-              vec::map(a.pats, {|p|
-                    pat_util::normalize_pat(tcx, p)})
-                with a}})
-}
-
 type pat_id_map = std::map::hashmap<str, node_id>;
 
 // This is used because same-named variables in alternative patterns need to
 // use the node_id of their namesake in the first pattern.
-fn pat_id_map(tcx: ty::ctxt, pat: @pat) -> pat_id_map {
-    let map = std::map::new_str_hash::<node_id>();
-    pat_bindings(normalize_pat(tcx, pat)) {|p_id, _s, n|
+fn pat_id_map(dm: resolve::def_map, pat: @pat) -> pat_id_map {
+    let map = std::map::new_str_hash();
+    pat_bindings(dm, pat) {|p_id, _s, n|
       map.insert(path_to_ident(n), p_id);
     };
     ret map;
 }
 
+fn pat_is_variant(dm: resolve::def_map, pat: @pat) -> bool {
+    alt pat.node {
+      pat_enum(_, _) { true }
+      pat_ident(_, none) {
+        alt dm.find(pat.id) {
+          some(def_variant(_, _)) { true }
+          _ { false }
+        }
+      }
+      _ { false }
+    }
+}
+
 // This does *not* normalize. The pattern should be already normalized
 // if you want to get a normalized pattern out of it.
 // Could return a constrained type in order to express that (future work)
-fn pat_bindings(pat: @pat, it: fn(node_id, span, @path)) {
-  alt pat.node {
-      pat_ident(pth, option::none) { it(pat.id, pat.span, pth); }
-      pat_ident(pth, option::some(sub)) { it(pat.id, pat.span, pth);
-        pat_bindings(sub, it); }
-      pat_enum(_, sub) { for p in sub { pat_bindings(p, it); } }
-      pat_rec(fields, _) { for f in fields { pat_bindings(f.pat, it); } }
-      pat_tup(elts) { for elt in elts { pat_bindings(elt, it); } }
-      pat_box(sub) { pat_bindings(sub, it); }
-      pat_uniq(sub) { pat_bindings(sub, it); }
-      pat_wild | pat_lit(_) | pat_range(_, _) { }
+fn pat_bindings(dm: resolve::def_map, pat: @pat,
+                it: fn(node_id, span, @path)) {
+    walk_pat(pat) {|p|
+        alt p.node {
+          pat_ident(pth, _) if !pat_is_variant(dm, p) {
+            it(p.id, p.span, pth);
+          }
+          _ {}
+        }
     }
 }
 
-fn pat_binding_ids(pat: @pat) -> [node_id] {
+fn walk_pat(pat: @pat, it: fn(@pat)) {
+    it(pat);
+    alt pat.node {
+      pat_ident(pth, some(p)) { walk_pat(p, it); }
+      pat_rec(fields, _) { for f in fields { walk_pat(f.pat, it); } }
+      pat_enum(_, s) | pat_tup(s) { for p in s { walk_pat(p, it); } }
+      pat_box(s) | pat_uniq(s) { walk_pat(s, it); }
+      pat_wild | pat_lit(_) | pat_range(_, _) | pat_ident(_, none) {}
+    }
+}
+
+fn pat_binding_ids(dm: resolve::def_map, pat: @pat) -> [node_id] {
     let found = [];
-    pat_bindings(pat) {|b_id, _sp, _pt| found += [b_id]; };
+    pat_bindings(dm, pat) {|b_id, _sp, _pt| found += [b_id]; };
     ret found;
 }
 
-fn path_to_ident(p: @path) -> ident {
-    alt vec::last(p.node.idents) {
-        none { // sigh
-          fail "Malformed path"; }
-      some(i) { ret i; }
-    }
-}
+fn path_to_ident(p: @path) -> ident { vec::last_total(p.node.idents) }

src/comp/middle/resolve.rs

@@ -624,7 +624,7 @@ fn visit_local_with_scope(e: @env, loc: @local, sc:scopes, v:vt<scopes>) {
     // a single identifier unambiguous (does the pattern "foo" refer
     // to enum foo, or is it binding a new name foo?)
     alt loc.node.pat.node {
-      pat_ident(an_ident,_) {
+      pat_ident(an_ident, _) {
           // Be sure to pass ns_an_enum to lookup_in_scope so that
           // if this is a name that's being shadowed, we don't die
           alt lookup_in_scope(*e, sc, loc.span,
@@ -1139,8 +1139,7 @@ fn lookup_in_ty_params(e: env, name: ident, ty_params: [ast::ty_param])
 fn lookup_in_pat(e: env, name: ident, pat: @ast::pat) -> option<def_id> {
     let found = none;
 
-    pat_util::pat_bindings(normalize_pat_def_map(e.def_map, pat))
-     {|p_id, _sp, n|
+    pat_util::pat_bindings(e.def_map, pat) {|p_id, _sp, n|
         if str::eq(path_to_ident(n), name)
                     { found = some(local_def(p_id)); }
     };
@@ -1776,7 +1775,7 @@ fn check_item(e: @env, i: @ast::item, &&x: (), v: vt<()>) {
 }
 
 fn check_pat(e: @env, ch: checker, p: @ast::pat) {
-    pat_util::pat_bindings(normalize_pat_def_map(e.def_map, p)) {|_i, p_sp, n|
+    pat_util::pat_bindings(e.def_map, p) {|_i, p_sp, n|
        add_name(ch, p_sp, path_to_ident(n));
     };
 }
@@ -1823,13 +1822,12 @@ fn check_block(e: @env, b: ast::blk, &&x: (), v: vt<()>) {
               ast::decl_local(locs) {
                 let local_values = checker(*e, "value");
                 for loc in locs {
-                     pat_util::pat_bindings
-                        (normalize_pat_def_map(e.def_map, loc.node.pat))
-                            {|_i, p_sp, n|
-                            let ident = path_to_ident(n);
-                            add_name(local_values, p_sp, ident);
-                            check_name(values, p_sp, ident);
-                          };
+                     pat_util::pat_bindings(e.def_map, loc.node.pat)
+                         {|_i, p_sp, n|
+                         let ident = path_to_ident(n);
+                         add_name(local_values, p_sp, ident);
+                         check_name(values, p_sp, ident);
+                     };
                 }
               }
               ast::decl_item(it) {