-
Notifications
You must be signed in to change notification settings - Fork 176
Printing top-level Expressions #2716
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
Changes from 4 commits
81224d7
4ff446a
d7e9451
22efacf
49f46c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6693,10 +6693,15 @@ class BodyVisitor : public CommonVisitor<BodyVisitor> { | |||||||||||||||
| } | ||||||||||||||||
| this->visit_expr(*x.m_value); | ||||||||||||||||
|
|
||||||||||||||||
| // If tmp is a statement and not an expression | ||||||||||||||||
| // never cast into expression using ASRUtils::EXPR | ||||||||||||||||
| // Just ignore and exit the function naturally. | ||||||||||||||||
| if( tmp && !ASR::is_a<ASR::stmt_t>(*tmp) ) { | ||||||||||||||||
| if (eval_count > 0) { | ||||||||||||||||
| // In Interactive mode | ||||||||||||||||
| if ((tmp) && (!ASR::is_a<ASR::expr_t>(*tmp))) { | ||||||||||||||||
| tmp = nullptr; | ||||||||||||||||
| } | ||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a bit confused here. We make
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Although we do not want to print anything, but we do want
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Does it actually return an evaluated result? Or does it simply convert expressions into assignment statements?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. The statement will be executed and ❯ lp
>>> x: i32
>>> x
0
>>> x = 5
>>> x
5
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It returns the evaluated result. You can run the interactive in verbose mode with
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the current approach, I doubt that we are/would-be skipping some stmts like lpython/src/lpython/semantics/python_ast_to_asr.cpp Lines 5220 to 5221 in 13bb54e
See my comment below https://github.com/lcompilers/lpython/pull/2716/files#r1614616838 which I think would avoid this. |
||||||||||||||||
| } else if (tmp && !ASR::is_a<ASR::stmt_t>(*tmp)) { | ||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you check if doing the following works?
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes work. I have committed it. |
||||||||||||||||
| // If tmp is a statement and not an expression | ||||||||||||||||
| // never cast into expression using ASRUtils::EXPR | ||||||||||||||||
| // Just ignore and exit the function naturally. | ||||||||||||||||
| LCOMPILERS_ASSERT(ASR::is_a<ASR::expr_t>(*tmp)); | ||||||||||||||||
| tmp = nullptr; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -619,5 +619,168 @@ TEST_CASE("PythonCompiler 1") { | |
| LCompilers::Result<PythonCompiler::EvalResult> | ||
| r = e.evaluate2("1"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::none); // TODO: change to integer4 and check the value once printing top level expressions is implemented | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer4); | ||
| CHECK(r.result.i32 == 1); | ||
| } | ||
|
|
||
| TEST_CASE("PythonCompiler i32 expressions") { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add this as a separate test and not change the existing test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. I can do that. |
||
| CompilerOptions cu; | ||
| cu.po.disable_main = true; | ||
| cu.emit_debug_line_column = false; | ||
| cu.generate_object_code = false; | ||
| cu.interactive = true; | ||
| cu.po.runtime_library_dir = LCompilers::LPython::get_runtime_library_dir(); | ||
| PythonCompiler e(cu); | ||
| LCompilers::Result<PythonCompiler::EvalResult> | ||
|
|
||
| r = e.evaluate2("1"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer4); | ||
| CHECK(r.result.i32 == 1); | ||
|
|
||
| r = e.evaluate2("1 + 2"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer4); | ||
| CHECK(r.result.i32 == 3); | ||
|
|
||
| r = e.evaluate2("1 - 2"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer4); | ||
| CHECK(r.result.i32 == -1); | ||
|
|
||
| r = e.evaluate2("1 * 2"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer4); | ||
| CHECK(r.result.i32 == 2); | ||
|
|
||
| r = e.evaluate2("3 ** 3"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer4); | ||
| CHECK(r.result.i32 == 27); | ||
|
|
||
| r = e.evaluate2("4 // 2"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer4); | ||
| CHECK(r.result.i32 == 2); | ||
|
|
||
| r = e.evaluate2("4 / 2"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::real8); | ||
| CHECK(r.result.f64 == 2); | ||
| } | ||
|
|
||
| TEST_CASE("PythonCompiler i32 declaration") { | ||
| CompilerOptions cu; | ||
| cu.po.disable_main = true; | ||
| cu.emit_debug_line_column = false; | ||
| cu.generate_object_code = false; | ||
| cu.interactive = true; | ||
| cu.po.runtime_library_dir = LCompilers::LPython::get_runtime_library_dir(); | ||
| PythonCompiler e(cu); | ||
| LCompilers::Result<PythonCompiler::EvalResult> | ||
|
|
||
| r = e.evaluate2("i: i32"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::none); | ||
| r = e.evaluate2("i = 5"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::statement); | ||
| r = e.evaluate2("i"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer4); | ||
| CHECK(r.result.i32 == 5); | ||
|
Comment on lines
+682
to
+691
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test here makes sure that the execution of statements is not skipped. |
||
|
|
||
| r = e.evaluate2("j: i32 = 9"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::none); | ||
| r = e.evaluate2("j"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer4); | ||
| CHECK(r.result.i32 == 9); | ||
|
|
||
| r = e.evaluate2("i + j"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer4); | ||
| CHECK(r.result.i32 == 14); | ||
| } | ||
|
|
||
| TEST_CASE("PythonCompiler i64 expressions") { | ||
| CompilerOptions cu; | ||
| cu.po.disable_main = true; | ||
| cu.emit_debug_line_column = false; | ||
| cu.generate_object_code = false; | ||
| cu.interactive = true; | ||
| cu.po.runtime_library_dir = LCompilers::LPython::get_runtime_library_dir(); | ||
| PythonCompiler e(cu); | ||
| LCompilers::Result<PythonCompiler::EvalResult> | ||
|
|
||
| r = e.evaluate2("i64(1)"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer8); | ||
| CHECK(r.result.i64 == 1); | ||
|
|
||
| r = e.evaluate2("i64(1) + i64(2)"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer8); | ||
| CHECK(r.result.i64 == 3); | ||
|
|
||
| r = e.evaluate2("i64(1) - i64(2)"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer8); | ||
| CHECK(r.result.i64 == -1); | ||
|
|
||
| r = e.evaluate2("i64(1) * i64(2)"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer8); | ||
| CHECK(r.result.i64 == 2); | ||
|
|
||
| r = e.evaluate2("i64(3) ** i64(3)"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer8); | ||
| CHECK(r.result.i64 == 27); | ||
|
|
||
| r = e.evaluate2("i64(4) // i64(2)"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer8); | ||
| CHECK(r.result.i64 == 2); | ||
|
|
||
| r = e.evaluate2("i64(4) / i64(2)"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::real8); | ||
| CHECK(r.result.f64 == 2); | ||
| } | ||
|
|
||
| TEST_CASE("PythonCompiler i64 declaration") { | ||
| CompilerOptions cu; | ||
| cu.po.disable_main = true; | ||
| cu.emit_debug_line_column = false; | ||
| cu.generate_object_code = false; | ||
| cu.interactive = true; | ||
| cu.po.runtime_library_dir = LCompilers::LPython::get_runtime_library_dir(); | ||
| PythonCompiler e(cu); | ||
| LCompilers::Result<PythonCompiler::EvalResult> | ||
|
|
||
| r = e.evaluate2("i: i64"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::none); | ||
| r = e.evaluate2("i = i64(5)"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::statement); | ||
| r = e.evaluate2("i"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer8); | ||
| CHECK(r.result.i64 == 5); | ||
|
|
||
| r = e.evaluate2("j: i64 = i64(9)"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::none); | ||
| r = e.evaluate2("j"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer8); | ||
| CHECK(r.result.i64 == 9); | ||
|
|
||
| r = e.evaluate2("i + j"); | ||
| CHECK(r.ok); | ||
| CHECK(r.result.type == PythonCompiler::EvalResult::integer8); | ||
| CHECK(r.result.i64 == 14); | ||
| } | ||
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.
Did we previously add all the above code somewhere else? (It feels like we added it somewhere before).
If this is being duplicated then I think we should create a function out of it and call that function 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 had initially implemented this in #2698, but we did not merge. That PR lacked tests. We also merged other changes between that PR and this. I was thinking of closing that PR without merging it once we merge this PR.