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

[BUG] Compiler bug of walrus operator with multiple ands #2783

Open
YichengDWu opened this issue May 22, 2024 · 12 comments
Open

[BUG] Compiler bug of walrus operator with multiple ands #2783

YichengDWu opened this issue May 22, 2024 · 12 comments
Labels
bug Something isn't working mojo-repo Tag all issues with this label QOI

Comments

@YichengDWu
Copy link
Contributor

YichengDWu commented May 22, 2024

Bug description

Having multiple assignments in if or while statement results in an error

Steps to reproduce

def main():
  if (a:=True) and (b:=True):
    print(a)
    print(b)
 
/source/prog.mojo:8:10: error: use of uninitialized value 'b'
    print(b)
         ^
/source/prog.mojo:6:21: note: 'b' declared here
  if (a:=True) and (b:=True):
                    ^
mojo: error: failed to run the pass manager

System information

The playground
@YichengDWu YichengDWu added bug Something isn't working mojo-repo Tag all issues with this label labels May 22, 2024
@soraros
Copy link
Contributor

soraros commented May 22, 2024

I don't think this one is a bug. It looks trivial in this particular case, but in general the compiler can't prove that b is always initialised because of short circuiting.

@YichengDWu
Copy link
Contributor Author

YichengDWu commented May 22, 2024

In general, for if (a:=A) and (b:=B), short circuiting means this branch is not taken and thus there's no need to worry about uninitialized references inside the branch. What you said is true for references outside the branch.

@bgreni
Copy link
Contributor

bgreni commented May 22, 2024

Regardless it's valid Python code so if possible it must work in Mojo as well.

@YichengDWu
Copy link
Contributor Author

That's a bigger problem. Python would raise a run-time error and mojo is doing some compile-time checks.

Note that the current compiler already know a must be initialized inside the branch, push also b into some kind of list in the pass is an easy and quick fix for this specific bug.

@bgreni
Copy link
Contributor

bgreni commented May 22, 2024

I meant that exact code runs successfully with no issue (however obviously it fails if you use and or instead). So not only should it be possible, but it is a regression from the expected behaviour in Python.

@YichengDWu
Copy link
Contributor Author

YichengDWu commented May 22, 2024

Yeah it's a serious issue if all valid python code with def should also be valid mojo code.

Mojo is a superset of Python.

@soraros
Copy link
Contributor

soraros commented May 22, 2024

In general, for if (a := A) and (b := B), short circuiting means this branch is not taken and thus there's no need to worry about uninitialized references inside the branch. What you said is true for references outside the branch.

I don't think that's correct. In a statement like if e1 and e2, b gets initialised only if e2 gets evaluated. We happens to be in a very special case where b has to be True. But consider this:

fn f(b: Bool) -> Bool:
  return ...

fn g(b: Bool) -> Bool:
  return ...

def test():
  if g((a := A) and f(b := B)):  # f is opaque
    print(a)
    print(b)

We can't guarantee that b is initialised. (Actually, we can if we only have a and.) One should also notice that the constructor of Bool is pretty side effect free. Should we have a more complex type, the unification problem True ~ f(?b) becomes super unsolvable.

Yeah it's a serious issue if all valid python code with def should also be valid mojo code.

Isn't it a good thing that a runtime error got turned into a compile time one?

Anyhow, my point being that we can't support even the slightly more general case until full dict based dynamism.

@YichengDWu
Copy link
Contributor Author

You changed my argument with evoking g.

Isn't it a good thing that a runtime error got turned into a compile time one?

Good if not buggy.

@soraros
Copy link
Contributor

soraros commented May 22, 2024

@YichengDWu So you would agree that if it was if (a := A) or (b := B), it should error out?

@YichengDWu
Copy link
Contributor Author

YichengDWu commented May 22, 2024

It depends on the level of dynamism. if (a := A) and (b := B) should not error on any level.

@soraros
Copy link
Contributor

soraros commented May 22, 2024

b is indeed not initialised if a is True, which is not buggy ("good if not buggy"). So I don't understand your argument.

@YichengDWu
Copy link
Contributor Author

YichengDWu commented May 22, 2024

Sorry there was a typo. Corrected:

if (a := A) and (b := B) should not error on any level.

@YichengDWu YichengDWu changed the title [BUG] walrus operator cannot handle multiple assignments [BUG] Compiler bug of walrus operator multiple with ands May 23, 2024
@YichengDWu YichengDWu changed the title [BUG] Compiler bug of walrus operator multiple with ands [BUG] Compiler bug of walrus operator with multiple ands May 23, 2024
@ematejska ematejska added the QOI label Jul 18, 2024 — with Linear
@ematejska ematejska removed the QOI label Jul 18, 2024
@ematejska ematejska added the QOI label Jul 18, 2024 — with Linear
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working mojo-repo Tag all issues with this label QOI
Projects
None yet
Development

No branches or pull requests

4 participants