[vm/aot/tfa] Add missing visitor methods for call site instrumentation.

askeksa · commit-bot@chromium.org · commit 022b362e8480 · 2020-02-13T15:32:48.000Z
Some visitor methods were missing in the TFA transformation, causing some call sites (such as PropertySet) to not be instrumented with metadata communicating the analysis results. Visiting all instance call sites (including PropertySet) is also needed by table dispatch to collect correct summary information on selectors. Change-Id: I488d5cd10700666dab05bd5c5304010aa90b1943 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/135319 Reviewed-by: Samir Jindel <sjindel@google.com> Reviewed-by: Alexander Markov <alexmarkov@google.com>
diff --git a/pkg/vm/lib/transformations/type_flow/analysis.dart b/pkg/vm/lib/transformations/type_flow/analysis.dart
@@ -162,6 +162,12 @@ class _DirectInvocation extends _Invocation {
     if (selector.member.function != null) {
       typeChecksNeeded = selector.member.function.typeParameters
           .any((t) => t.isGenericCovariantImpl);
+    } else {
+      Field field = selector.member;
+      if (selector.callKind == CallKind.PropertySet) {
+        // TODO(dartbug.com/40615): Use TFA results to improve this criterion.
+        typeChecksNeeded = field.isGenericCovariantImpl;
+      }
     }
   }
 
diff --git a/pkg/vm/lib/transformations/type_flow/transformer.dart b/pkg/vm/lib/transformations/type_flow/transformer.dart
@@ -340,6 +340,12 @@ class AnnotateKernel extends RecursiveVisitor<Null> {
     super.visitPropertyGet(node);
   }
 
+  @override
+  visitPropertySet(PropertySet node) {
+    _annotateCallSite(node, node.interfaceTarget);
+    super.visitPropertySet(node);
+  }
+
   @override
   visitDirectMethodInvocation(DirectMethodInvocation node) {
     _annotateCallSite(node, node.target);
@@ -352,6 +358,12 @@ class AnnotateKernel extends RecursiveVisitor<Null> {
     super.visitDirectPropertyGet(node);
   }
 
+  @override
+  visitDirectPropertySet(DirectPropertySet node) {
+    _annotateCallSite(node, node.target);
+    super.visitDirectPropertySet(node);
+  }
+
   @override
   visitSuperMethodInvocation(SuperMethodInvocation node) {
     _annotateCallSite(node, node.interfaceTarget);
@@ -364,6 +376,12 @@ class AnnotateKernel extends RecursiveVisitor<Null> {
     super.visitSuperPropertyGet(node);
   }
 
+  @override
+  visitSuperPropertySet(SuperPropertySet node) {
+    _annotateCallSite(node, node.interfaceTarget);
+    super.visitSuperPropertySet(node);
+  }
+
   @override
   visitStaticInvocation(StaticInvocation node) {
     _annotateCallSite(node, node.target);
@@ -375,6 +393,12 @@ class AnnotateKernel extends RecursiveVisitor<Null> {
     _annotateCallSite(node, node.target);
     super.visitStaticGet(node);
   }
+
+  @override
+  visitStaticSet(StaticSet node) {
+    _annotateCallSite(node, node.target);
+    super.visitStaticSet(node);
+  }
 }
 
 /// Tree shaking based on results of type flow analysis (TFA).
diff --git a/pkg/vm/testcases/transformations/type_flow/transformer/invalidation_set_field.dart.expect b/pkg/vm/testcases/transformations/type_flow/transformer/invalidation_set_field.dart.expect
@@ -53,7 +53,7 @@ static method use2([@vm.inferred-type.metadata=#lib::DeepCaller2] self::DeepCall
 static method getDynamic() → dynamic
   return [@vm.call-site-attributes.metadata=receiverType:dart.core::Function*] self::unknown.call();
 static method setField2([@vm.inferred-type.metadata=#lib::A] self::A* aa, [@vm.inferred-type.metadata=#lib::T2] dynamic value) → void {
-  [@vm.direct-call.metadata=#lib::A::field2] aa.{self::A::field2} = value;
+  [@vm.direct-call.metadata=#lib::A::field2] [@vm.inferred-type.metadata=!? (skip check)] aa.{self::A::field2} = value;
 }
 static method main(core::List<core::String*>* args) → dynamic {
   new self::A::•();
diff --git a/pkg/vm/testcases/transformations/type_flow/transformer/regress_flutter16182.dart.expect b/pkg/vm/testcases/transformations/type_flow/transformer/regress_flutter16182.dart.expect
@@ -20,7 +20,7 @@ class A1 extends core::Object {
     : super core::Object::•()
     ;
 [@vm.procedure-attributes.metadata=getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4]  method call([dynamic a1 = #C1, dynamic a2 = #C1, dynamic a3 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a4 = #C1, [@vm.inferred-type.metadata=#lib::T1?] dynamic a5 = #C1]) → void {
-    [@vm.direct-call.metadata=#lib::A1::foo] this.{self::A1::foo} = _in::unsafeCast<self::T1*>(a5);
+    [@vm.direct-call.metadata=#lib::A1::foo] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A1::foo} = _in::unsafeCast<self::T1*>(a5);
   }
 }
 class B1 extends core::Object {
@@ -43,7 +43,7 @@ class A2 extends core::Object {
     : super core::Object::•()
     ;
 [@vm.procedure-attributes.metadata=getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4]  method call([dynamic a1 = #C1, dynamic a2 = #C1, dynamic a3 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a4 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a5 = #C1, [@vm.inferred-type.metadata=#lib::T2?] dynamic a6 = #C1]) → void {
-    [@vm.direct-call.metadata=#lib::A2::foo] this.{self::A2::foo} = a6;
+    [@vm.direct-call.metadata=#lib::A2::foo] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A2::foo} = a6;
   }
 }
 abstract class B2Base extends core::Object {
@@ -76,7 +76,7 @@ class A3 extends core::Object {
     : super core::Object::•()
     ;
 [@vm.procedure-attributes.metadata=getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4]  method call([dynamic a1 = #C1, dynamic a2 = #C1, dynamic a3 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a4 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a5 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a6 = #C1, [@vm.inferred-type.metadata=#lib::T3?] dynamic a7 = #C1]) → void {
-    [@vm.direct-call.metadata=#lib::A3::foo] this.{self::A3::foo} = a7;
+    [@vm.direct-call.metadata=#lib::A3::foo] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A3::foo} = a7;
   }
 }
 class B3 extends core::Object {
@@ -99,7 +99,7 @@ class A4 extends core::Object {
     : super core::Object::•()
     ;
 [@vm.procedure-attributes.metadata=getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4]  method call([dynamic a1 = #C1, dynamic a2 = #C1, dynamic a3 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a4 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a5 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a6 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a7 = #C1, [@vm.inferred-type.metadata=#lib::T4?] dynamic a8 = #C1]) → void {
-    [@vm.direct-call.metadata=#lib::A4::foo] this.{self::A4::foo} = a8;
+    [@vm.direct-call.metadata=#lib::A4::foo] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A4::foo} = a8;
   }
 }
 class B4 extends core::Object {
diff --git a/pkg/vm/testcases/transformations/type_flow/transformer/write_only_field.dart.expect b/pkg/vm/testcases/transformations/type_flow/transformer/write_only_field.dart.expect
@@ -16,5 +16,5 @@ class C extends core::Object {
 }
 static method main() → void {
   null;
-  [@vm.direct-call.metadata=#lib::C::instanceField] new self::C::•().{self::C::instanceField} = null;
+  [@vm.direct-call.metadata=#lib::C::instanceField] [@vm.inferred-type.metadata=!? (skip check)] new self::C::•().{self::C::instanceField} = null;
 }
diff --git a/pkg/vm/testcases/transformations/type_flow/transformer/write_only_field3_nnbd.dart.expect b/pkg/vm/testcases/transformations/type_flow/transformer/write_only_field3_nnbd.dart.expect
@@ -9,7 +9,7 @@ class A extends core::Object {
     : super core::Object::•()
     ;
 [@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:1,getterSelectorId:2]  method use() → dynamic {
-    [@vm.direct-call.metadata=#lib::A::x] this.{self::A::x} = 3;
+    [@vm.direct-call.metadata=#lib::A::x] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A::x} = 3;
   }
 [@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasNonThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3]  set /*isNullableByDefault*/ x(core::int value) → void;
 }
@@ -19,7 +19,7 @@ class B extends core::Object {
     : super core::Object::•()
     ;
 [@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:1,getterSelectorId:2]  method use() → dynamic {
-    this.{self::B::x} = 3;
+    [@vm.inferred-type.metadata=!? (skip check)] this.{self::B::x} = 3;
   }
 }
 [@vm.inferred-type.metadata=dart.core::_Smi?]late static final field core::int staticLateB;

Original file line number	Diff line number	Diff line change
`@@ -162,6 +162,12 @@ class _DirectInvocation extends _Invocation {`
`162`	`162`	`if (selector.member.function != null) {`
`163`	`163`	`typeChecksNeeded = selector.member.function.typeParameters`
`164`	`164`	`.any((t) => t.isGenericCovariantImpl);`
	`165`	`+ } else {`
	`166`	`+ Field field = selector.member;`
	`167`	`+ if (selector.callKind == CallKind.PropertySet) {`
	`168`	`+ // TODO(dartbug.com/40615): Use TFA results to improve this criterion.`
	`169`	`+ typeChecksNeeded = field.isGenericCovariantImpl;`
	`170`	`+ }`
`165`	`171`	`}`
`166`	`172`	`}`
`167`	`173`
Original file line number	Diff line number	Diff line change
`@@ -53,7 +53,7 @@ static method use2([@vm.inferred-type.metadata=#lib::DeepCaller2] self::DeepCall`
`53`	`53`	`static method getDynamic() → dynamic`
`54`	`54`	`return [@vm.call-site-attributes.metadata=receiverType:dart.core::Function*] self::unknown.call();`
`55`	`55`	`static method setField2([@vm.inferred-type.metadata=#lib::A] self::A* aa, [@vm.inferred-type.metadata=#lib::T2] dynamic value) → void {`
`56`		`- [@vm.direct-call.metadata=#lib::A::field2] aa.{self::A::field2} = value;`
	`56`	`+ [@vm.direct-call.metadata=#lib::A::field2] [@vm.inferred-type.metadata=!? (skip check)] aa.{self::A::field2} = value;`
`57`	`57`	`}`
`58`	`58`	`static method main(core::List<core::String> args) → dynamic {`
`59`	`59`	`new self::A::•();`
Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,7 @@ class A1 extends core::Object {`
`20`	`20`	`: super core::Object::•()`
`21`	`21`	`;`
`22`	`22`	`[@vm.procedure-attributes.metadata=getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4] method call([dynamic a1 = #C1, dynamic a2 = #C1, dynamic a3 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a4 = #C1, [@vm.inferred-type.metadata=#lib::T1?] dynamic a5 = #C1]) → void {`
`23`		`- [@vm.direct-call.metadata=#lib::A1::foo] this.{self::A1::foo} = _in::unsafeCast<self::T1*>(a5);`
	`23`	`+ [@vm.direct-call.metadata=#lib::A1::foo] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A1::foo} = _in::unsafeCast<self::T1*>(a5);`
`24`	`24`	`}`
`25`	`25`	`}`
`26`	`26`	`class B1 extends core::Object {`
`@@ -43,7 +43,7 @@ class A2 extends core::Object {`
`43`	`43`	`: super core::Object::•()`
`44`	`44`	`;`
`45`	`45`	`[@vm.procedure-attributes.metadata=getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4] method call([dynamic a1 = #C1, dynamic a2 = #C1, dynamic a3 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a4 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a5 = #C1, [@vm.inferred-type.metadata=#lib::T2?] dynamic a6 = #C1]) → void {`
`46`		`- [@vm.direct-call.metadata=#lib::A2::foo] this.{self::A2::foo} = a6;`
	`46`	`+ [@vm.direct-call.metadata=#lib::A2::foo] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A2::foo} = a6;`
`47`	`47`	`}`
`48`	`48`	`}`
`49`	`49`	`abstract class B2Base extends core::Object {`
`@@ -76,7 +76,7 @@ class A3 extends core::Object {`
`76`	`76`	`: super core::Object::•()`
`77`	`77`	`;`
`78`	`78`	`[@vm.procedure-attributes.metadata=getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4] method call([dynamic a1 = #C1, dynamic a2 = #C1, dynamic a3 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a4 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a5 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a6 = #C1, [@vm.inferred-type.metadata=#lib::T3?] dynamic a7 = #C1]) → void {`
`79`		`- [@vm.direct-call.metadata=#lib::A3::foo] this.{self::A3::foo} = a7;`
	`79`	`+ [@vm.direct-call.metadata=#lib::A3::foo] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A3::foo} = a7;`
`80`	`80`	`}`
`81`	`81`	`}`
`82`	`82`	`class B3 extends core::Object {`
`@@ -99,7 +99,7 @@ class A4 extends core::Object {`
`99`	`99`	`: super core::Object::•()`
`100`	`100`	`;`
`101`	`101`	[@vm.procedure-attributes.metadata=getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3,getterSelectorId:4] method call([dynamic a1 = #C1, dynamic a2 = #C1, dynamic a3 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a4 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a5 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a6 = #C1, [@vm.inferred-type.metadata=dart.core::_Smi?] dynamic a7 = #C1, [@vm.inferred-type.metadata=#lib::T4?] dynamic a8 = #C1]) → void {
`102`		`- [@vm.direct-call.metadata=#lib::A4::foo] this.{self::A4::foo} = a8;`
	`102`	`+ [@vm.direct-call.metadata=#lib::A4::foo] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A4::foo} = a8;`
`103`	`103`	`}`
`104`	`104`	`}`
`105`	`105`	`class B4 extends core::Object {`
Original file line number	Diff line number	Diff line change
`@@ -16,5 +16,5 @@ class C extends core::Object {`
`16`	`16`	`}`
`17`	`17`	`static method main() → void {`
`18`	`18`	`null;`
`19`		`- [@vm.direct-call.metadata=#lib::C::instanceField] new self::C::•().{self::C::instanceField} = null;`
	`19`	`+ [@vm.direct-call.metadata=#lib::C::instanceField] [@vm.inferred-type.metadata=!? (skip check)] new self::C::•().{self::C::instanceField} = null;`
`20`	`20`	`}`
Original file line number	Diff line number	Diff line change
`@@ -9,7 +9,7 @@ class A extends core::Object {`
`9`	`9`	`: super core::Object::•()`
`10`	`10`	`;`
`11`	`11`	`[@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:1,getterSelectorId:2] method use() → dynamic {`
`12`		`- [@vm.direct-call.metadata=#lib::A::x] this.{self::A::x} = 3;`
	`12`	`+ [@vm.direct-call.metadata=#lib::A::x] [@vm.inferred-type.metadata=!? (skip check)] this.{self::A::x} = 3;`
`13`	`13`	`}`
`14`	`14`	`[@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasNonThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:3] set /isNullableByDefault/ x(core::int value) → void;`
`15`	`15`	`}`
`@@ -19,7 +19,7 @@ class B extends core::Object {`
`19`	`19`	`: super core::Object::•()`
`20`	`20`	`;`
`21`	`21`	`[@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:1,getterSelectorId:2] method use() → dynamic {`
`22`		`- this.{self::B::x} = 3;`
	`22`	`+ [@vm.inferred-type.metadata=!? (skip check)] this.{self::B::x} = 3;`
`23`	`23`	`}`
`24`	`24`	`}`
`25`	`25`	`[@vm.inferred-type.metadata=dart.core::_Smi?]late static final field core::int staticLateB;`