Skip to content

Commit 0149964

Browse files
cosminbascacosminbasca
committed
[GR-10739] Generator functions wrong closure setup
PullRequest: graalpython/105
2 parents bf5b153 + c00d9cd commit 0149964

File tree

11 files changed

+210
-68
lines changed

11 files changed

+210
-68
lines changed

‎graalpython/com.oracle.graal.python.test/src/tests/test_scope.py

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -852,6 +852,39 @@ def register():
852852
assert MyClass.a_counter == 24
853853

854854

855+
def test_generator_func_with_nested_nonlocals():
856+
def b_func():
857+
exec_gen = False
858+
859+
def _inner_func():
860+
def doit():
861+
nonlocal exec_gen
862+
exec_gen = True
863+
return [1]
864+
865+
assert set(doit.__code__.co_cellvars) == set()
866+
assert set(doit.__code__.co_freevars) == {'exec_gen'}
867+
for A in doit():
868+
for C in Y:
869+
yield A
870+
871+
assert set(_inner_func.__code__.co_cellvars) == set()
872+
assert set(_inner_func.__code__.co_freevars) == {'Y', 'exec_gen'}
873+
874+
gen = _inner_func()
875+
assert not exec_gen
876+
877+
Y = [1, 2]
878+
879+
list(gen)
880+
assert exec_gen
881+
return gen
882+
883+
assert set(b_func.__code__.co_cellvars) == {'Y', 'exec_gen'}
884+
assert set(b_func.__code__.co_freevars) == set()
885+
b_func()
886+
887+
855888
def test_generator_scope():
856889
my_obj = [1, 2, 3, 4]
857890
my_obj = (i for i in my_obj for j in y)

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/code/CodeBuiltins.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import com.oracle.graal.python.nodes.function.FunctionRootNode;
3737
import com.oracle.graal.python.nodes.function.PythonBuiltinBaseNode;
3838
import com.oracle.graal.python.nodes.function.PythonBuiltinNode;
39+
import com.oracle.graal.python.nodes.generator.GeneratorFunctionRootNode;
3940
import com.oracle.graal.python.runtime.PythonParseResult;
4041
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
4142
import com.oracle.truffle.api.dsl.GenerateNodeFactory;
@@ -60,6 +61,8 @@ protected Object doIt(PythonParseResult self) {
6061
RootNode rootNode = self.getRootNode();
6162
if (rootNode instanceof FunctionRootNode) {
6263
return factory().createTuple(((FunctionRootNode) rootNode).getFreeVars());
64+
} else if (rootNode instanceof GeneratorFunctionRootNode) {
65+
return factory().createTuple(((GeneratorFunctionRootNode) rootNode).getFreeVars());
6366
} else {
6467
return PNone.NONE;
6568
}
@@ -74,6 +77,8 @@ protected Object doIt(PythonParseResult self) {
7477
RootNode rootNode = self.getRootNode();
7578
if (rootNode instanceof FunctionRootNode) {
7679
return factory().createTuple(((FunctionRootNode) rootNode).getCellVars());
80+
} else if (rootNode instanceof GeneratorFunctionRootNode) {
81+
return factory().createTuple(((GeneratorFunctionRootNode) rootNode).getCellVars());
7782
} else {
7883
return PNone.NONE;
7984
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/function/PGeneratorFunction.java

Lines changed: 3 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -25,27 +25,24 @@
2525
*/
2626
package com.oracle.graal.python.builtins.objects.function;
2727

28-
import com.oracle.graal.python.PythonLanguage;
2928
import com.oracle.graal.python.builtins.objects.cell.PCell;
3029
import com.oracle.graal.python.builtins.objects.object.PythonObject;
3130
import com.oracle.graal.python.builtins.objects.type.PythonClass;
31+
import com.oracle.graal.python.nodes.generator.GeneratorFunctionRootNode;
3232
import com.oracle.graal.python.parser.ExecutionCellSlots;
3333
import com.oracle.graal.python.runtime.PythonCore;
34-
import com.oracle.graal.python.runtime.object.PythonObjectFactory;
3534
import com.oracle.truffle.api.RootCallTarget;
3635
import com.oracle.truffle.api.Truffle;
3736
import com.oracle.truffle.api.frame.FrameDescriptor;
38-
import com.oracle.truffle.api.frame.VirtualFrame;
39-
import com.oracle.truffle.api.nodes.RootNode;
4037

4138
public final class PGeneratorFunction extends PFunction {
4239

4340
public static PGeneratorFunction create(PythonClass clazz, PythonCore core, String name, String enclosingClassName, Arity arity, RootCallTarget callTarget,
44-
FrameDescriptor frameDescriptor, PythonObject globals, PCell[] closure, ExecutionCellSlots cellSlots,
41+
FrameDescriptor frameDescriptor, PythonObject globals, PCell[] closure, ExecutionCellSlots executionCellSlots,
4542
int numOfActiveFlags, int numOfGeneratorBlockNode, int numOfGeneratorForNode) {
4643

4744
GeneratorFunctionRootNode generatorFunctionRootNode = new GeneratorFunctionRootNode(core.getLanguage(), callTarget,
48-
frameDescriptor, closure, cellSlots, numOfActiveFlags, numOfGeneratorBlockNode, numOfGeneratorForNode);
45+
frameDescriptor, closure, executionCellSlots, numOfActiveFlags, numOfGeneratorBlockNode, numOfGeneratorForNode);
4946

5047
return new PGeneratorFunction(clazz, name, enclosingClassName, arity, Truffle.getRuntime().createCallTarget(generatorFunctionRootNode), frameDescriptor, globals, closure);
5148
}
@@ -64,33 +61,4 @@ public boolean isGeneratorFunction() {
6461
public PGeneratorFunction asGeneratorFunction() {
6562
return this;
6663
}
67-
68-
private final static class GeneratorFunctionRootNode extends RootNode {
69-
private final RootCallTarget callTarget;
70-
private final FrameDescriptor frameDescriptor;
71-
private final int numOfActiveFlags;
72-
private final int numOfGeneratorBlockNode;
73-
private final int numOfGeneratorForNode;
74-
private final PCell[] closure;
75-
private final ExecutionCellSlots cellSlots;
76-
77-
@Child private PythonObjectFactory factory = PythonObjectFactory.create();
78-
79-
protected GeneratorFunctionRootNode(PythonLanguage language, RootCallTarget callTarget, FrameDescriptor frameDescriptor, PCell[] closure, ExecutionCellSlots cellSlots,
80-
int numOfActiveFlags, int numOfGeneratorBlockNode, int numOfGeneratorForNode) {
81-
super(language);
82-
this.callTarget = callTarget;
83-
this.frameDescriptor = frameDescriptor;
84-
this.closure = closure;
85-
this.cellSlots = cellSlots;
86-
this.numOfActiveFlags = numOfActiveFlags;
87-
this.numOfGeneratorBlockNode = numOfGeneratorBlockNode;
88-
this.numOfGeneratorForNode = numOfGeneratorForNode;
89-
}
90-
91-
@Override
92-
public Object execute(VirtualFrame frame) {
93-
return factory.createGenerator(getName(), callTarget, frameDescriptor, frame.getArguments(), closure, cellSlots, numOfActiveFlags, numOfGeneratorBlockNode, numOfGeneratorForNode);
94-
}
95-
}
9664
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/PClosureFunctionRootNode.java

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
/*
2+
* Copyright (c) 2018, Oracle and/or its affiliates.
3+
*
4+
* The Universal Permissive License (UPL), Version 1.0
5+
*
6+
* Subject to the condition set forth below, permission is hereby granted to any
7+
* person obtaining a copy of this software, associated documentation and/or data
8+
* (collectively the "Software"), free of charge and under any and all copyright
9+
* rights in the Software, and any and all patent rights owned or freely
10+
* licensable by each licensor hereunder covering either (i) the unmodified
11+
* Software as contributed to or provided by such licensor, or (ii) the Larger
12+
* Works (as defined below), to deal in both
13+
*
14+
* (a) the Software, and
15+
* (b) any piece of software and/or hardware listed in the lrgrwrks.txt file if
16+
* one is included with the Software (each a "Larger Work" to which the
17+
* Software is contributed by such licensors),
18+
*
19+
* without restriction, including without limitation the rights to copy, create
20+
* derivative works of, display, perform, and distribute the Software and make,
21+
* use, sell, offer for sale, import, export, have made, and have sold the
22+
* Software and the Larger Work(s), and to sublicense the foregoing rights on
23+
* either these or other terms.
24+
*
25+
* This license is subject to the following condition:
26+
*
27+
* The above copyright notice and either this complete permission notice or at a
28+
* minimum a reference to the UPL must be included in all copies or substantial
29+
* portions of the Software.
30+
*
31+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
32+
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
33+
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
34+
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
35+
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
36+
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
37+
* SOFTWARE.
38+
*/
39+
package com.oracle.graal.python.nodes;
40+
41+
import com.oracle.graal.python.parser.ExecutionCellSlots;
42+
import com.oracle.truffle.api.CompilerDirectives;
43+
import com.oracle.truffle.api.TruffleLanguage;
44+
import com.oracle.truffle.api.frame.FrameDescriptor;
45+
import com.oracle.truffle.api.frame.FrameSlot;
46+
47+
public abstract class PClosureFunctionRootNode extends PClosureRootNode {
48+
@CompilerDirectives.CompilationFinal(dimensions = 1) protected final FrameSlot[] cellVarSlots;
49+
50+
protected PClosureFunctionRootNode(TruffleLanguage<?> language, FrameDescriptor frameDescriptor, ExecutionCellSlots executionCellSlots) {
51+
super(language, frameDescriptor, executionCellSlots.getFreeVarSlots());
52+
this.cellVarSlots = executionCellSlots.getCellVarSlots();
53+
}
54+
55+
public String[] getCellVars() {
56+
String[] cellVars = new String[cellVarSlots.length];
57+
for (int i = 0; i < cellVars.length; i++) {
58+
cellVars[i] = (String) cellVarSlots[i].getIdentifier();
59+
}
60+
return cellVars;
61+
}
62+
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/PClosureRootNode.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,4 +82,12 @@ protected void addClosureCellsToLocalsExploded(Frame frame, PCell[] closure) {
8282
frame.setObject(freeVarSlots[i], closure[i]);
8383
}
8484
}
85+
86+
public String[] getFreeVars() {
87+
String[] freeVars = new String[freeVarSlots.length];
88+
for (int i = 0; i < freeVarSlots.length; i++) {
89+
freeVars[i] = (String) freeVarSlots[i].getIdentifier();
90+
}
91+
return freeVars;
92+
}
8593
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/function/ExpressionDefinitionNode.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
package com.oracle.graal.python.nodes.function;
2727

2828
import com.oracle.graal.python.builtins.objects.cell.PCell;
29+
import com.oracle.graal.python.builtins.objects.function.PArguments;
2930
import com.oracle.graal.python.nodes.PNode;
3031
import com.oracle.graal.python.parser.DefinitionCellSlots;
3132
import com.oracle.graal.python.parser.ExecutionCellSlots;
@@ -61,4 +62,15 @@ PCell[] getClosureFromLocals(Frame frame) {
6162

6263
return closure;
6364
}
65+
66+
PCell[] getClosureFromGeneratorOrFunctionLocals(Frame frame) {
67+
PCell[] closure;
68+
Frame generatorFrame = PArguments.getGeneratorFrame(frame);
69+
if (generatorFrame != null) {
70+
closure = getClosureFromLocals(generatorFrame);
71+
} else {
72+
closure = getClosureFromLocals(frame);
73+
}
74+
return closure;
75+
}
6476
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/function/FunctionDefinitionNode.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public FunctionDefinitionNode(String functionName, String enclosingClassName, Py
6464
public Object execute(VirtualFrame frame) {
6565
defaults.executeVoid(frame);
6666

67-
PCell[] closure = getClosureFromLocals(frame);
67+
PCell[] closure = getClosureFromGeneratorOrFunctionLocals(frame);
6868
return factory().createFunction(functionName, enclosingClassName, arity, callTarget, frameDescriptor, PArguments.getGlobals(frame), closure);
6969
}
7070

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/function/FunctionRootNode.java

Lines changed: 3 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,11 @@
2828
import com.oracle.graal.python.PythonLanguage;
2929
import com.oracle.graal.python.builtins.objects.cell.PCell;
3030
import com.oracle.graal.python.builtins.objects.function.PArguments;
31-
import com.oracle.graal.python.nodes.PClosureRootNode;
31+
import com.oracle.graal.python.nodes.PClosureFunctionRootNode;
3232
import com.oracle.graal.python.nodes.PNode;
3333
import com.oracle.graal.python.nodes.cell.CellSupplier;
3434
import com.oracle.graal.python.parser.ExecutionCellSlots;
3535
import com.oracle.truffle.api.CompilerAsserts;
36-
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
3736
import com.oracle.truffle.api.frame.Frame;
3837
import com.oracle.truffle.api.frame.FrameDescriptor;
3938
import com.oracle.truffle.api.frame.FrameSlot;
@@ -46,11 +45,9 @@
4645
/**
4746
* RootNode of a Python Function body. It is invoked by a CallTarget.
4847
*/
49-
public class FunctionRootNode extends PClosureRootNode implements CellSupplier {
48+
public class FunctionRootNode extends PClosureFunctionRootNode implements CellSupplier {
5049

51-
@CompilationFinal(dimensions = 1) private final FrameSlot[] cellVarSlots;
5250
private final PCell[] cells;
53-
5451
private final ExecutionCellSlots executionCellSlots;
5552
private final String functionName;
5653
private final SourceSection sourceSection;
@@ -61,9 +58,8 @@ public class FunctionRootNode extends PClosureRootNode implements CellSupplier {
6158

6259
public FunctionRootNode(PythonLanguage language, SourceSection sourceSection, String functionName, boolean isGenerator, FrameDescriptor frameDescriptor, PNode body,
6360
ExecutionCellSlots executionCellSlots) {
64-
super(language, frameDescriptor, executionCellSlots.getFreeVarSlots());
61+
super(language, frameDescriptor, executionCellSlots);
6562
this.executionCellSlots = executionCellSlots;
66-
this.cellVarSlots = executionCellSlots.getCellVarSlots();
6763
this.cells = new PCell[this.cellVarSlots.length];
6864

6965
this.sourceSection = sourceSection;
@@ -91,22 +87,6 @@ public String getName() {
9187
return functionName;
9288
}
9389

94-
public String[] getCellVars() {
95-
String[] cellVars = new String[cellVarSlots.length];
96-
for (int i = 0; i < cellVars.length; i++) {
97-
cellVars[i] = (String) cellVarSlots[i].getIdentifier();
98-
}
99-
return cellVars;
100-
}
101-
102-
public String[] getFreeVars() {
103-
String[] freeVars = new String[freeVarSlots.length];
104-
for (int i = 0; i < freeVarSlots.length; i++) {
105-
freeVars[i] = (String) freeVarSlots[i].getIdentifier();
106-
}
107-
return freeVars;
108-
}
109-
11090
@Override
11191
public PCell[] getCells() {
11292
return cells;

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/function/GeneratorExpressionNode.java

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,6 @@
3333
import com.oracle.truffle.api.CompilerAsserts;
3434
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
3535
import com.oracle.truffle.api.RootCallTarget;
36-
import com.oracle.truffle.api.frame.Frame;
3736
import com.oracle.truffle.api.frame.FrameDescriptor;
3837
import com.oracle.truffle.api.frame.VirtualFrame;
3938
import com.oracle.truffle.api.nodes.RootNode;
@@ -130,13 +129,7 @@ public Object execute(VirtualFrame frame) {
130129
}
131130
PArguments.setGlobals(arguments, PArguments.getGlobals(frame));
132131

133-
PCell[] closure;
134-
Frame generatorFrame = PArguments.getGeneratorFrame(frame);
135-
if (generatorFrame != null) {
136-
closure = getClosureFromLocals(generatorFrame);
137-
} else {
138-
closure = getClosureFromLocals(frame);
139-
}
132+
PCell[] closure = getClosureFromGeneratorOrFunctionLocals(frame);
140133
return factory().createGenerator(name, callTarget, frameDescriptor, arguments, closure, executionCellSlots,
141134
numOfActiveFlags, numOfGeneratorBlockNode, numOfGeneratorForNode);
142135
}

0 commit comments

Comments
 (0)