Variable of list[i32] type has unnecessary newline character while printing.

[faze-geek]

A new line character is encountered before and after the list[i32] type. I checked integers, floats, strings which give expected answers when printed together.

def main1():
    a : i32 = 1
    s : str ="abc"
    l : list[i32] = [1,2,3,4,5]
    print(a,l,s)

main1()

(lp) C:\Users\kunni\lpython>python try.py
1 [1, 2, 3, 4, 5] abc

(lp) C:\Users\kunni\lpython>src\bin\lpython try.py
   Creating library try.lib and object try.exp
1
[1, 2, 3, 4, 5]
abc

[certik]

Yes, that's the same issue in LFortran. We need to fix this in ARS->LLVM.

[anutosh491]

Hello @certik , I'll be interested in fixing this issue , though being relatively new to lpython I haven't been able to figure out where the issue is or what exactly goes wrong .

There's no problem while printing an integer and say a string together , so I thought of using this as my base . I compared the ASTs, ASRs and LLVM IRs of both programs (printing int and a string and printing int and a list) and the AST/ASRs look relatively similar so I think it's not here where the issue is. Though I don't understand LLVM IRs that well as of now hence can't say if there errors here but yeah they both looked a bit different .

[Thirumalai-Shaktivel]

@anutosh491 go ahead and start working on this issue.
You can look into visit_print() (more precisely handle_print()) in asr_to_llvm.cpp, which handles the print statement.
Also, we have an asr_pass to print list, you can look into it as well.

[anutosh491]

Thanks for the information . I've been looking into it , especially print_list.cpp which helps us print lists whenever present in print statements. While going through the documentation for the ASR pass

lpython/src/libasr/pass/print_list.cpp

Lines 11 to 28 in 00ba081

	/*
	This ASR pass replaces print list or print tuple with print every value,
	comma_space, brackets and newline. The function
	`pass_replace_print_list` transforms the ASR tree in-place.
	Converts:
	print(a, b, l, sep="pqr", end="xyz") # l is a list (but not a & b)
	to:
	print(a, b, sep="pqr")
	print("[", end="")
	for i in range(len(l)):
	print(l[i], end="")
	if i < len(l) - 1:
	print(", ", end="")
	print("]", sep="pqr", end="xyz")

, I can see what's going wrong

These things aren't equivalent is what I realize . The first line should be print(a, b, sep="pqr", end="") and not print(a, b, sep="pqr") because otherwise there will be a newline after printing a, b which we don't want . So I was somewhat concerned with 2 functions from print_list.cpp which are visit_Print and print_list_helper . The latter one helps us print lists but as of now I don't think we're printing the list wrongly ... so I guess something needs to be done in the visit_Print function .

Have I decoded it correctly ? I am not so sure if the change needs to be addressed in visit_Print function of print_list.cpp or asr_to_llvm.cpp

[anutosh491]

Also while trying out some cases , I realized sep and end keywords are not supported correctly in lpython (would like someone to confirm this) . I tried the following

(lf) anutosh491@spbhat68:~/lpython/lpython$ cat examples/expr2.py 
def main0():
    a: i32 = 1
    b: i32 = 2
    c: str = "abc"
    d: str = "def"
    e: list[i32] = [1, 2]
    f: list[i32] = [3, 4]
    print(a, b, sep = "pqr", end = "xyz")
    print(a, c, sep = "pqr", end = "xyz")
    print(c, d, sep = "pqr", end = "xyz")
    print(e, f, sep = "pqr", end = "xyz")

main0()

And got the following output

(lf) anutosh491@spbhat68:~/lpython/lpython$ ./src/bin/lpython examples/expr2.py
1 2
1 abc
abc def
[1, 2]
[3, 4]

It seems #687, #704 were meant to address these , so we might need to look into this too !

[anutosh491]

Also while trying out some cases , I realized sep and end keywords are not supported correctly in lpython (would like someone to confirm this)

I tried to debug what might be going wrong here . I see that x.m_end and x.m_seperators are not assigned to the correct values .

lpython/src/libasr/codegen/asr_to_llvm.cpp

Lines 5796 to 5813 in 00ba081

	template <typename T>
	void handle_print(const T &x) {
	std::vector<llvm::Value *> args;
	std::vector<std::string> fmt;
	llvm::Value *sep = nullptr;
	llvm::Value *end = nullptr;
	if (x.m_separator) {
	this->visit_expr_wrapper(x.m_separator, true);
	sep = tmp;
	} else {
	sep = builder->CreateGlobalStringPtr(" ");
	}
	if (x.m_end) {
	this->visit_expr_wrapper(x.m_end, true);
	end = tmp;
	} else {
	end = builder->CreateGlobalStringPtr("\n");
	}

Here if the program flow doesn't enter the if(x.m_end) block because x.m_end turns out to be 0 which is not expected . Hence we get use the default end keyword of newline . @Thirumalai-Shaktivel would be glad if you could guide me regarding these issues .

[Thirumalai-Shaktivel]

#1477 (comment)
Yup, I think so.

The first line should be print(a, b, sep="pqr", end="") and not print(a, b, sep="pqr")

Yup, try to implement this and submit a PR, rest we will discuss there.

#1477 (comment)
There seem to be a bug in pass or ASR->LLVM that assign null to x.end and x.sep. We have to debug this.

[Thirumalai-Shaktivel]

print(a, b, l)
# becomes
print(a, b) # <-- change this to print(a, b, end="")
print("[", end="")
print(l.items, end="")
print("]", end="\n")

I think we have to handle this correctly inside this function:

lpython/src/libasr/pass/print_list.cpp

Line 303 in 00ba081

if (ASR::is_a<ASR::List_t>(*ASRUtils::expr_type(x.m_values[i])) ||

[Thirumalai-Shaktivel]

#1477 (comment)
I see, here is the bug:

lpython/src/libasr/pass/print_arr.cpp

Lines 123 to 125 in 00ba081

	print_stmt = ASRUtils::STMT(ASR::make_Print_t(
	al, x.base.base.loc, nullptr, body.p, body.size(),
	nullptr, nullptr));

We assign null to both x.end and x.sep, so the output.
@anutosh491 you can submit PRs for fixing these issues.
you understood how to fix them or work on them, right? If you have any doubts please ask.

[anutosh491]

I think we have to handle this correctly inside this function:

lpython/src/libasr/pass/print_list.cpp

Line 303 in 00ba081

if (ASR::is_a<ASR::List_t>(*ASRUtils::expr_type(x.m_values[i])) ||

Yeah correct, I had analyzed this similarly ... I think it has something to do with this make_Print_t function

lpython/src/libasr/pass/print_list.cpp

Lines 312 to 315 in 00ba081

	ASR::make_Print_t(al, x.base.base.loc, nullptr, tmp_vec.p, tmp_vec.size(),
	x.m_separator, nullptr));
	print_tmp.clear();
	pass_result.push_back(al, print_stmt);

Here the last parameter passed shouldn't be a nullptr . That is being dealt as a newline .... we would just like to have a space there I guess . Could you help me like build an object of type Print_t such that it's end keyword is registered as a space (" "), something along these lines ( I am struggling a bit with these things as I am not sure if there's documentation for the same ?)

expr_t *separator_expr = ASRUtils::EXPR(ASR::make_Const_t(al, x.base.base.loc, " "));
ASR::stmt_t *print_stmt = ASRUtils::STMT(
    ASR::make_Print_t(al, x.base.base.loc, nullptr, tmp_vec.p, tmp_vec.size(), x.m_separator, separator_expr));

I feel this would do the job for us !

[anutosh491]

We assign null to both x.end and x.sep, so the output.

Correct , once we address this , we've addressed the seperator and end issue .. I tried out some cases !

[anutosh491]

The following diff somewhat explains what I'm trying to convey in the above comment (#1477 (comment))

diff --git a/src/libasr/pass/print_list.cpp b/src/libasr/pass/print_list.cpp
index a4000f059..2a02c64e6 100644
--- a/src/libasr/pass/print_list.cpp
+++ b/src/libasr/pass/print_list.cpp
@@ -299,6 +299,10 @@ class PrintListVisitor
 
     void visit_Print(const ASR::Print_t &x) {
         std::vector<ASR::expr_t*> print_tmp;
+        ASR::ttype_t *str_type_len_1 = ASRUtils::TYPE(ASR::make_Character_t(
+        al, x.base.base.loc, 1, 1, nullptr, nullptr, 0));
+        ASR::expr_t *space = ASRUtils::EXPR(ASR::make_StringConstant_t(
+        al, x.base.base.loc, s2c(al, " "), str_type_len_1));
         for (size_t i=0; i<x.n_values; i++) {
             if (ASR::is_a<ASR::List_t>(*ASRUtils::expr_type(x.m_values[i])) ||
                 ASR::is_a<ASR::Tuple_t>(*ASRUtils::expr_type(x.m_values[i]))) {
@@ -310,13 +314,13 @@ class PrintListVisitor
                     }
                     ASR::stmt_t *print_stmt = ASRUtils::STMT(
                         ASR::make_Print_t(al, x.base.base.loc, nullptr, tmp_vec.p, tmp_vec.size(),
-                                    x.m_separator, nullptr));
+                                    x.m_separator, space));
                     print_tmp.clear();
                     pass_result.push_back(al, print_stmt);
 
                 }
                 if (ASR::is_a<ASR::List_t>(*ASRUtils::expr_type(x.m_values[i])))
-                    print_list_helper(x.m_values[i], x.m_separator, nullptr, x.base.base.loc);
+                    print_list_helper(x.m_values[i], x.m_separator, i == x.n_values - 1 ? x.m_end : space, x.base.base.loc);
                 else
                     print_tuple_helper(x.m_values[i], x.m_separator, nullptr, x.base.base.loc);
                 for (size_t j=0; j<print_pass_result_tmp.n; j++)

After applying this diff , I get correct results for all of these cases

def main1():
    a : i32 = 1
    l : list[i32] = [1,2,3]
    l2 : list[i32] = [4, 5, 6]
    s : str = "abc"
    print(a, l, s)
    print(l , a, s)
    print(a, s, l)
    print(a, l, s, end = " pqr ")
    print(l, a, s, end = " pqr ")
    print(a, s, l, end = " pqr ")

main1()

Output

1 [1, 2, 3] abc
[1, 2, 3] 1 abc
1 abc [1, 2, 3]
1 [1, 2, 3] abc pqr [1, 2, 3] 1 abc pqr 1 abc [1, 2, 3] pqr

[anutosh491]

The above snippet is only capable of addressing the end keyword , for addressing the sep keyword , we just need to build along the lines of how we handle the end cases . Doing that I got the following diff

diff --git a/src/libasr/pass/print_list.cpp b/src/libasr/pass/print_list.cpp
index a4000f059..f0f99f41d 100644
--- a/src/libasr/pass/print_list.cpp
+++ b/src/libasr/pass/print_list.cpp
@@ -299,25 +299,40 @@ class PrintListVisitor
 
     void visit_Print(const ASR::Print_t &x) {
         std::vector<ASR::expr_t*> print_tmp;
+        ASR::ttype_t *str_type_len_1 = ASRUtils::TYPE(ASR::make_Character_t(
+        al, x.base.base.loc, 1, 1, nullptr, nullptr, 0));
+        ASR::expr_t *space = ASRUtils::EXPR(ASR::make_StringConstant_t(
+        al, x.base.base.loc, s2c(al, " "), str_type_len_1));
         for (size_t i=0; i<x.n_values; i++) {
             if (ASR::is_a<ASR::List_t>(*ASRUtils::expr_type(x.m_values[i])) ||
                 ASR::is_a<ASR::Tuple_t>(*ASRUtils::expr_type(x.m_values[i]))) {
                 if (!print_tmp.empty()) {
                     Vec<ASR::expr_t*> tmp_vec;
+                    ASR::stmt_t *print_stmt;
                     tmp_vec.reserve(al, print_tmp.size());
                     for (auto &e: print_tmp) {
                         tmp_vec.push_back(al, e);
                     }
-                    ASR::stmt_t *print_stmt = ASRUtils::STMT(
+                    if(x.m_separator){
+                    print_stmt = ASRUtils::STMT(
+                        ASR::make_Print_t(al, x.base.base.loc, nullptr, tmp_vec.p, tmp_vec.size(),
+                                    x.m_separator, x.m_separator));
+                    } else{
+                    print_stmt = ASRUtils::STMT(
                         ASR::make_Print_t(al, x.base.base.loc, nullptr, tmp_vec.p, tmp_vec.size(),
-                                    x.m_separator, nullptr));
+                                    x.m_separator, space));
+                    }
                     print_tmp.clear();
                     pass_result.push_back(al, print_stmt);
 
                 }
-                if (ASR::is_a<ASR::List_t>(*ASRUtils::expr_type(x.m_values[i])))
-                    print_list_helper(x.m_values[i], x.m_separator, nullptr, x.base.base.loc);
-                else
+                if (ASR::is_a<ASR::List_t>(*ASRUtils::expr_type(x.m_values[i]))){
+                    if (x.m_separator){
+                        print_list_helper(x.m_values[i], x.m_separator, i == x.n_values - 1 ? x.m_end : x.m_separator, x.base.base.
loc);
+                    } else {
+                        print_list_helper(x.m_values[i], x.m_separator, i == x.n_values - 1 ? x.m_end : space, x.base.base.loc);
+                    }
+                } else
                     print_tuple_helper(x.m_values[i], x.m_separator, nullptr, x.base.base.loc);
                 for (size_t j=0; j<print_pass_result_tmp.n; j++)
                     pass_result.push_back(al, print_pass_result_tmp[j]);

As you can see I've made the changes only for the list container , the same could be made for the tuple container . Anyways the diff seems to address all cases related to the sep and end keyword

def main1():
    a : i32 = 1
    l : list[i32] = [1,2,3]
    l2 : list[i32] = [4, 5, 6]
    s : str = "abc"
    print(a, l, s)
    print(l , a, s)
    print(a, s, l)
    print(a, l, s, end = " pqr ")
    print(l, a, s, end = " pqr ")
    print(a, s, l, end = " pqr ")
    print(a, l, s, sep = "xyz")
    print(l, a, s, sep = "xyz")
    print(a, s, l, sep = "xyz")
    print(a, l, s, sep = "xyz", end = "pqr")
    print(l, a, s, sep = "xyz", end = "pqr")
    print(a, s, l, sep = "xyz", end = "pqr")
main1()

(lf) anutosh491@spbhat68:~/lpython/lpython$ ./src/bin/lpython examples/expr2.py 
1 [1, 2, 3] abc
[1, 2, 3] 1 abc
1 abc [1, 2, 3]
1 [1, 2, 3] abc pqr [1, 2, 3] 1 abc pqr 1 abc [1, 2, 3] pqr 1xyz[1, 2, 3]xyzabc
[1, 2, 3]xyz1xyzabc
1xyzabcxyz[1, 2, 3]
1xyz[1, 2, 3]xyzabcpqr[1, 2, 3]xyz1xyzabcpqr1xyzabcxyz[1, 2, 3]pqr(lf) anutosh491@spbhat68:~/lpython/lpython$ python examples/expr2.py 
1 [1, 2, 3] abc
[1, 2, 3] 1 abc
1 abc [1, 2, 3]
1 [1, 2, 3] abc pqr [1, 2, 3] 1 abc pqr 1 abc [1, 2, 3] pqr 1xyz[1, 2, 3]xyzabc
[1, 2, 3]xyz1xyzabc
1xyzabcxyz[1, 2, 3]
1xyz[1, 2, 3]xyzabcpqr[1, 2, 3]xyz1xyzabcpqr1xyzabcxyz[1, 2, 3]pqr(lf) anutosh491@spbhat68:~/lpython/lpython$

If you have some cases in mind which you would like me to check let me know @Thirumalai-Shaktivel ... otherwise I'm decently confident the diff would do the job !

[Thirumalai-Shaktivel]

Great job, I think this would be the correct fix.
Why don't you submit a PR with fixes and all the above tests in integration_tests?

[anutosh491]

Sure, will do it soon ! Thanks for your review.