Skip to content

Compound assignment#421

Open
Viriathus1 wants to merge 3 commits intospylang:mainfrom
Viriathus1:compound-assignment
Open

Compound assignment#421
Viriathus1 wants to merge 3 commits intospylang:mainfrom
Viriathus1:compound-assignment

Conversation

@Viriathus1
Copy link
Copy Markdown
Contributor

Closes #177

This adds compound assignment support for attributes and subscripts.

Copy link
Copy Markdown
Member

@antocuni antocuni left a comment

Choose a reason for hiding this comment

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

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


def test_aug_assign_subscript(self):
mod = self.compile("""
from _list import list
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's better to use __spy__.interp_list here.
I'd like to avoid test_basic to depend on _list.spy, because that needs "the whole spy" to work, and thus it defeats a bit the utility of having a "test_basic"

@Viriathus1
Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement += &co. for complex expressions

2 participants