Skip to content

Commit 3151714

Browse files
alexmarkovCommit Queue
alexmarkov
authored and
Commit Queue
committed
[cfe,dyn_modules] Fix dynamic module validation of redirecting factories
Previously, AST for invocation of a redirecting factory didn't keep the original redirecting factory (only a resolved redirection target). So dynamic module validator was incorrectly checking if target of a redirecting factory is listed as callable in the dynamic interface. After https://dart-review.googlesource.com/c/sdk/+/454820, AST now has a proper RedirectingFactoryInvocation node which contains a reference to the original redirecting factory. This changes fixes dynamic module validator to verify original redirecting factory instead of its target. Fixes b/448593948 Change-Id: I4ade5663fe878af508ec9aa59a5ee9176d60e41a Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/459520 Reviewed-by: Johnni Winther <johnniwinther@google.com> Commit-Queue: Alexander Markov <alexmarkov@google.com> Reviewed-by: Sigmund Cherem <sigmund@google.com>
1 parent f242e0f commit 3151714

File tree

8 files changed

+234
-80
lines changed

8 files changed

+234
-80
lines changed

‎pkg/front_end/lib/src/kernel/dynamic_module_validator.dart‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -586,6 +586,17 @@ class _DynamicModuleValidator extends RecursiveVisitor {
586586
super.visitConstructorTearOff(node);
587587
}
588588

589+
@override
590+
void visitRedirectingFactoryInvocation(RedirectingFactoryInvocation node) {
591+
_verifyCallable(node.redirectingFactoryTarget, node);
592+
// Do not visit node.expression in order to avoid checking
593+
// that target of a redirecting factory is callable.
594+
// However, still visit children of node.expression to validate arguments.
595+
final InvocationExpression expr = node.expression;
596+
assert(expr is ConstructorInvocation || expr is StaticInvocation);
597+
expr.visitChildren(this);
598+
}
599+
589600
@override
590601
// Coverage-ignore(suite): Not run.
591602
void visitRedirectingFactoryTearOff(RedirectingFactoryTearOff node) {
@@ -724,6 +735,7 @@ class _DynamicModuleValidator extends RecursiveVisitor {
724735
case Constructor():
725736
String name = target.enclosingClass.name;
726737
if (target.name.text.isNotEmpty) {
738+
// Coverage-ignore-block(suite): Not run.
727739
name += '.' + target.name.text;
728740
}
729741
loader.addProblem(

‎pkg/front_end/testcases/general/dynamic_modules/dynamic_interface.yaml‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,22 @@ callable:
5353
class: '_ConstForC7'
5454
- library: 'main_lib1.dart'
5555
member: '_privateMethodForC7'
56+
- library: 'main_lib1.dart'
57+
class: 'C8'
58+
member: 'fact3'
59+
- library: 'main_lib1.dart'
60+
class: 'C8'
61+
member: ''
62+
- library: 'main_lib1.dart'
63+
class: 'C8'
64+
member: 'constConstr'
65+
- library: 'main_lib1.dart'
66+
class: 'C9'
67+
member: 'fact1'
68+
- library: 'main_lib1.dart'
69+
class: 'C9'
70+
member: 'fact2'
5671
- library: 'main_lib1.dart'
5772
class: 'C9'
58-
member: '_'
73+
member: 'fact4'
5974
- library: 'main_lib2.dart'

‎pkg/front_end/testcases/general/dynamic_modules/main.dart‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -119,11 +119,16 @@ void test() {
119119
print(field2);
120120
field2 = 42;
121121

122-
// Not allowed - target of redirecting factory is not callable.
123-
print(C8());
124-
125-
// Allowed - target of redirecting factory is callable.
126-
print(C9());
122+
// Not allowed - redirecting factories are not callable
123+
// (although their targets are callable).
124+
print(C8.fact1());
125+
print(C8.fact2());
126+
print(const C8.fact4());
127+
128+
// Allowed - redirecting factories are callable.
129+
print(C9.fact1());
130+
print(C9.fact2());
131+
print(const C9.fact4());
127132

128133
// Not allowed.
129134
print(ExtType1);

‎pkg/front_end/testcases/general/dynamic_modules/main.dart.strong.expect‎

Lines changed: 64 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -165,22 +165,32 @@
165165
// field1 = 42;
166166
// ^
167167
//
168-
// pkg/front_end/testcases/general/dynamic_modules/main.dart:123:9: Error: Cannot invoke constructor 'C8._' from a dynamic module.
169-
// Try removing the call or update the dynamic interface to list constructor 'C8._' as callable.
170-
// print(C8());
171-
// ^
168+
// pkg/front_end/testcases/general/dynamic_modules/main.dart:124:12: Error: Cannot invoke member 'C8.fact1' from a dynamic module.
169+
// Try removing the call or update the dynamic interface to list member 'C8.fact1' as callable.
170+
// print(C8.fact1());
171+
// ^
172+
//
173+
// pkg/front_end/testcases/general/dynamic_modules/main.dart:125:12: Error: Cannot invoke member 'C8.fact2' from a dynamic module.
174+
// Try removing the call or update the dynamic interface to list member 'C8.fact2' as callable.
175+
// print(C8.fact2());
176+
// ^
172177
//
173-
// pkg/front_end/testcases/general/dynamic_modules/main.dart:129:9: Error: Cannot use extension type 'ExtType1' in a dynamic module.
178+
// pkg/front_end/testcases/general/dynamic_modules/main.dart:126:15: Error: Cannot invoke member 'C8.fact4' from a dynamic module.
179+
// Try removing the call or update the dynamic interface to list member 'C8.fact4' as callable.
180+
// print(const C8.fact4());
181+
// ^
182+
//
183+
// pkg/front_end/testcases/general/dynamic_modules/main.dart:134:9: Error: Cannot use extension type 'ExtType1' in a dynamic module.
174184
// Try removing the reference to extension type 'ExtType1' or update the dynamic interface to list extension type 'ExtType1' as callable.
175185
// print(ExtType1);
176186
// ^
177187
//
178-
// pkg/front_end/testcases/general/dynamic_modules/main.dart:130:9: Error: Cannot invoke member 'ExtType1.new' from a dynamic module.
188+
// pkg/front_end/testcases/general/dynamic_modules/main.dart:135:9: Error: Cannot invoke member 'ExtType1.new' from a dynamic module.
179189
// Try removing the call or update the dynamic interface to list member 'ExtType1.new' as callable.
180190
// print(ExtType1(42));
181191
// ^
182192
//
183-
// pkg/front_end/testcases/general/dynamic_modules/main.dart:131:12: Error: Cannot invoke member 'Ext1.isPositive' from a dynamic module.
193+
// pkg/front_end/testcases/general/dynamic_modules/main.dart:136:12: Error: Cannot invoke member 'Ext1.isPositive' from a dynamic module.
184194
// Try removing the call or update the dynamic interface to list member 'Ext1.isPositive' as callable.
185195
// print(42.isPositive);
186196
// ^
@@ -332,15 +342,19 @@ static method test() → void {
332342
mai::field1 = 42;
333343
core::print(mai::field2);
334344
mai::field2 = 42;
335-
core::print(new mai::C8::_());
336-
core::print(new mai::C9::_());
345+
core::print(new mai::C8::•());
346+
core::print(mai::C8::fact3());
337347
core::print(#C13);
348+
core::print(new mai::C9::•());
349+
core::print(mai::C9::fact3());
350+
core::print(#C14);
351+
core::print(#C15);
338352
core::print(mai::ExtType1|constructor#(42));
339353
core::print(mai::Ext1|get#isPositive(42));
340354
core::print(new mai2::Lib3Class::•());
341355
mai2::lib3Method();
342356
mai2::lib3Field = 42;
343-
core::print(#C13);
357+
core::print(#C15);
344358
core::print(mai2::Lib3ExtType|constructor#(42));
345359
core::print(mai2::Lib3Ext|get#lib3IsPositive(42));
346360
}
@@ -441,17 +455,35 @@ class C7 extends core::Object {
441455
: super core::Object::•()
442456
;
443457
}
444-
class C8 extends core::Object {
445-
constructor _() → mai::C8
458+
class C8 extends core::Object /*hasConstConstructor*/ {
459+
constructor •() → mai::C8
446460
: super core::Object::•() {}
447-
static factory •() → mai::C8 /* redirection-target: mai::C8::_ */
448-
return new mai::C8::_();
449-
}
450-
class C9 extends core::Object {
451-
constructor _() → mai::C9
461+
const constructor constConstr() → mai::C8
462+
: super core::Object::•()
463+
;
464+
static factory fact1() → mai::C8 /* redirection-target: mai::C8::• */
465+
return new mai::C8::•();
466+
static factory fact2() → mai::C8 /* redirection-target: mai::C8::fact3 */
467+
return mai::C8::fact3();
468+
static factory fact3() → mai::C8
469+
return new mai::C8::•();
470+
static factory fact4() → mai::C8 /* redirection-target: mai::C8::constConstr */
471+
return new mai::C8::constConstr();
472+
}
473+
class C9 extends core::Object /*hasConstConstructor*/ {
474+
constructor •() → mai::C9
452475
: super core::Object::•() {}
453-
static factory •() → mai::C9 /* redirection-target: mai::C9::_ */
454-
return new mai::C9::_();
476+
const constructor constConstr() → mai::C9
477+
: super core::Object::•()
478+
;
479+
static factory fact1() → mai::C9 /* redirection-target: mai::C9::• */
480+
return new mai::C9::•();
481+
static factory fact2() → mai::C9 /* redirection-target: mai::C9::fact3 */
482+
return mai::C9::fact3();
483+
static factory fact3() → mai::C9
484+
return new mai::C9::•();
485+
static factory fact4() → mai::C9 /* redirection-target: mai::C9::constConstr */
486+
return new mai::C9::constConstr();
455487
}
456488
extension Ext1 on core::int {
457489
get isPositive = mai::Ext1|get#isPositive;
@@ -463,8 +495,8 @@ extension type ExtType1(core::int raw) {
463495
}
464496
static field core::int field1 = 4;
465497
static field core::int field2 = 4;
466-
static const field mai::_C5 const1 = #C15;
467-
static const field mai::_C5 const2 = #C17;
498+
static const field mai::_C5 const1 = #C17;
499+
static const field mai::_C5 const2 = #C19;
468500
static method method1() → void {}
469501
static method method2() → void {}
470502
static method _privateMethod1() → void {}
@@ -534,11 +566,13 @@ constants {
534566
#C10 = constructor-tearoff mai::C2::•
535567
#C11 = static-tearoff mai::method1
536568
#C12 = static-tearoff mai::method2
537-
#C13 = TypeLiteralConstant(core::int)
538-
#C14 = static-tearoff mai::_privateMethod1
539-
#C15 = mai::_C5 {func:#C14}
540-
#C16 = static-tearoff mai::_privateMethod2
569+
#C13 = mai::C8 {}
570+
#C14 = mai::C9 {}
571+
#C15 = TypeLiteralConstant(core::int)
572+
#C16 = static-tearoff mai::_privateMethod1
541573
#C17 = mai::_C5 {func:#C16}
574+
#C18 = static-tearoff mai::_privateMethod2
575+
#C19 = mai::_C5 {func:#C18}
542576
}
543577

544578

@@ -548,3 +582,8 @@ org-dartlang-testcase:///main_lib1.dart:
548582
- Object. (from org-dartlang-sdk:///sdk/lib/core/object.dart)
549583
- _ConstForC7. (from org-dartlang-testcase:///main_lib1.dart:82:9)
550584
- _C5. (from org-dartlang-testcase:///main_lib1.dart:60:9)
585+
586+
org-dartlang-testcase:///main.dart:
587+
- C8.constConstr (from org-dartlang-testcase:///main_lib1.dart:98:9)
588+
- Object. (from org-dartlang-sdk:///sdk/lib/core/object.dart)
589+
- C9.constConstr (from org-dartlang-testcase:///main_lib1.dart:107:9)

‎pkg/front_end/testcases/general/dynamic_modules/main.dart.strong.modular.expect‎

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -165,22 +165,32 @@
165165
// field1 = 42;
166166
// ^
167167
//
168-
// pkg/front_end/testcases/general/dynamic_modules/main.dart:123:9: Error: Cannot invoke constructor 'C8._' from a dynamic module.
169-
// Try removing the call or update the dynamic interface to list constructor 'C8._' as callable.
170-
// print(C8());
171-
// ^
168+
// pkg/front_end/testcases/general/dynamic_modules/main.dart:124:12: Error: Cannot invoke member 'C8.fact1' from a dynamic module.
169+
// Try removing the call or update the dynamic interface to list member 'C8.fact1' as callable.
170+
// print(C8.fact1());
171+
// ^
172+
//
173+
// pkg/front_end/testcases/general/dynamic_modules/main.dart:125:12: Error: Cannot invoke member 'C8.fact2' from a dynamic module.
174+
// Try removing the call or update the dynamic interface to list member 'C8.fact2' as callable.
175+
// print(C8.fact2());
176+
// ^
172177
//
173-
// pkg/front_end/testcases/general/dynamic_modules/main.dart:129:9: Error: Cannot use extension type 'ExtType1' in a dynamic module.
178+
// pkg/front_end/testcases/general/dynamic_modules/main.dart:126:15: Error: Cannot invoke member 'C8.fact4' from a dynamic module.
179+
// Try removing the call or update the dynamic interface to list member 'C8.fact4' as callable.
180+
// print(const C8.fact4());
181+
// ^
182+
//
183+
// pkg/front_end/testcases/general/dynamic_modules/main.dart:134:9: Error: Cannot use extension type 'ExtType1' in a dynamic module.
174184
// Try removing the reference to extension type 'ExtType1' or update the dynamic interface to list extension type 'ExtType1' as callable.
175185
// print(ExtType1);
176186
// ^
177187
//
178-
// pkg/front_end/testcases/general/dynamic_modules/main.dart:130:9: Error: Cannot invoke member 'ExtType1.new' from a dynamic module.
188+
// pkg/front_end/testcases/general/dynamic_modules/main.dart:135:9: Error: Cannot invoke member 'ExtType1.new' from a dynamic module.
179189
// Try removing the call or update the dynamic interface to list member 'ExtType1.new' as callable.
180190
// print(ExtType1(42));
181191
// ^
182192
//
183-
// pkg/front_end/testcases/general/dynamic_modules/main.dart:131:12: Error: Cannot invoke member 'Ext1.isPositive' from a dynamic module.
193+
// pkg/front_end/testcases/general/dynamic_modules/main.dart:136:12: Error: Cannot invoke member 'Ext1.isPositive' from a dynamic module.
184194
// Try removing the call or update the dynamic interface to list member 'Ext1.isPositive' as callable.
185195
// print(42.isPositive);
186196
// ^
@@ -332,15 +342,19 @@ static method test() → void {
332342
mai::field1 = 42;
333343
core::print(mai::field2);
334344
mai::field2 = 42;
335-
core::print(new mai::C8::_());
336-
core::print(new mai::C9::_());
345+
core::print(new mai::C8::•());
346+
core::print(mai::C8::fact3());
337347
core::print(#C13);
348+
core::print(new mai::C9::•());
349+
core::print(mai::C9::fact3());
350+
core::print(#C14);
351+
core::print(#C15);
338352
core::print(mai::ExtType1|constructor#(42));
339353
core::print(mai::Ext1|get#isPositive(42));
340354
core::print(new mai2::Lib3Class::•());
341355
mai2::lib3Method();
342356
mai2::lib3Field = 42;
343-
core::print(#C13);
357+
core::print(#C15);
344358
core::print(mai2::Lib3ExtType|constructor#(42));
345359
core::print(mai2::Lib3Ext|get#lib3IsPositive(42));
346360
}
@@ -359,7 +373,9 @@ constants {
359373
#C10 = constructor-tearoff mai::C2::•
360374
#C11 = static-tearoff mai::method1
361375
#C12 = static-tearoff mai::method2
362-
#C13 = TypeLiteralConstant(core::int)
376+
#C13 = mai::C8 {}
377+
#C14 = mai::C9 {}
378+
#C15 = TypeLiteralConstant(core::int)
363379
}
364380

365381

@@ -368,3 +384,5 @@ org-dartlang-testcase:///main.dart:
368384
- _ConstForC6. (from org-dartlang-testcase:///main_lib1.dart:71:9)
369385
- Object. (from org-dartlang-sdk:///sdk/lib/core/object.dart)
370386
- _ConstForC7. (from org-dartlang-testcase:///main_lib1.dart:82:9)
387+
- C8.constConstr (from org-dartlang-testcase:///main_lib1.dart:98:9)
388+
- C9.constConstr (from org-dartlang-testcase:///main_lib1.dart:107:9)

‎pkg/front_end/testcases/general/dynamic_modules/main.dart.strong.outline.expect‎

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -184,17 +184,35 @@ class C7 extends core::Object {
184184
constructor •({core::Object param = const mai::_ConstForC7::•(mai::_privateMethodForC7)}) → mai::C7
185185
;
186186
}
187-
class C8 extends core::Object {
188-
constructor _() → mai::C8
187+
class C8 extends core::Object /*hasConstConstructor*/ {
188+
constructor •() → mai::C8
189189
;
190-
static factory •() → mai::C8 /* redirection-target: mai::C8::_ */
191-
return new mai::C8::_();
190+
const constructor constConstr() → mai::C8
191+
: super core::Object::•()
192+
;
193+
static factory fact1() → mai::C8 /* redirection-target: mai::C8::• */
194+
return new mai::C8::•();
195+
static factory fact2() → mai::C8 /* redirection-target: mai::C8::fact3 */
196+
return mai::C8::fact3();
197+
static factory fact3() → mai::C8
198+
;
199+
static factory fact4() → mai::C8 /* redirection-target: mai::C8::constConstr */
200+
return new mai::C8::constConstr();
192201
}
193-
class C9 extends core::Object {
194-
constructor _() → mai::C9
202+
class C9 extends core::Object /*hasConstConstructor*/ {
203+
constructor •() → mai::C9
204+
;
205+
const constructor constConstr() → mai::C9
206+
: super core::Object::•()
207+
;
208+
static factory fact1() → mai::C9 /* redirection-target: mai::C9::• */
209+
return new mai::C9::•();
210+
static factory fact2() → mai::C9 /* redirection-target: mai::C9::fact3 */
211+
return mai::C9::fact3();
212+
static factory fact3() → mai::C9
195213
;
196-
static factory •() → mai::C9 /* redirection-target: mai::C9::_ */
197-
return new mai::C9::_();
214+
static factory fact4() → mai::C9 /* redirection-target: mai::C9::constConstr */
215+
return new mai::C9::constConstr();
198216
}
199217
extension Ext1 on core::int {
200218
get isPositive = mai::Ext1|get#isPositive;
@@ -274,4 +292,4 @@ Evaluated: ConstructorInvocation @ org-dartlang-testcase:///main_lib1.dart:78:28
274292
Evaluated: ConstructorInvocation @ org-dartlang-testcase:///main_lib1.dart:89:28 -> InstanceConstant(const _ConstForC7{_ConstForC7._f: _privateMethodForC7})
275293
Evaluated: ConstructorInvocation @ org-dartlang-testcase:///main_lib1.dart:67:16 -> InstanceConstant(const _C5{_C5.func: _privateMethod1})
276294
Evaluated: ConstructorInvocation @ org-dartlang-testcase:///main_lib1.dart:68:16 -> InstanceConstant(const _C5{_C5.func: _privateMethod2})
277-
Extra constant evaluation: evaluated: 16, effectively constant: 6
295+
Extra constant evaluation: evaluated: 20, effectively constant: 6

0 commit comments

Comments
 (0)