TMI: Letting your guard down

I haven't written anything under this tag in a long while. For most of that time, I've been head-down working on the let/match refactoring in trans. I feel like I've learned a ton about trans in the meantime that I will be able to translate into comments that will help future trans hackers (pun possibly intended). But I also feel embarrassed about not having a lot of visible work to show for the past month or month-and-a-half.

The middle of this week, though, I decided to set that aside since we have the 0.5 release coming up soon, and I didn't think I was going to finish #3235 in time. Instead, I decided to just try to fix as many release blockers as possible. I'm particularly satisfied to have fixed #3121 just now (well, it's not committed yet, still waiting for tests to run). I spent a good chunk of time trying to isolate this bug when it was newer, and I gave up because I didn't understand trans::alt well enough. But thanks to that time I've spent trying to refactor, when I looked at it again today, I was able to find the buggy code in just a few minutes. The bug is even simple enough to explain in terms of code (I think):

fn enter_default(bcx: block, dm: DefMap, m: &[@Match/&r],
                 col: uint, val: ValueRef)
    -> ~[@Match/&r]
{
    do enter_match(bcx, dm, m, col, val) |p| {
        match p.node {
          ast::pat_wild | ast::pat_rec(_, _) | ast::pat_tup(_) |
          ast::pat_struct(*) => Some(~[]),
          ast::pat_ident(_, _, None) if pat_is_binding(dm, p) => Some(~[]),
          _ => None
        }
    }
}

This is the code that takes a list of Matchs (structures representing patterns that may match) and narrows down that list to patterns that are irrefutable: that match any value of the right type. Examples are _, the wildcard pattern that matches anything, and x if x is a free variable (which just binds any value to the name x. enter_match is a more general function -- the details aren't important -- but the point is that it takes a higher-order function that classifies patterns. Here, the match on p says that the following kinds of patterns are defaults: wildcards (_); record, struct, and tuple patterns (that is, matches on constructors for product types), and variable patterns. The higher-order function returns Some(~[]) (again, the reason why doesn't matter) for defaults, and None for non-defaults.

That's all fine and good, but in Rust we have pattern guards; to use the example from #3121:

match *m {
      to_go(_) => { }
      for_here(_) if cond => {}
      for_here(hamburger) => {}
      for_here(fries(s)) => {}
      for_here(shake) => {}
    }

Normally, for_here(_) would match any value of type order (see the bug report for the type definitions), since the _ nested pattern matches anything. But the pattern guard -- written if cond -- means this pattern only matches if cond is true. So the _ here isn't really a default, because a guard makes the entire pattern that it's attached to refutable.

Once I realized this, the fix was quite simple. And happily, I got to close several other issues that turned out to be instances of the very same bug: #2869, #3257, and #3895.

There's no greater moral here, no fancy theory; just another mundane, prosaic day in the life of a compiler writer. The real issue was that pattern guards were added on after the fact, and probably not tested all that thoroughly -- but I certainly can't place any blame for that, since I generally don't put as much effort as I should into test coverage either.

ETA: Well, the fix wasn't as simple as that after all, sadly. Don't want to have an "I am so smart" moment here. :P Still working on it.