Fix logical comparison

[kmr-srbh]

Fixes #2597

Working

Numeric output

print(2.0 + 0.0 or 2.0 - 0.5)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
2.00000000000000000e+00

print(2.0 + 0.0 and 2.0 - 0.5)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
1.50000000000000000e+00

Logical output

print(2.0 > 0.0 or 2.0<0.5)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
True

print(2.0 > 0.0 and 2.0<0.5)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
False

Error for invalid operands

print(2.0 > 0.0 and 2.0**0.5)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
semantic error: Type mismatch: 'bool' and 'f64'. Operand should be of type 'bool'
 --> ./examples/example.py:1:21
  |
1 | print(2.0 > 0.0 and 2.0**0.5)
  |                     ^^^^^^^^ 

Note: Please report unclear or confusing messages as bugs at
https://github.com/lcompilers/lpython/issues.

from lpython import i32

list1: list[i32] = [1, 2, 3, 4, 5]
list2: list[i32] = [1, 2, 3]

print(list1 or list2)

(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
code generation error: Only Integer, Real, Strings and Logical types are supported in logical binary operation.
 --> ./examples/example.py:6:7
  |
6 | print(list1 or list2)
  |       ^^^^^^^^^^^^^^ 

Note: Please report unclear or confusing messages as bugs at
https://github.com/lcompilers/lpython/issues.

[kmr-srbh]

The above change breaks https://github.com/lcompilers/lpython/pull/1506, although I think that was not properly implemented. On main, the following works:

a : i32
b : i32
x : i32 
y : i32 
or_op1 : i32
or_op2 : i32
a = 1
b = 2
x = 3
y = 4
or_op1 = a or b
or_op2 = x or y
print(or_op1,or_op2)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
1 3

but the following throws an unhandled exception:

print(1 or 2)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
Internal Compiler Error: Unhandled exception
Traceback (most recent call last):
  Binary file "/home/saurabh-kumar/Projects/System/lpython/src/bin/lpython", in _start()
  File "./csu/../csu/libc-start.c", line 360, in __libc_start_main_impl()
  File "./csu/../sysdeps/x86/libc-start.c", line 58, in __libc_start_call_main()
  File "/home/saurabh-kumar/Projects/System/lpython/src/bin/lpython.cpp", line 1929, in main()
    err = compile_python_to_object_file(arg_file, tmp_o, runtime_library_dir,
  File "/home/saurabh-kumar/Projects/System/lpython/src/bin/lpython.cpp", line 838, in compile_python_to_object_file()
    !(arg_c && compiler_options.po.disable_main), "__main__", infile);
  File "/home/saurabh-kumar/Projects/System/lpython/src/lpython/semantics/python_ast_to_asr.cpp", line 8123, in LCompilers::LPython::python_ast_to_asr(Allocator&, LCompilers::LocationManager&, LCompilers::SymbolTable*, LCompilers::LPython::AST::ast_t&, LCompilers::diag::Diagnostics&, LCompilers::CompilerOptions&, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool)
    ast_overload, allow_implicit_casting);
  File "/home/saurabh-kumar/Projects/System/lpython/src/lpython/semantics/python_ast_to_asr.cpp", line 8060, in LCompilers::LPython::body_visitor(Allocator&, LCompilers::LocationManager&, LCompilers::LPython::AST::Module_t const&, LCompilers::diag::Diagnostics&, LCompilers::ASR::asr_t*, bool, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::map<int, LCompilers::ASR::symbol_t*, std::less<int>, std::allocator<std::pair<int const, LCompilers::ASR::symbol_t*> > >&, bool)
    b.visit_Module(ast);
  File "/home/saurabh-kumar/Projects/System/lpython/src/lpython/semantics/python_ast_to_asr.cpp", line 4740, in LCompilers::LPython::BodyVisitor::visit_Module(LCompilers::LPython::AST::Module_t const&)
    visit_stmt(*x.m_body[i]);
  File "/home/saurabh-kumar/Projects/System/lpython/src/lpython/python_ast.h", line 1883, in LCompilers::LPython::AST::BaseVisitor<LCompilers::LPython::BodyVisitor>::visit_stmt(LCompilers::LPython::AST::stmt_t const&)
    void visit_stmt(const stmt_t &b) { visit_stmt_t(b, self()); }
  File "/home/saurabh-kumar/Projects/System/lpython/src/lpython/python_ast.h", line 1773, in visit_stmt_t<LCompilers::LPython::BodyVisitor>()
    case stmtType::Expr: { v.visit_Expr((const Expr_t &)x); return; }
  File "/home/saurabh-kumar/Projects/System/lpython/src/lpython/semantics/python_ast_to_asr.cpp", line 6533, in LCompilers::LPython::BodyVisitor::visit_Expr(LCompilers::LPython::AST::Expr_t const&)
    visit_Call(*c);
  File "/home/saurabh-kumar/Projects/System/lpython/src/lpython/semantics/python_ast_to_asr.cpp", line 7591, in LCompilers::LPython::BodyVisitor::visit_Call(LCompilers::LPython::AST::Call_t const&)
    visit_expr_list(x.m_args, x.n_args, args);
  File "/home/saurabh-kumar/Projects/System/lpython/src/lpython/semantics/python_ast_to_asr.cpp", line 613, in LCompilers::LPython::CommonVisitor<LCompilers::LPython::BodyVisitor>::visit_expr_list(LCompilers::LPython::AST::expr_t**, unsigned long, LCompilers::Vec<LCompilers::ASR::call_arg_t>&)
    this->visit_expr(*exprs[i]);
  File "/home/saurabh-kumar/Projects/System/lpython/src/lpython/python_ast.h", line 1910, in LCompilers::LPython::AST::BaseVisitor<LCompilers::LPython::BodyVisitor>::visit_expr(LCompilers::LPython::AST::expr_t const&)
    void visit_expr(const expr_t &b) { visit_expr_t(b, self()); }
  File "/home/saurabh-kumar/Projects/System/lpython/src/lpython/python_ast.h", line 1784, in visit_expr_t<LCompilers::LPython::BodyVisitor>()
    case exprType::BoolOp: { v.visit_BoolOp((const BoolOp_t &)x); return; }
AssertFailed: ASR::is_a<ASR::Logical_t>(*dest_type)

[kmr-srbh]

@Shaikh-Ubaid I request guidance. What should we do? Throw a semantic error if both operands are not of type logical? But then it breaks #1506. Should we hint the programmer that currently both the types should be equal for the logical operators to work? Like this:

if (!ASRUtils::check_equal_type(ASRUtils::expr_type(lhs), ASRUtils::expr_type(rhs))) {
            diag.add(diag::Diagnostic(
                    "Logical operators currently only work with operands of same type",
                    diag::Level::Error, diag::Stage::Semantic, {
                        diag::Label("Operands are not of the same type (" + ASRUtils::type_to_str_python(ASRUtils::expr_type(lhs)) + ", " 
                + ASRUtils::type_to_str_python(ASRUtils::expr_type(rhs)) + ")", {x.base.base.loc})
                    })
                        );
            throw SemanticAbort();
        }

[Shaikh-Ubaid]

Throw a semantic error if both operands are not of type logical

I think we should throw an error. But let's wait for one more view on this. I shared it here https://lfortran.zulipchat.com/#narrow/stream/311866-LPython/topic/logical.20operation/near/425880351.

[kmr-srbh]

I think we should throw an error. But let's wait for one more view on this. I shared it here https://lfortran.zulipchat.com/#narrow/stream/311866-LPython/topic/logical.20operation/near/425880351.

I support the idea of explicit casting and an error otherwise too.

[Shaikh-Ubaid]

It looks like or is overloaded to work with logicals (for boolean operation) and other types. When other types are used, or works like a selective operation. It selects the first value if it evaluates to True, else the second value. For example, the following prints:

% python
Python 3.12.1 | packaged by conda-forge | (main, Dec 23 2023, 08:01:35) [Clang 16.0.6 ] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> 1 or 3
1
>>> 0 or 3
3

Throw a semantic error if both operands are not of type logical?

So, we cannot throw error if both operands are not of type logical. We need to think how to handle this properly.

[kmr-srbh]

We need to think how to handle this properly.

You are right. This was handled in #1506 and it works here too. The same is true for and.

from lpython import i32, i64, f32, f64

a: i32 = 1
b: i32 = 3
print(a or b)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
1

Although the above is buggy and fails for this - print(1 or 3).

CPython does this by assigning a truth value to every type. Does LPython have the same structure?

[Shaikh-Ubaid]

CPython does this by assigning a truth value to every type. Does LPython have the same structure?

Could you elaborate your query? Same structure in the sense, do you mean does LPython assigns a truth value to every type? You can compute the truth value to an operand by comparing to 0 or none or null. This is currently done in the llvm backend inside visit_LogicalBinOp().

[kmr-srbh]

Do you mean does LPython assigns a truth value to every type?

Yes. I see, as stated by you, that we do have truth values for all the basic types like Integer, Real and Character. The fix I see can be to just let go the checks for a LogicalConstant as we do need to operate on non-logical constants to obtain a value, and because we do have truth values for all these types. The case of a List, Dict or a Tuple can be separately handled. Although I think we should support a truth value for them - it can be used as a check for emptiness.

Please tell me your thoughts about this. I see that we cannot directly down cast the values to a logical constant. How do we go about this?

[Shaikh-Ubaid]

Please tell me your thoughts about this. I see that we cannot directly down cast the values to a logical constant. How do we go about this?

I am not sure if I idea get your idea correctly.

Please tell me your thoughts about this. I see that we cannot directly down cast the values to a logical constant. How do we go about this?

If you have any ideas/plans, please implement and push it here. I will review and we can see how to move forward with this.

For now we just need to tackle https://github.com/lcompilers/lpython/issues/2597#issuecomment-2002133981 and https://github.com/lcompilers/lpython/issues/2597#issuecomment-2002135527.

[Shaikh-Ubaid]

Please mark it as "Ready for review" when ready.

[kmr-srbh]

@Shaikh-Ubaid the latest commits fixes the issue for all the cases. The idea is simple, handle the case of logical operands and strings separately and let the backend do the work for the other types which support the operation. #1506 claimed to work for strings, but I think there is some issue with it.

Working

Numeric output

print(2.0 + 0.0 or 2.0 - 0.5)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
2.00000000000000000e+00

print(2.0 + 0.0 and 2.0 - 0.5)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
1.50000000000000000e+00

Logical output

print(2.0 > 0.0 or 2.0<0.5)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
True

print(2.0 > 0.0 and 2.0<0.5)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
False

Error for invalid operands

print(2.0 > 0.0 and 2.0**0.5)

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
semantic error: Type mismatch: 'bool' and 'f64'. Operand should be of type 'bool'
 --> ./examples/example.py:1:21
  |
1 | print(2.0 > 0.0 and 2.0**0.5)
  |                     ^^^^^^^^ 

Note: Please report unclear or confusing messages as bugs at
https://github.com/lcompilers/lpython/issues.

print("hello" or "world")

(lp) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
semantic error: Logical operation not supported on object of type 'str'
 --> ./examples/example.py:1:7
  |
1 | print("hello" or "world")
  |       ^^^^^^^ 

Note: Please report unclear or confusing messages as bugs at
https://github.com/lcompilers/lpython/issues.

[Shaikh-Ubaid]

I think the approach in this PR is in the right direction, but is not the correct fix.

1. to handle the assert failure in https://github.com/lcompilers/lpython/issues/2597#issue-2178514051, we just need to throw a SemanticError that the types are not equal (inplace of the LCOMPILERS_ASSERT)
2. next we need to fix the failure in https://github.com/lcompilers/lpython/issues/2597#issuecomment-2002135527 which currently expects/supports only logical types. I think we need to extend it to support other types.

[kmr-srbh]

I think the approach in this PR is in the right direction, but is not the correct fix.

1. to handle the assert failure in [Bug: Assert failure in logical operation #2597 (comment)](https://github.com/lcompilers/lpython/issues/2597#issue-2178514051), we just need to throw a SemanticError that the types are not equal (inplace of the LCOMPILERS_ASSERT)

2. next we need to fix the failure in [Bug: Assert failure in logical operation #2597 (comment)](https://github.com/lcompilers/lpython/issues/2597#issuecomment-2002135527) which currently expects/supports only logical types. I think we need to extend it to support other types.

@Shaikh-Ubaid The above 2 tasks are already solved by the latest commit.

For 1

print(2.0 > 0.0 and 2.0**0.5)

(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
semantic error: Type mismatch: 'bool' and 'f64'. Operand should be of type 'bool'
 --> ./examples/example.py:1:21
  |
1 | print(2.0 > 0.0 and 2.0**0.5)
  |                     ^^^^^^^^ 

Note: Please report unclear or confusing messages as bugs at
https://github.com/lcompilers/lpython/issues.

For 2

print(1 or 3)

(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
1

[Shaikh-Ubaid]

@Shaikh-Ubaid The above 2 tasks are already solved by the latest commit.

It might be working, but we need it to be in the right way. Currently, you explicitly check for when one type is logical and the other is not. But this check would not be needed when we do https://github.com/lcompilers/lpython/pull/2598#discussion_r1527569879. The approach I suggested is more general.

[kmr-srbh]

@Shaikh-Ubaid The above 2 tasks are already solved by the latest commit.

It might be working, but we need it to be in the right way. Currently, you explicitly check for when one type is logical and the other is not. But this check would not be needed when we do #2598 (comment). The approach I suggested is more general.

So, summing up everything, the tasks for me are:

1. Add type check to verify both operands are equal.
2. Evaluate the logical operation at compile time for all supported types.

Am I right @Shaikh-Ubaid ?

[Shaikh-Ubaid]

Add type check to verify both operands are equal.

Yes.

Evaluate the logical operation at compile time for all supported types.

Need not be all. Just ensure the examples that we discussed in https://github.com/lcompilers/lpython/pull/2598#issuecomment-2002559579 work.

[kmr-srbh]

@Shaikh-Ubaid I have made the changes requested by you. :+1: Strings are supported now.

[Shaikh-Ubaid]

Let's add some tests for this.

• one we need is an error reference test for the generation of a semantic error
• second we need is an integration test to check if print(1 or 3) works perfectly or not. Also ensure that we have an assert after the print.
• Feel free to test other types that we support in the selective or/and operation.

[kmr-srbh]

@Shaikh-Ubaid , why does the expr_value of a str variable become a nullptr? This is causing issues here as working with string variables leads to segfault.

[Shaikh-Ubaid]

why does the expr_value of a str variable become a nullptr?

expr_value returns the compile-time value. It becomes a nullptr, if the compile-time value is not available.

[Shaikh-Ubaid]

This is causing issues here as working with string variables leads to segfault.

Why don't you figure out the smallest test example that fails and we can figure out how to fix it from there.

[kmr-srbh]

This is causing issues here as working with string variables leads to segfault.

Why don't you figure out the smallest test example that fails and we can figure out how to fix it from there.

Fails for str variables

a: str = "hello"
b: str = "world"

print(a or b)

```console
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
Segmentation fault (core dumped)

Works for numeric variables

from lpython import i32

a: i32 = 1
b: i32 = 3

print(a or b)

(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
1

from lpython import f64

a: f64 = 1.33
b: f64 = 3.67

print(a and b)

(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
3.66999999999999993e+00

[kmr-srbh]

@Shaikh-Ubaid I do not know why, but the latest commit, that is the addition of the tests, causes other tests related to string methods and dictionaries, tuples to fail. I request you to have a look.

[kmr-srbh]

@Shaikh-Ubaid the issue related to str variables persists.

[Shaikh-Ubaid]

Did you try this https://github.com/lcompilers/lpython/pull/2598#discussion_r1528106831?

[kmr-srbh]

Did you try this #2598 (comment)?

Yes. I had forgotten to push that change, but the segfault persists.

@Shaikh-Ubaid I do not know why, but the latest commit, that is the addition of the tests, causes other tests related to string methods and dictionaries, tuples to fail. I request you to have a look.

This problem is now solved. Thank you very much!