Possible regression: x > y == z should not be allowed

[certik]

This used to correctly fail:

semantic error: Type mismatch in comparison operator, the types must be compatible
 --> /Users/ondrej/repos/lpython/integration_tests/expr_22.py:8:12
  |
8 |     assert x > y == z
  |            ^^^^^    ^ type mismatch ('bool' and 'i32')

But now it works. It seems this could should keep returning the same error, so we should add a test for this error message.

[certik]

The AST seems correct:

        (Assert
            (Compare
                (Compare
                    (Name
                        x
                        Load
                    )
                    Gt
                    [(Name
                        y
                        Load
                    )]
                )
                Eq
                [(Name
                    z
                    Load
                )]
            )
            ()
        )

It becomes assert (x > y) == z. This is now translated as the following in ASR:

                                    (Assert
                                        (LogicalBinOp
                                            (IntegerCompare
                                                (Var 3 x)
                                                Gt
                                                (Var 3 y)
                                                (Logical 4)
                                                ()
                                            )
                                            And
                                            (IntegerCompare
                                                (Var 3 y)
                                                Eq
                                                (Var 3 z)
                                                (Logical 4)
                                                ()
                                            )
                                            (Logical 4)
                                            ()
                                        )
                                        ()
                                    )

This seems equivalent to (x > y) and (y == z), which seems incorrect? Or is that how CPython works?

I think this was introduced in #2213.

[certik]

I think #2212 introduced a bug. It's subtle.

Consider this in regular CPython:

>>> (2 > 1)
True
>>> (2 > 1) == 2
False
>>> (2 > 1) == 1
True
>>> (3 > 2) == 2
False
>>> (3 > 2) == 1
True

The LHS gets evaluated as True. Then the condition just becomes:

>>> True == 2
False
>>> True == 1
True

As CPython casts integers to bool. We should not do any automatic casting.

[certik]

To see the failure, apply this patch:

--- a/integration_tests/expr_22.py
+++ b/integration_tests/expr_22.py
@@ -1,11 +1,12 @@
 from lpython import i32
 
 def f():
-    x: i32 = 2
-    y: i32 = 1
-    z: i32 = 1
-    t: i32 = 1
+    x: i32 = 3
+    y: i32 = 2
+    z: i32 = 2
+    t: i32 = 2
     assert x > y == z
+    assert (x > y) == z
     assert not (x == y == z)
     assert y == z == t != x
     assert x > y == z >= t

This will pass in LPython but fail in CPython:

$ PYTHONPATH=../src/runtime/lpython python expr_22.py
Traceback (most recent call last):
  File "/Users/ondrej/repos/lpython/integration_tests/expr_22.py", line 23, in <module>
    f()
  File "/Users/ondrej/repos/lpython/integration_tests/expr_22.py", line 9, in f
    assert (x > y) == z
AssertionError

The issue is the difference in parsing for x > y == z vs (x > y) == z, which I think in LPython we do not distinguish, but CPython does and it means something completely different. It's subtle.

I think for now let's revert #2213, because right now we parse x > y == z as (x > y) == z. And (x > y) == z is currently handled incorrectly. So let's first get (x > y) == z to behave correctly as CPython. In LPython it should just give the original error message that bool and i32 is incompatible.

Then to fix this, we first need to extend the parser to parse x > y == z into a flat structure, and then we hook it up in semantics to behave as (x > y) and (y == z) which we can then compile as #2213.

@Smit-create let me know if this makes sense.

[Smit-create]

You're right! I didn't notice that LPython parses both of them to same AST. I'll revert the PR and raise the error for mismatch type.

[certik]

Thanks! Sorry about that --- also open that issue that it closed. I think the fix is not complicated, but it must be done slightly differently already in the parser. I also didn't notice when I approved your PR. I just noticed by accident that I updated my master, but didn't recompile and it failed in that test, and the error message looked great, while the test looked wrong. So that's how I noticed.