Skip to content

Interactive Shell: Printing top level expression #2698

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Interactive Shell: Printing top level expression #2698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/libasr/pass/global_stmts.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ void pass_wrap_global_stmts(Allocator &al,
target = return_var_ref;
idx++;
} else {
// TODO: We will need to add support to return other ASR::ttypeType::*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need to handle all other cases like list, set, dict, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are going to fail due to this reason.

throw LCompilersException("Return type not supported in interactive mode");
}
ASR::stmt_t* asr_stmt = ASRUtils::STMT(ASR::make_Assignment_t(al, loc, target, value, nullptr));
Expand Down Expand Up @@ -119,7 +120,7 @@ void pass_wrap_global_stmts(Allocator &al,
/* a_body */ body.p,
/* n_body */ body.size(),
/* a_return_var */ (return_var ? return_var_ref : nullptr),
(return_var ? ASR::abiType::BindC : ASR::abiType::Source),
ASR::abiType::Source,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed this because asr_to_llvm changes the final function depending on ABI type (Look at

lpython/src/libasr/codegen/asr_to_llvm.cpp

Lines 3794 to 3805 in 476d9bb

if (ASRUtils::get_FunctionType(x)->m_abi == ASR::abiType::BindC) {
if (ASRUtils::get_FunctionType(x)->m_bindc_name) {
fn_name = ASRUtils::get_FunctionType(x)->m_bindc_name;
} else {
fn_name = sym_name;
}
} else if (ASRUtils::get_FunctionType(x)->m_deftype == ASR::deftypeType::Interface &&
ASRUtils::get_FunctionType(x)->m_abi != ASR::abiType::Intrinsic) {
fn_name = sym_name;
} else {
fn_name = mangle_prefix + sym_name;
}
). We don't want the function name to change, it becomes difficult to figure out the correct function name in PythonCompile.evaluate.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if this change works with LFortran? Simply submit a PR with this exact change and we can see if the CI passes.

ASR::Public, ASR::Implementation,
nullptr,
false, false, false, false, false,
Expand Down
37 changes: 36 additions & 1 deletion src/lpython/python_evaluator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,48 @@ Result<PythonCompiler::EvalResult> PythonCompiler::evaluate(
}

bool call_run_fn = false;
std::string return_type;
if (m->get_return_type(run_fn) != "none") {
call_run_fn = true;
return_type = m->get_return_type(run_fn);
}

e->add_module(std::move(m));
if (call_run_fn) {
e->voidfn(run_fn);
if (return_type == "integer4") {
int32_t r = e->int32fn(run_fn);
result.type = EvalResult::integer4;
result.i32 = r;
} else if (return_type == "integer8") {
int64_t r = e->int64fn(run_fn);
result.type = EvalResult::integer8;
result.i64 = r;
} else if (return_type == "real4") {
float r = e->floatfn(run_fn);
result.type = EvalResult::real4;
result.f32 = r;
} else if (return_type == "real8") {
double r = e->doublefn(run_fn);
result.type = EvalResult::real8;
result.f64 = r;
} else if (return_type == "complex4") {
std::complex<float> r = e->complex4fn(run_fn);
result.type = EvalResult::complex4;
result.c32.re = r.real();
result.c32.im = r.imag();
} else if (return_type == "complex8") {
std::complex<double> r = e->complex8fn(run_fn);
result.type = EvalResult::complex8;
result.c64.re = r.real();
result.c64.im = r.imag();
} else if (return_type == "void") {
e->voidfn(run_fn);
result.type = EvalResult::statement;
} else if (return_type == "none") {
result.type = EvalResult::none;
} else {
throw LCompilersException("FortranEvaluator::evaluate(): Return type not supported");
}
Comment on lines +145 to +149
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking we could accept struct as the return type here for dict, list, etc. And handle it accordingly.
But I have no idea how would we print it later.
Example:

>>> n: list[i32] = [1, 2, 3]
>>> n.append(4)
>>> n

We would like to print the list n in the last line. But it would be returned as a struct here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wrap this code into a new function that returns the return variable (or populates it when passed as argument).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok will do it in the next commit.

}

if (call_run_fn) {
Expand Down
19 changes: 13 additions & 6 deletions src/lpython/semantics/python_ast_to_asr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4088,8 +4088,8 @@ class SymbolTableVisitor : public CommonVisitor<SymbolTableVisitor> {

// Every module goes into a Module_t
SymbolTable *parent_scope = current_scope;
ASR::Module_t* module_sym = nullptr;
if (parent_scope->get_scope().find(module_name) == parent_scope->get_scope().end()) {
ASR::Module_t* module_sym = nullptr;
current_scope = al.make_new<SymbolTable>(parent_scope);
ASR::asr_t *tmp1 = ASR::make_Module_t(al, x.base.base.loc,
/* a_symtab */ current_scope,
Expand All @@ -4103,21 +4103,29 @@ class SymbolTableVisitor : public CommonVisitor<SymbolTableVisitor> {
for (size_t i=0; i<x.n_body; i++) {
visit_stmt(*x.m_body[i]);
}

LCOMPILERS_ASSERT(module_sym != nullptr);
module_sym->m_dependencies = current_module_dependencies.p;
module_sym->n_dependencies = current_module_dependencies.size();
if (!overload_defs.empty()) {
create_GenericProcedure(x.base.base.loc);
}
} else {
ASR::Module_t* module_sym =
ASR::down_cast<ASR::Module_t>(parent_scope->resolve_symbol(module_name));
module_sym = ASR::down_cast<ASR::Module_t>(parent_scope->resolve_symbol(module_name));
LCOMPILERS_ASSERT(module_sym != nullptr);
current_scope = module_sym->m_symtab;
for (size_t i=0; i<x.n_body; i++) {
visit_stmt(*x.m_body[i]);
}
for (size_t i = 0; i < module_sym->n_dependencies; i++) {
if (module_sym->m_dependencies[i]) {
current_module_dependencies.push_back(al, module_sym->m_dependencies[i]);
}
}
module_sym->m_dependencies = current_module_dependencies.p;
module_sym->n_dependencies = current_module_dependencies.size();
if (!overload_defs.empty()) {
create_GenericProcedure(x.base.base.loc);
}
Comment on lines +4124 to +4128
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This portion seems to be the same as that of the if part. So, let's move this out of the if-else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should be able to do that. Will do it in the next commit.

}

global_scope = nullptr;
Expand Down Expand Up @@ -6691,8 +6699,7 @@ class BodyVisitor : public CommonVisitor<BodyVisitor> {
// 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) ) {
LCOMPILERS_ASSERT(ASR::is_a<ASR::expr_t>(*tmp));
if( tmp && !ASR::is_a<ASR::expr_t>(*tmp) ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is changed to keep track of the final expression executed, and printing its result.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get this. Could you elaborate this, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please look at the difference in the AST generated with this change and without this change (i.e. this PR and the main branch). It may become easier to understand. Start the interactive shell with -v option to print the AST and LLVM IR.

With this change, the pass_wrap_global_stmt will generate a function that returns the result of the expression tmp, avoiding the modifications you specified with make_dummy_assignment (#2698 (review)).

tmp = nullptr;
}
}
Expand Down
Loading