Remove addNodeLast from update chnages

[Alokzh]

Fixes: #17847

1. Replaced addNodeLast from RBAddSubtreeTransformation & updated its logic
2. Removed the addNodeLast method from OCSequenceNode
3. All Tests pass locally

[jecisc]

This

[Ducasse]

With Balsa we wrote a little visitor fully tested that adds the last node correctly.
I'm thinking that a good little project would to extract (rewrite out of the tree in a first place) the replacement logic. Because like that we could write nice tests and then remove the logic from the tree. Visitors are much nicer. If you are interested in this little project let me know.
the tasks would be

• check the tests if any for the replace logic
• enhance the tests following a systematic approach
• reuse the tests to start moving the behavior in a separate visitor
• replace existing code by a call to the new visitor.

[Alokzh]

Sure @Ducasse

I researched the whole codebase I could not find any visitor that adds the last node which you mentioned.
I found OCReturnNodeAdderVisitor so I tried to understand its implementation & probably got good idea how visitors work.

If you could help me finding where adding last node visitor is, I could definitely try this issue.

[guillep]

Isn't this a bit buggy?

I know you're just inlining the method here, but my point is:

• sometimes it adds the node as the last one
• sometimes as the one before the last one

I don't see how this could work in general, there are probably cases where this produces broken code

[Alokzh]

Thnx @guillep

But according to this current implementation first it checks if the last statement is a return statement or not
If it is, only then it adds the new node one before the last one else it adds new node at the end ( Don't you think this is correct behaviour )

Yeah there could be case where this might lead to unexpected behaviour
In case if we find that last statement is not a return statement then it will add the node in last which could lead to some issue

For example:

calculateSum [
    | a b result |
    a := 5.
    b := 10.
    result := a + b.
    result
]

Here it returns result implicitly & if we use this logic to add a new node let's say ( a * b ) then it will be added to last according to logic & the method becomes this

calculateSum [
    | a b result |
    a := 5.
    b := 10.
    result := a + b.
    result.
    a * b
]

Now in this case it will start returning different value.

But for this what we can do is , here we can use OCReturnNodeAdderVisitor to first wrap the node as return & then add new node one before the last.

But there might be some methods which intentionally does not want to return any value in that case we are forcing to return value so maybe again we will have to come up with some logic to find whether it has implicit return or not then wotk out according to that. I just wanted to tell the idea I came up with .

Am I making any sense here or this is completely wrong approach ?

If you have some time could you give me some example where the already implemented logic would produces broken code as it would help me understand the issue better ?

Sorry this became a very long message

[guillep]

I think the original issue meant to remove this behavior entirely

#17847

Probably this means analysing in if there is not a bug there, propose some tests, do a bit of redesign...

[Ducasse]

Guille you are right I forgot what I wanted.
@Alokzh yes I meant OCReturnNodeAdderVisitor because we were not happy with addReturnNode logic.

[Alokzh]

Guille you are right I forgot what I wanted. @Alokzh yes I meant OCReturnNodeAdderVisitor because we were not happy with addReturnNode logic.

So you want me to replace the adding node logic using OCReturnNodeAdderVisitor but I am bit confused here as this particular visitor just wraps the last statement in a return node ( correct me if I am wrong ) & it does not add a new node.

[Ducasse]

Sorry I was confusing.
Now we want to remove the only user ot AddNodeLast
Then this is another issue we want to see if we can externalise the node replacement into a specific visitor. This follows what we did in the OCReturnNodeAdderVisitor because before it was a addReturnNode message that was half way tested and unclear.

[Ducasse]

I'm dead coming back from a long trip so my brain is out of order. I will have to take time to reply to you.

[Alokzh]

Sorry I was confusing. Now we want to remove the only user ot AddNodeLast Then this is another issue we want to see if we can externalise the node replacement into a specific visitor. This follows what we did in the OCReturnNodeAdderVisitor because before it was a addReturnNode message that was half way tested and unclear.

Yeah I get this now.

[Alokzh]

I'm dead coming back from a long trip so my brain is out of order. I will have to take time to reply to you.

For sure @Ducasse I am okay with it.
Please take rest , meanwhile if I get some idea i'll let you know later on.