Support multiple assignments

[Smit-create]

Fixes #928

[certik]

Thanks. Can you add a test for this as well please?

[certik]

Let me try to understand. This:

    a = b = c = 10

creates this ASR:

               (= 
                  (Var 2 c) (IntegerConstant 10 
                  (Integer 4 [])) 
                  (= 
                     (Var 2 b) (IntegerConstant 10 
                     (Integer 4 [])) 
                     (= 
                        (Var 2 a) (IntegerConstant 10 
                        (Integer 4 [])) ())))] () 

So this is doing c=10 and overloading it recursively, so we have b=10 and it is overloaded with a=10. I think that will not work with the way overloaded works: overloaded allows to specify a different (user defined) operation, typically a subroutine call. We should have checked this in verify, but our verify is not very strong yet.

This in turn is the reason for this change:

    void visit_Assignment(const ASR::Assignment_t &x) {
        if( x.m_overloaded ) {
            this->visit_stmt(*x.m_overloaded);
-           return ;
        }

But it will break LFortran, as in there we must do the overloaded operation instead of the original one. So I think the current approach will not work.

So how should this be implemented?

Well, what is a = b = c = 10?

Isn't that exactly equivalent to:

a = 10
b = 10
c = 10

? Or is there some semantic difference?

If it is equivalent, then the AST->ASR should encounter the AST node(s) for a = b = c = 10, and it should generate three ASR statement nodes (append them to the body) of the type a = 10, no overload.

There is a slight issue of appending more than one statement, but I think we have some mechanism for it, and if not, we need to create it.

[Smit-create]

Maybe we can follow the same logic but let's leave the overload for now, and implement a new argument in the Assignment node something like multiple_assignment

 stmt
     = Allocate(alloc_arg* args, expr? stat, expr? errmsg, expr? source)
     | Assign(int label, identifier variable)
-    | Assignment(expr target, expr value, stmt? overloaded)
+    | Assignment(expr target, expr value, stmt? overloaded, stmt? multiple_assignment)
     | Associate(expr target, expr value)
     | Cycle()

or,
One approach might be to insert all the individual assignment nodes and then visit them.

[certik]

It seems quite complicated, because every backend then has to support it etc. We are trying to keep the ASR as minimal as possible, but without losing any semantic information. It's not black and white, but this feature seems quite rarely used, so using the workaround with multiple Assignments seems like a better way, than modifying ASR. We should only extend ASR when we need to represent the semantic operation, or it simplifies the backends. In this case, it seems we do not necessarily need to preserve this operation, and it makes backends more complicated. So it seems it is not worth doing that way.

[czgdp1807]

I think converting a = b = c = 10 into,

c = 10
b = c
a = b

should be doable. We should do this because it keeps the ASR simple. And semantics of a = b = c = 10 and the above three assignments are no different for LPython (as we do deepcopy by default). If we support shallow copy in future then also, c = 10 will work as it is and b = c and a = b will eventually make a, b point to c (which is the expected behaviour for shallow copy I think).

Appending multiple statements to body can be implemented in multiple ways. You can track the current body by making it a state of the visitor then modify it directly when you create multiple assignment statements. Not so safe approach. The other way is to make Vec<ASR::stmt_t*> stmts and then use it at places where body.push_back is called. Anyways should be doable.

[Smit-create]

This is ready for review @czgdp1807 @certik

[certik]

Rather than assigning to both tmp and tmp_vec, why not use the convention that if it is just one, it will be in tmp, and if tmp == nullptr, then it will be in tmp_vec?

[certik]

Here is how I am thinking about it:

• tmp is not null: we have exactly one statement, the most common case
• tmp is null: the tmp_vec is used to communicate what is returned: tmp_vec.size() == 0 (nothing), size() == 2 or more (two or more statements).

We could in principle only use tmp_vec even for 1 statement (and remove tmp), but I think the tmp convention is used for expressions as well, and it seems like the above idea is better.

[czgdp1807]

Yes. I agree.

[certik]

In this case we should update other code that currently returns tmp=null to mean "none" to ensure it sets tmp_vec.size() == 0.

[czgdp1807]

Yeah. Otherwise incorrect set of statements mind end up getting into ASR. Well we should always clear the tmp_vec after pushing all the elements from it to the body. That way we will know that its always empty once used.

[certik]

Looks good to me. @czgdp1807 let me know if this looks good to you. See also my question above regarding the convention of tmp and tmp_vec.

[czgdp1807]

Let's try with variables of different types as well. Something like,

d: f64; e: f32; f: c64; g: i32
c = d = e = g + 1.0

Also try with lists, tuples (of the same element types) variables everywhere in the multiple assignment statement.

[certik]

Something like this?

Suggested change

	tmp = make_DictInsert_t(al, x.base.base.loc, se, key, tmp_value);
	tmp_vec.push_back(tmp);
	tmp = nullptr;
	tmp_vec.push_back(make_DictInsert_t(al, x.base.base.loc, se, key, tmp_value));

[certik]

Wait, or is this just one? In that case, just put it into tmp, and leave tmp_vec be.

[Smit-create]

No, we can have many of them. We have a continue statement below this.

[certik]

I think this is good to be merged, after adding the documentation comment (see my comment above) and polishing the git history.

[certik]

Awesome, thanks for implementing this @Smit-create !