modularml / mojo

The Mojo Programming Language
https://docs.modular.com/mojo/manual/
Other
23.05k stars 2.59k forks source link

[BUG] Compiler bug of walrus operator with multiple `and`s #2783

Open YichengDWu opened 4 months ago

YichengDWu commented 4 months ago

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
soraros commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

YichengDWu commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

YichengDWu commented 4 months ago

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

soraros commented 4 months ago

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 commented 4 months ago

Sorry there was a typo. Corrected:

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