Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
whitequark opened this issue May 5, 2020 · 8 comments

Comments

@whitequark
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member Author

whitequark commented Dec 5, 2020

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

Er, nevermind, I misread what you wrote.

@whitequark
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants