Emit a diagnostic when parentheses are omitted in logical expressions with comparisons

[whitequark]

E.g. foo and bar == 0 is OK, but foo & bar == 0 is parsed differently due to precedence, and virtually always results in a nasty bug.

It's not completely clear what would be the robust pattern to detect here, but it's the single biggest nMigen footgun, so it's probably better to come up with a less reliable than ideal diagnostic than have nothing at all.

See also #159, which fixes a similar issue. The diagnostic in #159 is only triggered in m.If conditions, which is (now that I think about it) incomplete; it's entirely possible that someone will capture or assign such an expression and get a broken design anyway. Ideally we also need a more robust fix for that.

[RobertBaruch]

Very footgun. Much debug.

I'm wondering if just steering people to use a different construct as a best practice would be sufficient. For example, instead of this, which is prone to error:

        with m.If((self._instr_phase == 0) & ~self._execute_trap):

Use:

        with m.If(all(self._instr_phase == 0, ~self._execute_trap)):

or something similar to all, which takes any number of statements and requires each to be 1.

[whitequark]

That doesn't actually work, try it. (Unless we override all, which is even worse.)

I know how to detect this issue mostly reliably; it just needs to be implemented.

[RobertBaruch]

I mean something like this (which seems to have worked):

def allcond(*args):
    cond = 1
    for arg in args:
        cond &= arg.bool()
    return cond

  with m.If(allcond(self._instr_phase == 0, ~self._execute_trap)):

[RobertBaruch]

Could make this all_true and also any_true, all_false, any_false to complete the set. Again, this is just to get people away from using bitwise-& as a boolean operator.

[whitequark]

I'm not a fan. This trades off some complexity in the DSL for another kind of complexity that is not clearly better, and in addition, effectively splits the language into two dialects: one that uses & and one that uses thes new operators.

[RobertBaruch]

So you'd be able to detect m.If(x == 1 & y) (a possible footgun) versus m.If(x == (1 & y)) (fully unambiguous)? Because wouldn't the parse tree be identical?

[whitequark]

Er, nevermind, I misread what you wrote.

[whitequark]

I think what we can do is warn on

m.If(x == (1 & y)):

where the shape of x isn't unsigned(1), with the suppression being:

m.If(x == (1 & y).bool()):

which is a no-op when 1 & y is intended to be a boolean operation, or else:

m.If(x == (1 & y)[:]):

which is a no-op when 1 & y is intended to be a bit container operation.

Thoughts? The second suppression is kind of ugly, I don't know how often it will fire.