-
Notifications
You must be signed in to change notification settings - Fork 157
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 logical comparison #2598
Fix logical comparison #2598
Conversation
The above change breaks #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) |
@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();
} |
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. |
It looks like % 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
So, we cannot throw error if both operands are not of type logical. 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 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 - 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 |
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 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.
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 #2597 (comment) and #2597 (comment). |
Please mark it as "Ready for review" when ready. |
@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. WorkingNumeric outputprint(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 outputprint(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 operandsprint(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. |
bool left_value = ASR::down_cast<ASR::LogicalConstant_t>( | ||
ASRUtils::expr_value(lhs))->m_value; | ||
bool right_value = ASR::down_cast<ASR::LogicalConstant_t>( | ||
ASRUtils::expr_value(rhs))->m_value; | ||
bool result; | ||
switch (op) { | ||
case (ASR::logicalbinopType::And): { result = left_value && right_value; break; } | ||
case (ASR::logicalbinopType::Or): { result = left_value || right_value; break; } | ||
default : { | ||
throw SemanticError("Boolean operator type not supported", | ||
x.base.base.loc); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change was made in this section. Only indentation was added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems an assert LCOMPILERS_ASSERT(ASR::is_a<ASR::Logical_t>(*dest_type));
is removed. I suggest not doing indentation changes in this PR. Instead you can submit a separate PR for indentation changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shaikh-Ubaid Removal of that assert statement was intentional. In this case, indentation was required to add readability to the if
block.
I think the approach in this PR is in the right direction, but is not the correct fix.
|
} else if(ASR::is_a<ASR::Logical_t>(*left_operand_type) | ||
&& !ASR::is_a<ASR::Logical_t>(*right_operand_type)) { | ||
throw SemanticError("Type mismatch: '" + ASRUtils::type_to_str_python(left_operand_type) | ||
+ "' and '" + ASRUtils::type_to_str_python(right_operand_type) | ||
+ "'. Operand should be of type 'bool'", rhs->base.loc); | ||
} else if(!ASR::is_a<ASR::Logical_t>(*left_operand_type) | ||
&& ASR::is_a<ASR::Logical_t>(*right_operand_type)) { | ||
throw SemanticError("Type mismatch: '" + ASRUtils::type_to_str_python(left_operand_type) | ||
+ "' and '" + ASRUtils::type_to_str_python(right_operand_type) | ||
+ "'. Operand should be of type 'bool'", lhs->base.loc); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checking will not be needed once we have https://github.com/lcompilers/lpython/pull/2598/files#r1527569879.
@Shaikh-Ubaid The above 2 tasks are already solved by the latest commit. For 1print(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 2print(1 or 3) (base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
1 |
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. |
@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. |
@Shaikh-Ubaid the issue related to |
Did you try this #2598 (comment)? |
Yes. I had forgotten to push that change, but the segfault persists.
This problem is now solved. Thank you very much! |
|
||
if (x.n_args > 0 && ASRUtils::is_array(ASRUtils::expr_type(args_[0])) && | ||
if (ASRUtils::is_array(ASRUtils::expr_type(args_[0])) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was done automatically when I pulled the new changes after yesterdays commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please undo it.
assert f_var == 6.28 | ||
|
||
|
||
test_logical_assignment() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Why is this test separate from the other? It seems to test
selective or
for strings, why is it called asassignment
? -
Can you add a print before the asserts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a separate test for testing assignments through logical operations that we implemented. I am adding the print statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, Let's merge this test with the other integration test added in this PR.
print([1, 2, 3] or [1, 2, 3, 4]) | ||
|
||
|
||
f() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do not need this test. It is redundant at the moment. Both the operands of the operation are of the same type, so we do not expect any SemanticError here. It is a different case that we do not support lists in selective or
operation for which we can open an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please ensure that you also remove the reference outputs associated with this test.
tests/tests.toml
Outdated
[[test]] | ||
filename = "../integration_tests/test_logical_compare.py" | ||
asr = true | ||
|
||
[[test]] | ||
filename = "../integration_tests/test_logical_assignment.py" | ||
asr = true | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove these from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am removing these. Same must be done for the error tests, right? Also, please guide me on when one should add a test here, and when not. I mean, the possible cases when one adds the test here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kmr-srbh we discussed it before. We add an integration test when we would like to check/verify if a the newly added feature compiles, executes and produces the expected output. We add a reference test when we would like to check/verify some intermediate output (for example a SemanticError) or when adding an integration test is not possible/feasible.
We mostly try to add integration tests. We use reference tests when adding integration test is not feasible. We add an error reference test when we want to test generation of a SemanticError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same must be done for the error tests
We need one error reference test to check for semantic error when different types are used with the selective or
operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sorry for asking the same question, my question stems from the fact that we have many integration_tests listed in the above file. This is a bit confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shared couple comments.
The integration tests also need to be enabled in integration_tests/CMakeLists.txt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good. Thanks!
# Can be uncommented after fixing the segfault | ||
# _LPYTHON: str = "LPython" | ||
# s_var: str = "" or _LPYTHON | ||
# assert s_var == "LPython" | ||
# print(s_var) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please open an issue for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shaikh-Ubaid What can be a possible way to solve this? This is the same problem which causes issues with the input()
accepting a string variable as an argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second check, I noticed one thing below.
Fixes #2597
Working
Numeric output
Logical output
Error for invalid operands