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

Fix some const asserts not throwing errors #205

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cmlsharp
Copy link
Contributor

@cmlsharp cmlsharp commented Aug 4, 2024

Currently some assertions that can be statically determined to be false, do not cause a compile time error to be thrown, while some do.

For example,

def main() -> bool:
    assert(2u32 == 3u32)
    return true

causes this error to be thrown

Error: Const assert failed: (no error message given) at
    assert(2u32 == 3u32)
; context:
    assert(2u32 == 3u32)

whereas this essentially identical program does not

def main() -> bool: 
    u32 x = 2
    assert(x == 3)
    return true

(instead circ happily generates a circuit that always returns false)

Needless to say this can make debugging difficult. Luckily, I believe fixing this is a one line change. The culprit seems to be here:

match self.expr_impl_::<true>(&e.expression).and_then(|v| {

Since expr_impl is called with IS_CNST = true, it returns Err in the latter program since x is not a const variable. However, as far as I can tell, this check is unnecessary. The relevant matter is whether (after constant folding), the expression evaluates to false which is what the subsequent call to const_bool is checking anyway. Hence the solution seems to be to call expr_impl with IS_CNST = false instead.

@cmlsharp
Copy link
Contributor Author

cmlsharp commented Aug 4, 2024

Further testing reveals that this change can cause the frontend to slow down substantially on large programs (presumably due to a lot of redundant constant folding, with each call using a fresh, empty cache)

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

Successfully merging this pull request may close these issues.

1 participant