New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support exceptions handling in generator #831

Merged
merged 39 commits into from Aug 11, 2018

Conversation

Projects
None yet
2 participants
@BPYap
Contributor

BPYap commented Jun 4, 2018

This PR include:

  • Allows yield to be used in try-finally suite
  • Allows return statement to be used in a generator
  • Implements Generator's throw method**
  • Implements Generator's close method
  • Implements Generator's __del__ method

**Currently working on parsing *args and **kwargs of throw method

@freakboy3742

This is looking really good. The test suite has great coverage, and I can't argue with the fact the tests pass, too :-)

A few comments inline; mostly about explaining what is going on for the benefit of the next person who has to read the code. Generator has some fairly complex internal state; making sure we're clear about exactly what that state is, and what it means, is an important part of this process.

args = {"type"},
default_args = {"value", "traceback"}
)
public org.python.Object _throw(org.python.Object type, org.python.Object value, org.python.Object traceback) {

This comment has been minimized.

@freakboy3742

freakboy3742 Jun 5, 2018

Member

Two points here:

  1. I remembered the naming trick that is used elsewhere in VOC: $ is a legal symbol in Java names, but not in Python. So, if you name this method throw$, it's a name that can't collide with a Java keyword, and can't collide with a user-provided name, either (throw_ would be a legal name in Python)
  2. @org.python.Method has an additional argument - name - to allow the java method name to differ from the symbol that is exposed in the Python namespace. If @org.python.Method specifies a name key, that is the name that will be exposed to the Python namespace, rather than the method's actual name.

This comment has been minimized.

@BPYap

BPYap Jun 5, 2018

Contributor

Resolved. Can't believe I missed that name attribute when browsing through voc documentation!
Now that there is already a mechanism to resolve name conflicts, shall I closed issue #827 ?

@@ -8,6 +11,7 @@
private boolean just_started = true;

This comment has been minimized.

@freakboy3742

freakboy3742 Jun 5, 2018

Member

A random thought looking at this code - is there any way just_started could be merged with yield_point? ISTM they serve a very similar purpose; possibly treating the initial value of yield_point as -1, set to 0 when the generator is started, and then to an integer value when an actual yield point is reached? I'm just wary of having two variables tracking very similar things.

This comment has been minimized.

@BPYap

BPYap Jun 5, 2018

Contributor

Resolved. It seems yield_point == 0 is sufficient to differentiate a just started generator and yield_point with any integer value indicates a already started generator.

try {
this.close();
} catch (Exception e) {
//TODO: log error message in java equivalent of sys.stderr

This comment has been minimized.

@freakboy3742

freakboy3742 Jun 5, 2018

Member

That would be System.err.println

This comment has been minimized.

@BPYap

BPYap Jun 5, 2018

Contributor

Resolved. Thanks! 😄

@@ -26,7 +30,12 @@ public Generator(
this.expression = expression;
this.yield_point = 0;
this.stack = stack;
this.message = new org.python.types.NoneType();
this.message = org.python.types.NoneType.NONE;

This comment has been minimized.

@freakboy3742

freakboy3742 Jun 5, 2018

Member

We're acquiring a lot of "state" variables in the Generator; I can see that they're all needed (with the possible exception of just_started), but it would be helpful to have some explanatory notes of what the values all mean in the generator context. (e.g., expression == null means the generator has exhausted itself). A class-level documentation block explaining all the possible "states" of a generator is all we'd need here.

This comment has been minimized.

@BPYap

BPYap Jun 5, 2018

Contributor

Resolved. Added a class-level documentation block explaining possible states of a generator.

try:
g.throw(ZeroDivisionError)
except ZeroDivisionError:
pass

This comment has been minimized.

@freakboy3742

freakboy3742 Jun 5, 2018

Member

The VOC test suite is based on comparing output, so having tests that "pass" and do nothing in a branch is potentially risky. Ideally, you want to print something that confirms this branch was actually executed (and was executed in the right order); you also want to have print statements in places that can't be reached (e.g., after the throw statement) to confirm that code that should be dead is actually dead.

This comment has been minimized.

@BPYap

BPYap Jun 5, 2018

Contributor

Resolved.

@@ -694,7 +712,7 @@ def visit_Try(self, node):
# Finally content is duplicated at the end of the body
# if it is defined.
if node.finalbody:
for child in node.finalbody:
for child in copy.deepcopy(node.finalbody):

This comment has been minimized.

@freakboy3742

freakboy3742 Jun 5, 2018

Member

Why the deep copy?

This comment has been minimized.

@BPYap

BPYap Jun 5, 2018

Contributor

Currently parse_yield from previous PR is modifying yield expression nodes in place, if I did not copy the list, subsequent visit to final body's childs (that contains yield expression nodes) will visit the transformed nodes and generate different opcodes.

I'm trying to refactor either parse_yield or come up with a way to copy generated opcodes from first visit since using deepcopy might not be efficient and optimal when the code is large.

This comment has been minimized.

@freakboy3742

freakboy3742 Jun 5, 2018

Member

I don't know if that's a likely outcome - the body of a yield has to be an expression, not a statement block, so there's a practical limit to how complex a yield body can be.

This comment has been minimized.

@BPYap

BPYap Jun 23, 2018

Contributor

@freakboy3742 I've refactored the code in latest commit so that I'm no longer modify/transform the nodes. However, deepcopy is still needed in the following lines:

node_to_resolve = copy.deepcopy(node)  # deepcopy is required for multiple visits of finalbody from visitTry
self.context.yield_points.append(node_to_resolve)
self.context.next_resolve_list.append((node_to_resolve, OpcodePosition.YIELD))

in order to resolve the next opcode correctly for duplicated final body as the code snippet below in blocks.py changed node's attribute in place:

# Resolve any references to the "next" opcode.
for (obj, attr) in self.next_resolve_list:
    # print("        resolve %s reference on %s %s with %s %s" % (
    #     attr, obj, id(obj), opcode, id(opcode))
    # )
    setattr(obj, attr.value, opcode)

If everything is fine then it is ready for merge 😃

)
)
# throw exception if there is one

This comment has been minimized.

@freakboy3742

freakboy3742 Jun 5, 2018

Member

The comment says "if there is one" - but this code is executed regardless. It would be worth noting (both here, and in a docstring in the Java code) that throw_exception is a no-op if the generator doesn't have an exception state.

This comment has been minimized.

@BPYap

BPYap Jun 5, 2018

Contributor

Resolved.

# The node is a regular yield statement
# Mark it as 'regular_yield' to flush the generator's message upon restored
if isinstance(node, ast.Yield):
setattr(node, "regular_yield", True)

This comment has been minimized.

@freakboy3742

freakboy3742 Jun 5, 2018

Member

What's the alternative to "regular" yield?

This comment has been minimized.

@BPYap

BPYap Jun 5, 2018

Contributor

Would is_yield_stament be better?

This comment has been minimized.

@freakboy3742

freakboy3742 Jun 9, 2018

Member

It's not (necessarily) a question of naming - I don't understand what a "regular yield" is. What's an "irregular" yield? How do you get an "irregular" yield?

This comment has been minimized.

@BPYap

BPYap Jun 9, 2018

Contributor

Initially I want to differentiate a yield statement (e.g. the plain old yield 123, which I called "regular yield") from the yield expression (x = yield 123).

I did some code refactoring just now and the attribute is no longer needed.

* 1. yield_point == 0 : The generator is 'fresh' i.e. (just started)
* 2. expression == null : The generator is exhausted
* 3. message != none : The generator currently holds value sent from caller via send() method
* 4. exception != null : The generator currently holds exception sent from caller via throw() method

This comment has been minimized.

@freakboy3742

freakboy3742 Jun 5, 2018

Member

Are any of these states mutually exclusive? e.g., can I have both a message and an exception?

This comment has been minimized.

@BPYap

BPYap Jun 8, 2018

Contributor

Yes every states are mutually exclusive

@BPYap BPYap changed the title from Support exceptions handling in generator [WIP] to Support exceptions handling in generator Jun 23, 2018

@freakboy3742

👍

@freakboy3742 freakboy3742 merged commit 18b06d5 into pybee:master Aug 11, 2018

5 checks passed

beekeeper:0/beefore:javacheckstyle Java lint checks passed.
Details
beekeeper:0/beefore:pycodestyle Python lint checks passed.
Details
beekeeper:1/smoke-test Smoke build (Python 3.4) passed.
Details
beekeeper:2/full-test:py3.5 Python 3.5 tests passed.
Details
beekeeper:2/full-test:py3.6 Python 3.6 tests passed.
Details

@BPYap BPYap deleted the BPYap:generator_throw_method branch Aug 12, 2018

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