Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2991853002:
Fix duplicate context creation when closures appear in initializers. (Closed)
|
Created:
3 years, 4 months ago by sjindel Modified:
3 years, 4 months ago Reviewers:
Dmitry Stefantsov CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFix duplicate context creation when closures appear in initializers.
Summary:
Previously, when a function parameter was captured both in an initializer and in
the body of a constructor, we would create two contexts: one created in a local
initializer and used by other initializers, and another in the body of the
function. This is incorrect, as it means that changes to the parameter in the
initializer's closure won't be visible in the body.
Now, to work around this problem we re-use the context created for the
initializers in the body of the constructor by moving the body into a new
constructor, and redirecting the original constructor to that one, passing the
context as an additional argument. This dance is necessary because local
initializers aren't visible in the body of a constructor.
Test Plan:
A few of the existing closure conversion tests were changed or fixed by this
revision. We also modify the 'closure_in_initializer.dart' test to hit this case
directly.
R=dmitryas@google.com
Reviewers: dmitryas@google.com
Committed: https://github.com/dart-lang/sdk/commit/454ab91576ffd071b2e6a9b54f67453f33b8ee21
Patch Set 1 #
Total comments: 22
Patch Set 2 : Review comments: don't use the redirecting technique unless necessary. #
Total comments: 25
Patch Set 3 : Review comments. #
Total comments: 16
Patch Set 4 : Review comments. #
Messages
Total messages: 11 (2 generated)
sjindel@google.com changed reviewers: + dmitryas@google.com
I have some questions. Please, find them below. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... File pkg/kernel/lib/transformations/closure/converter.dart (right): https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:235: for (var i = 1; i < node.initializers.length; ++i) I guess here we make some assumptions about the structure of node.initializers, that is, that the first initializer is different from the others. Can we make it more explicit in the code? So that if something changes in the structure of the initializers above, the code here won't be broken. At the very least, a comment is required. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:251: assert(function.typeParameters.length == 0); I think we can't supply additional type arguments to constructors, only those that are specified at the class level. So, I think function.typeParameters should be empty anyway. But we may need additional clarification for that. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:267: (new CloneVisitor()).visitVariableDeclaration(contextDecl); Can we use [cv] from above here? https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:272: var contextInit = node.initializers[0] as LocalInitializer; Same note about treating 0-th initializer as special from above applies here. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:445: [Context reuse = null]) { Why do we need this optional parameter here? https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:479: // constants, which excludes closures. I would suggest rewriting this comment, so that it is not a TODO-comment. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:480: if (context is NoContext) { Why do we need this check here? https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/testcases/closur... File pkg/kernel/testcases/closures/closure_in_constructor.dart.expect (right): https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/testcases/closur... pkg/kernel/testcases/closures/closure_in_constructor.dart.expect:11: class C2 extends core::Object { If I'm getting this right, here is the Dart code for this class: ``` class C2 { var x; C2(y) { x = () => print('Hello $y'); } } ``` My understanding is that we shouldn't apply this technique with redirecting constructors here, because there are no initializers. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/testcases/closur... File pkg/kernel/testcases/closures/closure_in_initializer_closure.dart.expect (right): https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/testcases/closur... pkg/kernel/testcases/closures/closure_in_initializer_closure.dart.expect:5: class C extends core::Object { I believe, this is the corresponding Dart code: ``` C.foo(f) : t = (() { var prefix; var g = (x) { f("$prefix$x"); }; prefix = 'hest'; return g; }) { print(1); } ``` Here the captured variable is not used in the body of the constructor, and there's no need for the redirecting technique here.
https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... File pkg/kernel/lib/transformations/closure/converter.dart (right): https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:235: for (var i = 1; i < node.initializers.length; ++i) On 2017/07/28 12:36:16, Dmitry Stefantsov wrote: > I guess here we make some assumptions about the structure of node.initializers, > that is, that the first initializer is different from the others. Can we make > it more explicit in the code? So that if something changes in the structure of > the initializers above, the code here won't be broken. At the very least, a > comment is required. I rewrote it to look for the context's initializer specifically. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:251: assert(function.typeParameters.length == 0); On 2017/07/28 12:36:15, Dmitry Stefantsov wrote: > I think we can't supply additional type arguments to constructors, only those > that are specified at the class level. So, I think function.typeParameters > should be empty anyway. But we may need additional clarification for that. The comment above the [Constructor] class confirms this: /// Constructors do not take type parameters. Type arguments from a constructor /// invocation should be matched with the type parameters declared in the class. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:267: (new CloneVisitor()).visitVariableDeclaration(contextDecl); On 2017/07/28 12:36:16, Dmitry Stefantsov wrote: > Can we use [cv] from above here? Done. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:272: var contextInit = node.initializers[0] as LocalInitializer; On 2017/07/28 12:36:16, Dmitry Stefantsov wrote: > Same note about treating 0-th initializer as special from above applies here. Done. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:445: [Context reuse = null]) { On 2017/07/28 12:36:16, Dmitry Stefantsov wrote: > Why do we need this optional parameter here? So we don't reset the function body's context when an initializer has already been set up to hold the context. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:479: // constants, which excludes closures. On 2017/07/28 12:36:15, Dmitry Stefantsov wrote: > I would suggest rewriting this comment, so that it is not a TODO-comment. Done. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:480: if (context is NoContext) { On 2017/07/28 12:36:15, Dmitry Stefantsov wrote: > Why do we need this check here? We don't want to re-visit the function's parameters if they were already handled in "visitConstructor". https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/testcases/closur... File pkg/kernel/testcases/closures/closure_in_constructor.dart.expect (right): https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/testcases/closur... pkg/kernel/testcases/closures/closure_in_constructor.dart.expect:11: class C2 extends core::Object { On 2017/07/28 12:36:16, Dmitry Stefantsov wrote: > If I'm getting this right, here is the Dart code for this class: > > ``` > class C2 { > var x; > C2(y) { > x = () => print('Hello $y'); > } > } > ``` > > My understanding is that we shouldn't apply this technique with redirecting > constructors here, because there are no initializers. Done. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/testcases/closur... File pkg/kernel/testcases/closures/closure_in_initializer_closure.dart.expect (right): https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/testcases/closur... pkg/kernel/testcases/closures/closure_in_initializer_closure.dart.expect:5: class C extends core::Object { On 2017/07/28 12:36:16, Dmitry Stefantsov wrote: > I believe, this is the corresponding Dart code: > > ``` > C.foo(f) > : t = (() { > var prefix; > var g = (x) { > f("$prefix$x"); > }; > prefix = 'hest'; > return g; > }) { > print(1); > } > ``` > > Here the captured variable is not used in the body of the constructor, and > there's no need for the redirecting technique here. Done.
Great to see the situation with closures in initializers improving. I have some questions about the changes though. Please, find them below. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... File pkg/kernel/lib/transformations/closure/converter.dart (right): https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:445: [Context reuse = null]) { On 2017/07/31 12:06:20, sjindel wrote: > On 2017/07/28 12:36:16, Dmitry Stefantsov wrote: > > Why do we need this optional parameter here? > > So we don't reset the function body's context when an initializer has already > been set up to hold the context. I asked this question because [reuse] wasn't mentioned anywhere in the method body. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:480: if (context is NoContext) { On 2017/07/31 12:06:20, sjindel wrote: > On 2017/07/28 12:36:15, Dmitry Stefantsov wrote: > > Why do we need this check here? > > We don't want to re-visit the function's parameters if they were already handled > in "visitConstructor". Yes, I meant that if this check makes distinction between two cases, they should be more clearly and descriptively separated. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/closure/converter.dart (right): https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:73: final Map<VariableDeclaration, int /*capture flags -- see info.dart*/ > Please, add put this comment on its own line above the variable declaration. Also, it would be nice to have a small note about how the flag values are interpreted by the converter. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:186: extendContextConditionally(bool inInitializer) => I would suggest to make [inInitializer] a named parameter. Having it a positional parameter makes invocations like `extendContextConditionally(true)` read as `true` is the condition on which we extend the context. I think it would be less ambiguous to write `extendContextConditionally(inInitializer: true)` implying that the condition is determined somewhere else. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:186: extendContextConditionally(bool inInitializer) => As Karl pointed out, the arrow notation is usually used when the expression can be put on the same line. Personally, I don't mind this, but I think it would be nice to have the same style throughout the code. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:188: var captured = capturedVariables[parameter]; I think using type annotation `int` here helps with readability. We know that we expect a flag here. Types are nullable in Dart, so using `int` is perfectly fine. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:188: var captured = capturedVariables[parameter]; Also I think that the name [captured] is a bit misleading. We don't capture the flags; the flags describe some conditions on the captured variable. How about the name "capturingFlags" or just "flags" here? https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:255: for (var init in node.initializers) { This version is definitely better. I was thinking about keeping the context declaration and other initializers in separate places from the beginning, with descriptive names. This way we may avoid the loop here. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:457: context = new NoContext(this); Why was [context] initialization moved here? https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:470: void setupContextForFunctionBody(FunctionNode function) { Now this function name is misleading, after actual context initialization was moved somewhere else. Please, consider refactoring or renaming. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/closure/info.dart (right): https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/info.dart:34: final Map<VariableDeclaration, int /*captured flags*/ > variables = I think a more elaborate comment about the flags, how they are set, and how they are used later is better than the in-line comment. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/info.dart:127: node.function.accept(this); To this point, we've already visited [typeParameters], [positionalParameters], and [namedParameters]. Now we may just visit [returnType] and [body]. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/info.dart:203: variables[node.variable] = variables[node.variable] | captureFlags; How about using `|=` operation here and below?
https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... File pkg/kernel/lib/transformations/closure/converter.dart (right): https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:445: [Context reuse = null]) { On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > On 2017/07/31 12:06:20, sjindel wrote: > > On 2017/07/28 12:36:16, Dmitry Stefantsov wrote: > > > Why do we need this optional parameter here? > > > > So we don't reset the function body's context when an initializer has already > > been set up to hold the context. > > I asked this question because [reuse] wasn't mentioned anywhere in the method > body. I see, I suppose it wasn't actually needed then. https://codereview.chromium.org/2991853002/diff/1/pkg/kernel/lib/transformati... pkg/kernel/lib/transformations/closure/converter.dart:480: if (context is NoContext) { On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > On 2017/07/31 12:06:20, sjindel wrote: > > On 2017/07/28 12:36:15, Dmitry Stefantsov wrote: > > > Why do we need this check here? > > > > We don't want to re-visit the function's parameters if they were already > handled > > in "visitConstructor". > > Yes, I meant that if this check makes distinction between two cases, they should > be more clearly and descriptively separated. The case is removed. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/closure/converter.dart (right): https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:73: final Map<VariableDeclaration, int /*capture flags -- see info.dart*/ > On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > Please, add put this comment on its own line above the variable declaration. > Also, it would be nice to have a small note about how the flag values are > interpreted by the converter. Done. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:186: extendContextConditionally(bool inInitializer) => On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > As Karl pointed out, the arrow notation is usually used when the expression can > be put on the same line. Personally, I don't mind this, but I think it would be > nice to have the same style throughout the code. Done. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:186: extendContextConditionally(bool inInitializer) => On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > I would suggest to make [inInitializer] a named parameter. Having it a > positional parameter makes invocations like `extendContextConditionally(true)` > read as `true` is the condition on which we extend the context. I think it > would be less ambiguous to write `extendContextConditionally(inInitializer: > true)` implying that the condition is determined somewhere else. Done. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:188: var captured = capturedVariables[parameter]; On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > Also I think that the name [captured] is a bit misleading. We don't capture the > flags; the flags describe some conditions on the captured variable. How about > the name "capturingFlags" or just "flags" here? Done. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:188: var captured = capturedVariables[parameter]; On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > I think using type annotation `int` here helps with readability. We know that > we expect a flag here. Types are nullable in Dart, so using `int` is perfectly > fine. Done. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:255: for (var init in node.initializers) { On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > This version is definitely better. I was thinking about keeping the context > declaration and other initializers in separate places from the beginning, with > descriptive names. This way we may avoid the loop here. True, but I feel that this approach keeps the logic associated with this particular transformation mostly isolated in this function. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:457: context = new NoContext(this); On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > Why was [context] initialization moved here? It didn't really make sense to do it in the function "setupContextForFunctionBody" (now called "setupRewriterForFunctionBody") because I needed to execute this line conditionally at the other call side, and there are only two callers. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:470: void setupContextForFunctionBody(FunctionNode function) { On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > Now this function name is misleading, after actual context initialization was > moved somewhere else. Please, consider refactoring or renaming. Done. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/closure/info.dart (right): https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/info.dart:34: final Map<VariableDeclaration, int /*captured flags*/ > variables = On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > I think a more elaborate comment about the flags, how they are set, and how they > are used later is better than the in-line comment. Done. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/info.dart:127: node.function.accept(this); On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > To this point, we've already visited [typeParameters], [positionalParameters], > and [namedParameters]. Now we may just visit [returnType] and [body]. Yes, but I wrote it this way so if a new field is added to FunctionNode and FunctionNode::visitChildren, we won't ignore it. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/info.dart:203: variables[node.variable] = variables[node.variable] | captureFlags; On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > How about using `|=` operation here and below? Done.
I still have some questions :) Please, note that there are some new comments from me in different Patch Sets, not just the last one. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/closure/converter.dart (right): https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:255: for (var init in node.initializers) { On 2017/07/31 15:32:16, sjindel wrote: > On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > > This version is definitely better. I was thinking about keeping the context > > declaration and other initializers in separate places from the beginning, with > > descriptive names. This way we may avoid the loop here. > > True, but I feel that this approach keeps the logic associated with this > particular transformation mostly isolated in this function. But it's not completely isolated. I think that subclassing [AstRewriter] is a better way to do that. This way we would save some time for guessing to the future reader. Also, writing a sublclass of [AstRewriter] for redirecting constructor makes sense, because the redirecting technique may be seen as the special case for creating contexts, and this is exactly why we have the already existing subclasses of [AstRewriter], that is, to treat different cases for context creation differently. However, feel free to leave the code as it is. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:457: context = new NoContext(this); On 2017/07/31 15:32:16, sjindel wrote: > On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > > Why was [context] initialization moved here? > > It didn't really make sense to do it in the function > "setupContextForFunctionBody" (now called "setupRewriterForFunctionBody") > because I needed to execute this line > conditionally at the other call side, and there are only two callers. If we had a special kind of rewriter for the case when we need the redirecting technique, we may have used it in, for example, a factory of [Context] like this: ``` // In [setupContextForFunctionBody], converter.dart. context = new Context.emptyContextForConverter(this); ``` ``` // In [Context], context.dart. factory Context.emptyContextForConverter(Converter converter) { if (converter.rewriter is RedirectingRewriter) { // In case of [RedirectingRewriter] the new context chain was // created while visiting the constructor that makes the redirection. return converter.context; } return new NoContext(converter); } ``` That would save some guessing about some special context treatment. Feel free to leave the code as it is though. https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/closure/info.dart (right): https://codereview.chromium.org/2991853002/diff/20001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/info.dart:127: node.function.accept(this); On 2017/07/31 15:32:16, sjindel wrote: > On 2017/07/31 15:05:35, Dmitry Stefantsov wrote: > > To this point, we've already visited [typeParameters], [positionalParameters], > > and [namedParameters]. Now we may just visit [returnType] and [body]. > > Yes, but I wrote it this way so if a new field is added to FunctionNode and > FunctionNode::visitChildren, we won't ignore it. I guess it's ok to leave it like this, but consider all those AST visitors that need specific order of visiting the nodes. They all should be updated when new fields are added to the specially treated nodes. https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/closure/converter.dart (right): https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:75: // they are captured inside or outside an initializer. Please, add a reference to the field in [ClosureInfo] with the explanation of the flags values. https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:200: : flags == ClosureInfo.OUTSIDE_INITIALIZER) { How about breaking this condition into many, for example, this way: ``` if (flags == null) { // The variable is not captured. return 0; } if (inInitializer && (flags & ClosureInfo.INSIDE_INITIALIZER) == 0) { // The variable is not captured in an initializer, so we don't need // to extend the context in initializers with it. return flags; } if (!inInitializer && flags != ClosureInfo.OUTSIDE_INITIALIZER) { // The variable wasn't used in the body of a constructor or a function, // so we don't need to extend the context in a body with it. return flags; } context.extend(parameter, new VariableGet(parameter)); return flags; ``` Feel free to leave the code as it is though. https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:225: int capturedBoth = I think using `final` modifier makes sense for this variable. https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:267: node.initializers = node.initializers.take(1).toList(); Here we still treat the first initializer as special, although we now have it in a local variable with a descriptive name. How about doing the following here? ``` node.initializers = <Initializer>[contextDeclInit]; ``` https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:280: assert(function.typeParameters.length == 0); If we are now sure that the [FunctionNode] of the constructor may not have its own type parameters, how about removing this line? https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:502: // which excludes closures. Looks like the cause for this comment was lost during rewriting :) How about adding the following? "Therefore, we can avoid looking for closures in initializers of the parameters." https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/closure/info.dart (right): https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/info.dart:40: // Please, remove the empty comment line here. https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/info.dart:41: final Map<VariableDeclaration, int> variables = <VariableDeclaration, int>{}; How about adding helper functions to hide the internals of the flags? For example, like this: ``` static bool isCapturedInInitializer(int flags) { return (flags & INSIDE_INITIALIZER) != 0; } static bool isUsedInBody(int flags) { return (flags & OUTSIDE_INITIALIZER) != 0; } static bool isCausingRedirection(int flags) { return flags == (INSIDE_INITIALIZER | OUTSIDE_INITIALIZER); } ``` Or even creating a helper class [CapturedVariable] and an enum for flags? ``` enum CapturingFlag { capturedInInitializer, usedInBody, } class CapturedVariable { VariableDeclaration variable; int _flags; CapturedVariable(this.variable) : _flags = 0; void addFlag(CapturingFlag flag) { _flags |= (1 << flag.index); } // ... helper functions ... } ```
https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/closure/converter.dart (right): https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:75: // they are captured inside or outside an initializer. On 2017/08/01 09:36:13, Dmitry Stefantsov wrote: > Please, add a reference to the field in [ClosureInfo] with the explanation of > the flags values. Done. https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:225: int capturedBoth = On 2017/08/01 09:36:13, Dmitry Stefantsov wrote: > I think using `final` modifier makes sense for this variable. Done. https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:267: node.initializers = node.initializers.take(1).toList(); On 2017/08/01 09:36:13, Dmitry Stefantsov wrote: > Here we still treat the first initializer as special, although we now have it in > a local variable with a descriptive name. How about doing the following here? > > ``` > node.initializers = <Initializer>[contextDeclInit]; > ``` Done. https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:280: assert(function.typeParameters.length == 0); On 2017/08/01 09:36:13, Dmitry Stefantsov wrote: > If we are now sure that the [FunctionNode] of the constructor may not have its > own type parameters, how about removing this line? Done. https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/converter.dart:502: // which excludes closures. On 2017/08/01 09:36:13, Dmitry Stefantsov wrote: > Looks like the cause for this comment was lost during rewriting :) How about > adding the following? > > "Therefore, we can avoid looking for closures in initializers of the > parameters." Done. https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/closure/info.dart (right): https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/info.dart:40: // On 2017/08/01 09:36:14, Dmitry Stefantsov wrote: > Please, remove the empty comment line here. Done. https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/info.dart:41: final Map<VariableDeclaration, int> variables = <VariableDeclaration, int>{}; On 2017/08/01 09:36:14, Dmitry Stefantsov wrote: > How about adding helper functions to hide the internals of the flags? For > example, like this: > > ``` > static bool isCapturedInInitializer(int flags) { > return (flags & INSIDE_INITIALIZER) != 0; > } > static bool isUsedInBody(int flags) { > return (flags & OUTSIDE_INITIALIZER) != 0; > } > static bool isCausingRedirection(int flags) { > return flags == (INSIDE_INITIALIZER | OUTSIDE_INITIALIZER); > } > ``` > > Or even creating a helper class [CapturedVariable] and an enum for flags? > > ``` > enum CapturingFlag { > capturedInInitializer, > usedInBody, > } > > class CapturedVariable { > VariableDeclaration variable; > int _flags; > CapturedVariable(this.variable) : _flags = 0; > void addFlag(CapturingFlag flag) { > _flags |= (1 << flag.index); > } > // ... helper functions ... > } > ``` I would prefer the second approach over the first, but personally I feel that "boilerplate" code like this should be kept to a minimum, especially when it doesn't significantly simplify the places where the flag is used. However, I'll change it if you strongly disagree.
LGTM https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... File pkg/kernel/lib/transformations/closure/info.dart (right): https://codereview.chromium.org/2991853002/diff/40001/pkg/kernel/lib/transfor... pkg/kernel/lib/transformations/closure/info.dart:41: final Map<VariableDeclaration, int> variables = <VariableDeclaration, int>{}; On 2017/08/01 14:58:25, sjindel wrote: > On 2017/08/01 09:36:14, Dmitry Stefantsov wrote: > > How about adding helper functions to hide the internals of the flags? For > > example, like this: > > > > ``` > > static bool isCapturedInInitializer(int flags) { > > return (flags & INSIDE_INITIALIZER) != 0; > > } > > static bool isUsedInBody(int flags) { > > return (flags & OUTSIDE_INITIALIZER) != 0; > > } > > static bool isCausingRedirection(int flags) { > > return flags == (INSIDE_INITIALIZER | OUTSIDE_INITIALIZER); > > } > > ``` > > > > Or even creating a helper class [CapturedVariable] and an enum for flags? > > > > ``` > > enum CapturingFlag { > > capturedInInitializer, > > usedInBody, > > } > > > > class CapturedVariable { > > VariableDeclaration variable; > > int _flags; > > CapturedVariable(this.variable) : _flags = 0; > > void addFlag(CapturingFlag flag) { > > _flags |= (1 << flag.index); > > } > > // ... helper functions ... > > } > > ``` > > I would prefer the second approach over the first, but personally I feel that > "boilerplate" code like this should be kept to a minimum, especially when it > doesn't significantly simplify the places where the flag is used. > > However, I'll change it if you strongly disagree. I guess it depends on the definition of "significantly" :) To me, manipulations with flags always bring too many parens which makes it harder to read the conditions on flags. Feel free to leave the code as it is.
Description was changed from ========== Fix duplicate context creation when closures appear in initializers. Summary: Previously, when a function parameter was captured both in an initializer and in the body of a constructor, we would create two contexts: one created in a local initializer and used by other initializers, and another in the body of the function. This is incorrect, as it means that changes to the parameter in the initializer's closure won't be visible in the body. Now, to work around this problem we re-use the context created for the initializers in the body of the constructor by moving the body into a new constructor, and redirecting the original constructor to that one, passing the context as an additional argument. This dance is necessary because local initializers aren't visible in the body of a constructor. Test Plan: A few of the existing closure conversion tests were changed or fixed by this revision. We also modify the 'closure_in_initializer.dart' test to hit this case directly. Reviewers: dmitryas@google.com ========== to ========== Fix duplicate context creation when closures appear in initializers. Summary: Previously, when a function parameter was captured both in an initializer and in the body of a constructor, we would create two contexts: one created in a local initializer and used by other initializers, and another in the body of the function. This is incorrect, as it means that changes to the parameter in the initializer's closure won't be visible in the body. Now, to work around this problem we re-use the context created for the initializers in the body of the constructor by moving the body into a new constructor, and redirecting the original constructor to that one, passing the context as an additional argument. This dance is necessary because local initializers aren't visible in the body of a constructor. Test Plan: A few of the existing closure conversion tests were changed or fixed by this revision. We also modify the 'closure_in_initializer.dart' test to hit this case directly. R=dmitryas@google.com Reviewers: dmitryas@google.com Committed: https://github.com/dart-lang/sdk/commit/454ab91576ffd071b2e6a9b54f67453f33b8ee21 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) manually as 454ab91576ffd071b2e6a9b54f67453f33b8ee21 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
