Fixes for closure in comprehensions and tracing of lambda functions #245

MatthieuDartiailh · 2017-11-30T20:08:41Z

Wrap functions defined inside operators or declarative function to call them with their scope of definition. This allows to handle properly comprehensions and lambdas (whose body is now properly traced). Also ensure that we compile the body of the :: operator as a function to properly handle closure.

Bump the compiler version to 26

I added tests for all the new behaviors

MatthieuDartiailh · 2017-11-30T21:37:09Z

@sccolbert in theory this could allow to define function in declarative functions and event handler but I do not see a compelling use case to remove this restriction.

codecov-io · 2017-11-30T21:40:45Z

Codecov Report

Merging #245 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #245      +/-   ##
==========================================
+ Coverage   63.93%   63.95%   +0.02%     
==========================================
  Files         287      287              
  Lines       23356    23372      +16     
==========================================
+ Hits        14932    14948      +16     
  Misses       8424     8424

Impacted Files	Coverage Δ
enaml/core/code_tracing.py	`82.35% <ø> (ø)`	⬆️
enaml/core/enaml_compiler.py	`97.29% <100%> (ø)`	⬆️
enaml/core/parser/base_parser.py	`72.06% <100%> (+0.1%)`	⬆️
enaml/core/compiler_common.py	`93.55% <100%> (+0.03%)`	⬆️
enaml/core/compiler_helpers.py	`84.84% <100%> (+0.35%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ed8a29...6c78bf0. Read the comment docs.

frmdstryr · 2017-11-30T22:06:56Z

For callbacks to async functions it would be nice.

MatthieuDartiailh · 2017-11-30T22:07:53Z

@frmdstryr could you test this code ?

frmdstryr · 2017-11-30T22:13:03Z

Tests are passing in 2.7 and 3.5 for me.

frmdstryr · 2017-11-30T22:33:56Z

Also undid my previous workarounds for the listcomp and local var issue and both are working 👍

MatthieuDartiailh · 2017-12-04T07:00:34Z

@sccolbert this is the last pending PR that needs to be in before the next bugfix release that I plan mid-december. It would be terrific if you could give it a look.

sccolbert · 2017-12-04T14:48:53Z

enaml/core/compiler_helpers.py

@@ -755,5 +778,5 @@ def add_decl_function(node, func, is_override):
    'validate_spec': validate_spec,
    'validate_template': validate_template,
    'validate_unpack_size': validate_unpack_size,
-    'call_func': call_func,


there's no need to remove this, just add the new helper.

I removed it because I was the one to add it in the first place when adding first support for comprehensions.

sccolbert · 2017-12-04T14:49:12Z

enaml/core/compiler_helpers.py

+    def wrapper(*args, **kwargs):
+        return call_func(func, args, kwargs, scope)
+
+    update_wrapper(wrapper, func)


why this over @functools.wraps?

No reason I will use functools.wraps

Actually it is because wraps expects the decorator to take as single argument the function.

sccolbert · 2017-12-04T15:01:30Z

enaml/core/parser/base_parser.py

+
+        mod = ast.Module(body=[func_node])
+        ast.fix_missing_locations(mod)
+


I don't think this is correct. Can you give an example of the closure variables you're hoping to pick up with this change?

Also, I don't think this is going to work as you expect. When the :: operator is run to generate the expression handler, it wraps the generated bytecode in it's own function, which is then invoked by the handler: https://github.com/nucleic/enaml/blob/master/enaml/core/operators.py#L70 So each time the notification is triggered, this new internal function is going to be defined, instead of executed. I'm not entirely sure how this is passing tests.

If you look at the compilation of the ast for the operator you will notice that I extract the body of the function that I then pass onward. I simply chose to keep the ast manipulation located in the parser, but I could do the wrapping in the compiler.

As far as closure are concerned the point is that if one compile the following:

a = 1 [a for i in range (2)]

In a top level module a is accessed using LOAD_GLOBAL which fails. By wrappring the expression in a function, a closure is used. The point is that we need either to directly compile the right function or simply extract its code which is what we currently do.

But your example already works without this change, because all LOAD_GLOBALS are re-written to LOAD_NAME.

Can you give me an example of something that doesn't work currently, that this change fixes? Also, can you link me to where you "extract the body of the function and pass it forward"?

Ah, okay, you mean that doesn't work on Python 3. Sorry.

We just need to recursively rewrite LOAD_GLOBAL to LOAD_NAME for nested code objects.

It is not so simple because LOAD_NAME fails to access fast locals of the outer scope. I will answer in more details later. I am attending a conference this week.

I don't think there's anything in fast locals that the inner code will need to access.

Still looking at this kind of comprehension:

a = 1 [a for i in range (2)]

Digging deeper I found that:

when Python compile the code as if it was a top-level module (which it does without this fix), it uses STORE_NAME and LOAD_GLOBAL to handle a

to unify the handling of comprehensions between operators and declarative functions I refactored the handling of global variables such that as LOAD_GLOBAL and STORE_GLOBAL are replaced by LOAD_NAME/STORE_NAME save if the variable is marked using the global keyword, both in declarative functions and operators. Currently this is not done if there is no comprehension in an operator which is inconsistent. cf https://github.com/nucleic/enaml/blob/master/enaml/core/compiler_common.py#L685

finally when creating the operator (https://github.com/nucleic/enaml/blob/master/enaml/core/operators.py#L156), we optimize the code for locals (https://github.com/nucleic/enaml/blob/master/enaml/core/operators.py#L49) which replaces LOAD_NAME/STORE_NAME for local variables by LOAD_FAST/STORE_FAST.

As a consequence a is not stored in the namespace, which explains the failure.

From my point of view the proposed fixed is the more correct since the body of a notification operator is truly a function body (even without return and yield). ~~Alternatively we could suppress the global variable analysis in operators~~. Alternatively we can suppress the locals optimization in gen_simple.

I will update the code to always analyse global variables in operators for the time being, so that the current behavior is consistent.

MatthieuDartiailh · 2017-12-04T22:14:25Z

I will try to answer all your concerns point by point.

First concerning the extraction of the code in the compiler the relevant change is https://github.com/MatthieuDartiailh/enaml/blob/b779e5ae62a0cda8e90e85874c1a33bff23ed337/enaml/core/compiler_common.py#L683. As we know that the code has been wrapped in a function we find its code object and pass it along. As mentioned, I can move this part back into the compiler_common if you want.

Concerning your other concerns, I created a small gist testing just the problematic cases https://gist.github.com/MatthieuDartiailh/578b6c7c13b65f966a7cab8c41e7f300. You can look at the bytecode by adding the following at https://github.com/MatthieuDartiailh/enaml/blob/b779e5ae62a0cda8e90e85874c1a33bff23ed337/enaml/core/compiler_common.py#L697.

for op, op_arg in bp.Code.from_code(code).code:
        print(op, op_arg)
        if isinstance(op_arg, bp.Code):
            for op2, op_arg2 in op_arg.code:
                print('   ', op2, op_arg2)

I inspected the bytecode both on master (where the test case using the 'test' value) and on this branch. The bytecode on master uses STORE_NAME and LOAD_NAME but fails and to me this is a mystery. On my branch we use a closure and LOAD_DEREF and it works.
Adding locals() just before the comprehension (after test=1) fix the situation on master. Again this does not make sense to me. If you have any suggestions on how to investigate further I am interested.
The last inline post explains the observed behavior.

MatthieuDartiailh · 2017-12-09T10:59:07Z

I believe I have answered all your questions in my last two posts @sccolbert, could you see if that answers your concerns.

MatthieuDartiailh · 2017-12-13T14:40:01Z

ping @sccolbert
I guess you are fairly busy but do you think you could review this so that we get the bugfix release out before Christmas.

MatthieuDartiailh · 2018-01-21T10:19:26Z

ping @sccolbert
I would like to make the next release at the beginning of February and this PR is really needed.

sccolbert · 2018-01-24T16:22:01Z

enaml/core/compiler_common.py

+        code = compile(node.value.ast, cg.filename, mode=mode)
+        for op, op_arg in bp.Code.from_code(code).code:
+            if isinstance(op_arg, bp.Code):
+                rewrite_globals_access(op_arg, global_vars)


Is this duplicating logic from line 697 below?

Looks like it is indeed. I will fix it.

All point addressed and agreement on hangout

Wrap functions defined inside operators or declarative function to call them with their scope of definition. This allows to handle properly comprehensions and lambdas (whose body is now properly traced). Also ensure that we compile the body of the :: operator as a function to properly handle closure. Bump the compiler version to 26

sccolbert reviewed Dec 4, 2017

View reviewed changes

sccolbert previously requested changes Dec 4, 2017

View reviewed changes

MatthieuDartiailh force-pushed the function_wrapping branch from 25a2a59 to d77804b Compare December 9, 2017 16:11

sccolbert reviewed Jan 24, 2018

View reviewed changes

MatthieuDartiailh added 4 commits January 24, 2018 21:49

core: fix global access

b1b817b

core: homogeneous handling of global variables in operators

a77d445

core: remove duplicate logic in code generation of operators

6c78bf0

MatthieuDartiailh force-pushed the function_wrapping branch from 3550ad8 to 6c78bf0 Compare January 24, 2018 20:49

MatthieuDartiailh merged commit efa4a61 into nucleic:master Jan 24, 2018

MatthieuDartiailh deleted the function_wrapping branch January 24, 2018 20:59

frmdstryr mentioned this pull request Jan 24, 2018

Add themes, autocomplete, markers, indicators, and show_line_numbers to scintilla #243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes for closure in comprehensions and tracing of lambda functions #245

Fixes for closure in comprehensions and tracing of lambda functions #245

MatthieuDartiailh commented Nov 30, 2017

MatthieuDartiailh commented Nov 30, 2017

codecov-io commented Nov 30, 2017 •

edited

Loading

frmdstryr commented Nov 30, 2017

MatthieuDartiailh commented Nov 30, 2017 •

edited

Loading

frmdstryr commented Nov 30, 2017

frmdstryr commented Nov 30, 2017

MatthieuDartiailh commented Dec 4, 2017

sccolbert Dec 4, 2017

MatthieuDartiailh Dec 9, 2017

sccolbert Jan 24, 2018

sccolbert Dec 4, 2017

MatthieuDartiailh Dec 9, 2017

MatthieuDartiailh Dec 9, 2017

sccolbert Dec 4, 2017 •

edited

Loading

MatthieuDartiailh Dec 4, 2017

sccolbert Dec 4, 2017

sccolbert Dec 4, 2017

sccolbert Dec 4, 2017

MatthieuDartiailh Dec 4, 2017

sccolbert Dec 4, 2017

MatthieuDartiailh Dec 9, 2017 •

edited

Loading

MatthieuDartiailh commented Dec 4, 2017 •

edited

Loading

MatthieuDartiailh commented Dec 9, 2017

MatthieuDartiailh commented Dec 13, 2017

MatthieuDartiailh commented Jan 21, 2018

sccolbert Jan 24, 2018

MatthieuDartiailh Jan 24, 2018


		mod = ast.Module(body=[func_node])
		ast.fix_missing_locations(mod)

Fixes for closure in comprehensions and tracing of lambda functions #245

Fixes for closure in comprehensions and tracing of lambda functions #245

Conversation

MatthieuDartiailh commented Nov 30, 2017

MatthieuDartiailh commented Nov 30, 2017

codecov-io commented Nov 30, 2017 • edited Loading

Codecov Report

frmdstryr commented Nov 30, 2017

MatthieuDartiailh commented Nov 30, 2017 • edited Loading

frmdstryr commented Nov 30, 2017

frmdstryr commented Nov 30, 2017

MatthieuDartiailh commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sccolbert Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthieuDartiailh Dec 9, 2017 • edited Loading

Choose a reason for hiding this comment

MatthieuDartiailh commented Dec 4, 2017 • edited Loading

MatthieuDartiailh commented Dec 9, 2017

MatthieuDartiailh commented Dec 13, 2017

MatthieuDartiailh commented Jan 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 30, 2017 •

edited

Loading

MatthieuDartiailh commented Nov 30, 2017 •

edited

Loading

sccolbert Dec 4, 2017 •

edited

Loading

MatthieuDartiailh Dec 9, 2017 •

edited

Loading

MatthieuDartiailh commented Dec 4, 2017 •

edited

Loading