Compound assignment - #421
Conversation
There was a problem hiding this comment.
thank you!
The overall approach works, but I'm not 100% sure that doing the work directly in the parser is the right thing to do.
See e.g. this comment:
#435 (comment)
One possible "more clean" solution is to have proper "AugSetItem" and "AugSetAttr" nodes, and the we do desugaring in astframe as we do for for loops and exising AugAssign. What do you think?
Moreover, we need to make sure that the index is evaluated only once.
E.g.:
def foo() -> int:
print('foo')
return 0
x[foo()] += 1
needs to print foo only once, but I think that with your change it would print it twice
|
Thanks for the review, the approach just seemed like the easiest solution but I do see that it breaks away from desugaring in the AST frame and the other issues you mentioned. I'll work on the cleaner solution when I can. |
|
Hi @antocuni this requires another review. I've pushed the desugaring into the astframe, let me know if anything caught your eye. |
antocuni
left a comment
There was a problem hiding this comment.
Hi @Viriathus1 and sorry for the late review (the last month has been crazy).
I like this approach much better than the earlier one 🎉.
The part which I'm not sure about is using AugSetAttr.target_name() to get a temporary variable.
We already do something similar for for loops: in that case, the parser assigns an unique seq ID, which is then used to generate the temp variable for the iterator.
I think we could use the same approach for Aug assign, what do you think?
Another small note about desugaring: for other cases we do desugaring on the first execution and then we cache the result in e.g. ASTFrame._desugared_fors, and then the doppler can just reuse it without having to re-desugar.
Is there any particular reason why you didn't do that?
|
@Viriathus1 have a look at PR #649. |
afb5a64 to
429c757
Compare
antocuni
left a comment
There was a problem hiding this comment.
Thank you @Viriathus1 .
I like this version much better :).
I noticed that you added a test in test_cli.py to check the output of astcompile: that's good but test_cli was not really the right place.
I realized that we should have proper astcompile tests, so I added them in #651 , rebased your PR on top of it and added "proper" tests for AugSetItem and AugSetAttr, I hope you don't mind.
Will merge it as soon as it's green! :)
Closes #177
This adds compound assignment support for attributes and subscripts.