Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(283)

Side by Side Diff: pkg/kernel/lib/transformations/closure/converter.dart

Issue 2991853002: Fix duplicate context creation when closures appear in initializers. (Closed)
Patch Set: Created 3 years, 4 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View unified diff | Download patch
OLDNEW
1 // Copyright (c) 2016, the Dart project authors. Please see the AUTHORS file 1 // Copyright (c) 2016, the Dart project authors. Please see the AUTHORS file
2 // for details. All rights reserved. Use of this source code is governed by a 2 // for details. All rights reserved. Use of this source code is governed by a
3 // BSD-style license that can be found in the LICENSE file. 3 // BSD-style license that can be found in the LICENSE file.
4 4
5 library kernel.transformations.closure.converter; 5 library kernel.transformations.closure.converter;
6 6
7 import '../../ast.dart' 7 import '../../ast.dart'
8 show 8 show
9 Arguments, 9 Arguments,
10 Block, 10 Block,
(...skipping 10 matching lines...) Expand all
21 ForStatement, 21 ForStatement,
22 FunctionDeclaration, 22 FunctionDeclaration,
23 FunctionExpression, 23 FunctionExpression,
24 FunctionNode, 24 FunctionNode,
25 FunctionType, 25 FunctionType,
26 InterfaceType, 26 InterfaceType,
27 InvalidExpression, 27 InvalidExpression,
28 InvocationExpression, 28 InvocationExpression,
29 Library, 29 Library,
30 LocalInitializer, 30 LocalInitializer,
31 RedirectingInitializer,
31 Member, 32 Member,
32 MethodInvocation, 33 MethodInvocation,
33 Name, 34 Name,
34 NamedExpression, 35 NamedExpression,
35 NamedType, 36 NamedType,
36 NullLiteral, 37 NullLiteral,
37 Procedure, 38 Procedure,
38 ProcedureKind, 39 ProcedureKind,
39 PropertyGet, 40 PropertyGet,
40 ReturnStatement, 41 ReturnStatement,
(...skipping 13 matching lines...) Expand all
54 import '../../frontend/accessors.dart' show VariableAccessor; 55 import '../../frontend/accessors.dart' show VariableAccessor;
55 56
56 import '../../clone.dart' show CloneVisitor; 57 import '../../clone.dart' show CloneVisitor;
57 58
58 import '../../core_types.dart' show CoreTypes; 59 import '../../core_types.dart' show CoreTypes;
59 60
60 import '../../type_algebra.dart' show substitute; 61 import '../../type_algebra.dart' show substitute;
61 62
62 import 'clone_without_body.dart' show CloneWithoutBody; 63 import 'clone_without_body.dart' show CloneWithoutBody;
63 64
64 import 'context.dart' show Context, NoContext; 65 import 'context.dart' show Context, NoContext, LocalContext;
65 66
66 import 'info.dart' show ClosureInfo; 67 import 'info.dart' show ClosureInfo;
67 68
68 import 'rewriter.dart' show AstRewriter, BlockRewriter, InitializerListRewriter; 69 import 'rewriter.dart' show AstRewriter, BlockRewriter, InitializerListRewriter;
69 70
70 class ClosureConverter extends Transformer { 71 class ClosureConverter extends Transformer {
71 final CoreTypes coreTypes; 72 final CoreTypes coreTypes;
72 final Set<VariableDeclaration> capturedVariables; 73 final Set<VariableDeclaration> capturedVariables;
73 final Map<FunctionNode, Set<TypeParameter>> capturedTypeVariables; 74 final Map<FunctionNode, Set<TypeParameter>> capturedTypeVariables;
74 final Map<FunctionNode, VariableDeclaration> thisAccess; 75 final Map<FunctionNode, VariableDeclaration> thisAccess;
(...skipping 123 matching lines...) Expand 10 before | Expand all | Expand 10 after
198 // TODO(karlklose): add a fine-grained analysis of captured parameters. 199 // TODO(karlklose): add a fine-grained analysis of captured parameters.
199 node.function.positionalParameters 200 node.function.positionalParameters
200 .where(capturedVariables.contains) 201 .where(capturedVariables.contains)
201 .forEach(extendContextWith); 202 .forEach(extendContextWith);
202 node.function.namedParameters 203 node.function.namedParameters
203 .where(capturedVariables.contains) 204 .where(capturedVariables.contains)
204 .forEach(extendContextWith); 205 .forEach(extendContextWith);
205 206
206 transformList(node.initializers, this, node); 207 transformList(node.initializers, this, node);
207 node.initializers.insertAll(0, initRewriter.prefix); 208 node.initializers.insertAll(0, initRewriter.prefix);
208 context = rewriter = null; 209 rewriter = null;
209 } 210 }
210 211
211 // Transform constructor body. 212 // Transform constructor body.
212 FunctionNode function = node.function; 213 FunctionNode function = node.function;
213 if (function.body != null && function.body is! EmptyStatement) { 214 if (function.body != null && function.body is! EmptyStatement) {
214 setupContextForFunctionBody(function); 215 // If we created a context for the initializers, we need to re-use that
216 // context in the body of the function. Unfortunately, the context is
217 // declared in a local initializer and local initializers aren't visible
218 // in the body of the constructor. To work around this issue, we move the
219 // body into a new constructor and make this constructor redirect to that
220 // one, passing the context as an argument to the new constructor.
221 var movingCtor = context != null && context is! NoContext;
222
223 setupContextForFunctionBody(function, context);
215 VariableDeclaration self = thisAccess[currentMemberFunction]; 224 VariableDeclaration self = thisAccess[currentMemberFunction];
216 if (self != null) { 225 if (self != null) {
217 context.extend(self, new ThisExpression()); 226 context.extend(self, new ThisExpression());
218 } 227 }
219 node.function.accept(this); 228 node.function.accept(this);
220 resetContext(); 229
230 if (movingCtor) {
231 var newCtorName = new Name("${node.name.name}#redir");
232 var newCtor = new Constructor(node.function, name: newCtorName);
233 newClassMembers.add(newCtor);
234
235 for (var i = 1; i < node.initializers.length; ++i)
Dmitry Stefantsov 2017/07/28 12:36:16 I guess here we make some assumptions about the st
sjindel 2017/07/31 12:06:20 I rewrote it to look for the context's initializer
236 newCtor.initializers.add(node.initializers[i]);
237
238 node.initializers = node.initializers.take(1).toList();
239
240 var cv = new CloneVisitor();
241 var oldCtorParams = function.positionalParameters
242 .map(cv.visitVariableDeclaration)
243 .toList();
244 var oldCtorNamedParams =
245 function.namedParameters.map(cv.visitVariableDeclaration).toList();
246
247 function.positionalParameters.addAll(function.namedParameters);
248 function.namedParameters = [];
249
250 var args = <Expression>[];
251 assert(function.typeParameters.length == 0);
Dmitry Stefantsov 2017/07/28 12:36:15 I think we can't supply additional type arguments
sjindel 2017/07/31 12:06:20 The comment above the [Constructor] class confirms
252 args.addAll(oldCtorParams.map((decl) => new VariableGet(decl)));
253 args.addAll(oldCtorNamedParams.map((decl) => new VariableGet(decl)));
254
255 node.function = new FunctionNode(new EmptyStatement(),
256 typeParameters: [],
257 positionalParameters: oldCtorParams,
258 namedParameters: oldCtorNamedParams,
259 requiredParameterCount: function.requiredParameterCount,
260 returnType: function.returnType,
261 asyncMarker: function.asyncMarker,
262 dartAsyncMarker: function.dartAsyncMarker);
263 node.function.parent = node;
264
265 var contextDecl = (context as LocalContext).self;
266 var oldCtorDecl =
267 (new CloneVisitor()).visitVariableDeclaration(contextDecl);
Dmitry Stefantsov 2017/07/28 12:36:16 Can we use [cv] from above here?
sjindel 2017/07/31 12:06:20 Done.
268 contextDecl.initializer = null;
269 function.positionalParameters.add(contextDecl);
270 function.requiredParameterCount++;
271
272 var contextInit = node.initializers[0] as LocalInitializer;
Dmitry Stefantsov 2017/07/28 12:36:16 Same note about treating 0-th initializer as speci
sjindel 2017/07/31 12:06:20 Done.
273 contextInit.variable = oldCtorDecl;
274 oldCtorDecl.parent = contextInit;
275
276 args.add(new VariableGet(oldCtorDecl));
277 var redirInit =
278 new RedirectingInitializer(newCtor, new Arguments(args));
279 node.initializers.add(redirInit);
280 }
221 } 281 }
282 resetContext();
222 return node; 283 return node;
223 } 284 }
224 285
225 AstRewriter makeRewriterForBody(FunctionNode function) { 286 AstRewriter makeRewriterForBody(FunctionNode function) {
226 Statement body = function.body; 287 Statement body = function.body;
227 if (body is! Block) { 288 if (body is! Block) {
228 body = new Block(<Statement>[body]); 289 body = new Block(<Statement>[body]);
229 function.body = function.body.parent = body; 290 function.body = function.body.parent = body;
230 } 291 }
231 return new BlockRewriter(body); 292 return new BlockRewriter(body);
(...skipping 141 matching lines...) Expand 10 before | Expand all | Expand 10 after
373 if (self != null) { 434 if (self != null) {
374 context.extend(self, new ThisExpression()); 435 context.extend(self, new ThisExpression());
375 } 436 }
376 node.transformChildren(this); 437 node.transformChildren(this);
377 resetContext(); 438 resetContext();
378 } 439 }
379 440
380 return node; 441 return node;
381 } 442 }
382 443
383 void setupContextForFunctionBody(FunctionNode function) { 444 void setupContextForFunctionBody(FunctionNode function,
445 [Context reuse = null]) {
Dmitry Stefantsov 2017/07/28 12:36:16 Why do we need this optional parameter here?
sjindel 2017/07/31 12:06:20 So we don't reset the function body's context when
Dmitry Stefantsov 2017/07/31 15:05:35 I asked this question because [reuse] wasn't menti
sjindel 2017/07/31 15:32:16 I see, I suppose it wasn't actually needed then.
384 Statement body = function.body; 446 Statement body = function.body;
385 assert(body != null); 447 assert(body != null);
386 currentMemberFunction = function; 448 currentMemberFunction = function;
387 // Ensure that the body is a block which becomes the current block. 449 // Ensure that the body is a block which becomes the current block.
388 rewriter = makeRewriterForBody(function); 450 rewriter = makeRewriterForBody(function);
389 // Start with no context. This happens after setting up _currentBlock 451 // Start with no context. This happens after setting up _currentBlock
390 // so statements can be emitted into _currentBlock if necessary. 452 // so statements can be emitted into _currentBlock if necessary.
391 context = new NoContext(this); 453 if (context == null) context = new NoContext(this);
392 } 454 }
393 455
394 void resetContext() { 456 void resetContext() {
395 rewriter = null; 457 rewriter = null;
396 context = null; 458 context = null;
397 currentMemberFunction = null; 459 currentMemberFunction = null;
398 currentMember = null; 460 currentMember = null;
399 } 461 }
400 462
401 bool get isEmptyContext { 463 bool get isEmptyContext {
402 return rewriter == null && context == null; 464 return rewriter == null && context == null;
403 } 465 }
404 466
405 TreeNode visitLocalInitializer(LocalInitializer node) { 467 TreeNode visitLocalInitializer(LocalInitializer node) {
406 assert(!capturedVariables.contains(node.variable)); 468 assert(!capturedVariables.contains(node.variable));
407 node.transformChildren(this); 469 node.transformChildren(this);
408 return node; 470 return node;
409 } 471 }
410 472
411 TreeNode visitFunctionNode(FunctionNode node) { 473 TreeNode visitFunctionNode(FunctionNode node) {
412 transformList(node.typeParameters, this, node); 474 transformList(node.typeParameters, this, node);
413 // TODO: Can parameters contain initializers (e.g., for optional ones) that 475 // TODO: Can parameters contain initializers (e.g., for optional ones) that
414 // need to be closure converted? 476 // need to be closure converted?
415 node.positionalParameters 477 //
416 .where(capturedVariables.contains) 478 // Answer: no, initializers for optional parameters must be compile-time
417 .forEach(extendContextWith); 479 // constants, which excludes closures.
Dmitry Stefantsov 2017/07/28 12:36:15 I would suggest rewriting this comment, so that it
sjindel 2017/07/31 12:06:20 Done.
418 node.namedParameters 480 if (context is NoContext) {
Dmitry Stefantsov 2017/07/28 12:36:15 Why do we need this check here?
sjindel 2017/07/31 12:06:20 We don't want to re-visit the function's parameter
Dmitry Stefantsov 2017/07/31 15:05:35 Yes, I meant that if this check makes distinction
sjindel 2017/07/31 15:32:16 The case is removed.
419 .where(capturedVariables.contains) 481 node.positionalParameters
420 .forEach(extendContextWith); 482 .where(capturedVariables.contains)
483 .forEach(extendContextWith);
484 node.namedParameters
485 .where(capturedVariables.contains)
486 .forEach(extendContextWith);
487 }
421 assert(node.body != null); 488 assert(node.body != null);
422 node.body = node.body.accept(this); 489 node.body = node.body.accept(this);
423 node.body.parent = node; 490 node.body.parent = node;
424 return node; 491 return node;
425 } 492 }
426 493
427 TreeNode visitBlock(Block node) { 494 TreeNode visitBlock(Block node) {
428 return saveContext(() { 495 return saveContext(() {
429 BlockRewriter blockRewriter = rewriter = rewriter.forNestedBlock(node); 496 BlockRewriter blockRewriter = rewriter = rewriter.forNestedBlock(node);
430 blockRewriter.transformStatements(this); 497 blockRewriter.transformStatements(this);
(...skipping 264 matching lines...) Expand 10 before | Expand all | Expand 10 after
695 copy.function.body.parent = copy.function; 762 copy.function.body.parent = copy.function;
696 return copy; 763 return copy;
697 } 764 }
698 765
699 void addGetterForwarder(Name name, Procedure getter) { 766 void addGetterForwarder(Name name, Procedure getter) {
700 assert(getter.isGetter); 767 assert(getter.isGetter);
701 newClassMembers 768 newClassMembers
702 .add(copyWithBody(getter, forwardToThisProperty(getter))..name = name); 769 .add(copyWithBody(getter, forwardToThisProperty(getter))..name = name);
703 } 770 }
704 } 771 }
OLDNEW

Powered by Google App Engine
This is Rietveld 408576698