Skip to content
This repository has been archived by the owner on May 31, 2020. It is now read-only.

Added support for Yield from statement #821

Merged
merged 6 commits into from
Jun 3, 2018
Merged

Added support for Yield from statement #821

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e0daee5
Added tests for yield_from
BPYap May 23, 2018
3a64b7b
Added opcode for yield_from
BPYap May 23, 2018
105e291
changed visit condition
BPYap May 24, 2018
271e476
Removed redundant test case
BPYap May 25, 2018
b3b013d
Code cleanup
BPYap May 25, 2018
2071e50
Added comment explaning condition check for manually defined yield node
BPYap Jun 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 37 additions & 13 deletions tests/structures/test_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,24 +72,48 @@ def fizz_buzz(self):
print(i)
""")

def test_yield_from_not_used(self):
"""Yield from is currently not implemented.
Unused yield from statements must not break
the program compilation, but only ensue a warning."""
def test_simplest_yieldfrom(self):
self.assertCodeExecution("""

def unused():
yield from range(5)
def gen1():
yield 1
yield 2
yield 3

def gen2():
yield from gen1()

for i in gen2():
print(i)
""")

print('Hello, world!')
""")
def test_yieldfrom_list(self):
self.assertCodeExecution("""
def gen():
yield from [1, 2, 3]

for i in gen():
print(i)
""")

def test_chainning_yieldfrom(self):
Copy link
Member

Choose a reason for hiding this comment

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

spelling: chaining

self.assertCodeExecution("""
def gen1():
yield 'a'
yield 'b'

def gen2():
yield from gen1()
yield 'gen2'

def gen3():
yield from gen2()

for i in gen3():
print(i)
""")

@expectedFailure
def test_yield_from_used(self):
Copy link
Member

Choose a reason for hiding this comment

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

This is one of those rare situations where a test can be removed. The test itself is a much simpler version of what you've tested thoroughly with your other yield from tests.

"""Yield from is currently not implemented.
Yield from statements become NotImplementedErrors at runtime."""
self.assertCodeExecution("""

def using_yieldfrom():
yield from range(5)

Expand Down
42 changes: 32 additions & 10 deletions voc/python/ast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1712,7 +1712,8 @@ def visit_GeneratorExp(self, node):

@node_visitor
def visit_Yield(self, node):
self.visit(node.value)
if hasattr(node, "lineno"):
self.visit(node.value)
Copy link
Member

Choose a reason for hiding this comment

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

It isn't clear to me why lineno is a relevant attribute here. Is there a type of AST node that doesn't have a linen that might be visited?

At the very least, a comment explaining the need for this branch is required; better still would be a test case that exercises this case (assuming there isn't one already)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is sort of a 'hackish' way to differentiate a programmatically defined yield node from those generated normally by ast.parse.

Combined with #823, there will be 3 types of yield node passed into visit_Yield method:

  1. Ordinary yield node generated from a line of code (eg yield x). In this case we need to visit x (the default implementation).
  2. Ordinary yield node generated from a line of code, but without value/operand (eg yield or x=yield). In this case we push a org.python.types.NoneType.NONE object on stack without visiting the none value.
  3. Programmatically defined yield node (in line 1778 self.visit(ast.Yield(None))) This node is created in visit_YieldFrom and the absent of lineno is use to indicate that a value has been pushed on stack hence there is no need to call self.visit(node.value)

Edit: I just remembered it is possible to add custom attribute to a class so I'm thinking of using a flag (i.e skip_visit) as custom attribute of the programmatically defined yield node instead of the lineno to increase clarity, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Ok - that reasoning makes sense; however, it isn't immediately obvious from the code. Adding a comment explaining what is going on would be helpful. A custom attribute isn't necessary; as long as the code explains what it's doing, that will be sufficient.

yield_point = len(self.context.yield_points) + 1
self.context.add_opcodes(
# Convert to a new value for return purposes
Expand Down Expand Up @@ -1747,18 +1748,39 @@ def visit_Yield(self, node):

@node_visitor
def visit_YieldFrom(self, node):
# expr value):
print(
'"yield from" statement not yet implemented. '
'Will throw a NotImplementedError at runtime'
self.visit(node.value) # pushes expression on stack
self.context.add_opcodes(
python.Object.iter() # pops expression from stack then gets and pushes its iterator on stack
)
self.context.store_name('#yield-iter-%x' % id(node))

loop = START_LOOP()
self.context.add_opcodes(
java.THROW(
'org/python/exceptions/NotImplementedError',
['Ljava/lang/String;',
JavaOpcodes.LDC_W('"yield from" statement not yet implemented')]
)
loop,
)
self.context.add_opcodes(
TRY(),
)
Copy link
Member

Choose a reason for hiding this comment

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

As a style thing, it's preferable to include as many opcodes as possible in a single add_opcodes call. The end result is exactly the same in terms of functionality, but there's less code to read, which can make the intent of the code easier to interpret for those coming in later.

self.context.load_name('#yield-iter-%x' % id(node))
self.context.add_opcodes(
python.Iterable.next(),
)
self.context.add_opcodes(
CATCH('org/python/exceptions/StopIteration'),
)
self.context.add_opcodes(
JavaOpcodes.POP(),
jump(JavaOpcodes.GOTO(0), self.context, loop, OpcodePosition.NEXT),
)
self.context.add_opcodes(
END_TRY(),
)
self.visit(ast.Yield(None))
self.context.add_opcodes(
END_LOOP(),
)

self.context.delete_name('#yield-iter-%x' % id(node))

@node_visitor
def visit_Compare(self, node):
Expand Down