[VM] Removes types from reused temps in async.

Clement Skau · commit-bot@chromium.org · commit ffde1585dae0 · 2021-07-12T08:55:53.000Z
When we detect that a temporary variable is being reused with a different type, we change the type of the temp to dynamic. This allows us to retain types for any non-reused temps. For any dynamic temps we still have type information available for use sites, which we can pass on via unsafeCast. One drawback of this approach is that we don't know ahead of time which temps are reused, and get turned into dynamic, so we have to unsafeCast all uses. This change is similar to what was done in: https://dart-review.googlesource.com/c/sdk/+/138500 TEST=Adds tests/language/vm/regress_flutter_85311_test.dart, updates relevant expect files. Bug: flutter/flutter#85311 Change-Id: I821c5266327892d5c3fd5bae1bebba7f3fe3931b Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/205647 Commit-Queue: Clement Skau <cskau@google.com> Reviewed-by: Alexander Markov <alexmarkov@google.com>
diff --git a/pkg/front_end/testcases/general/async_nested.dart.weak.transformed.expect b/pkg/front_end/testcases/general/async_nested.dart.weak.transformed.expect
@@ -36,9 +36,9 @@ static method main() → void /* originally async */ {
   core::int* :await_jump_var = 0;
   dynamic :await_ctx_var;
   dynamic :saved_try_context_var0;
-  dynamic :async_temporary_0;
-  dynamic :async_temporary_1;
-  dynamic :async_temporary_2;
+  self::Node* :async_temporary_0;
+  self::Node* :async_temporary_1;
+  self::Node* :async_temporary_2;
   function :async_op([dynamic :result, dynamic :exception, dynamic :stack_trace]) → dynamic yielding 
     try {
       #L1:
diff --git a/pkg/front_end/testcases/general/await_complex.dart.weak.transformed.expect b/pkg/front_end/testcases/general/await_complex.dart.weak.transformed.expect
@@ -65,12 +65,12 @@ static method staticMembers() → dynamic /* originally async */ {
   core::int* :await_jump_var = 0;
   dynamic :await_ctx_var;
   dynamic :saved_try_context_var0;
-  dynamic :async_temporary_0;
-  dynamic :async_temporary_1;
-  dynamic :async_temporary_2;
-  dynamic :async_temporary_3;
-  dynamic :async_temporary_4;
-  dynamic :async_temporary_5;
+  core::int* :async_temporary_0;
+  core::int* :async_temporary_1;
+  core::int* :async_temporary_2;
+  core::int* :async_temporary_3;
+  core::int* :async_temporary_4;
+  core::int* :async_temporary_5;
   function :async_op([dynamic :result, dynamic :exception, dynamic :stack_trace]) → dynamic yielding 
     try {
       #L1:
@@ -121,11 +121,11 @@ static method topLevelMembers() → dynamic /* originally async */ {
   core::int* :await_jump_var = 0;
   dynamic :await_ctx_var;
   dynamic :saved_try_context_var0;
-  dynamic :async_temporary_0;
-  dynamic :async_temporary_1;
-  dynamic :async_temporary_2;
-  dynamic :async_temporary_3;
-  dynamic :async_temporary_4;
+  core::int* :async_temporary_0;
+  core::int* :async_temporary_1;
+  core::int* :async_temporary_2;
+  core::int* :async_temporary_3;
+  core::int* :async_temporary_4;
   function :async_op([dynamic :result, dynamic :exception, dynamic :stack_trace]) → dynamic yielding 
     try {
       #L2:
@@ -172,11 +172,11 @@ static method instanceMembers() → dynamic /* originally async */ {
   core::int* :await_jump_var = 0;
   dynamic :await_ctx_var;
   dynamic :saved_try_context_var0;
-  dynamic :async_temporary_0;
-  dynamic :async_temporary_1;
-  dynamic :async_temporary_2;
-  dynamic :async_temporary_3;
-  dynamic :async_temporary_4;
+  core::int* :async_temporary_0;
+  core::int* :async_temporary_1;
+  core::int* :async_temporary_2;
+  core::int* :async_temporary_3;
+  core::int* :async_temporary_4;
   function :async_op([dynamic :result, dynamic :exception, dynamic :stack_trace]) → dynamic yielding 
     try {
       #L3:
@@ -225,10 +225,10 @@ static method others() → dynamic /* originally async */ {
   dynamic :await_ctx_var;
   dynamic :saved_try_context_var0;
   dynamic :async_temporary_0;
-  dynamic :async_temporary_1;
-  dynamic :async_temporary_2;
-  dynamic :async_temporary_3;
-  dynamic :async_temporary_4;
+  core::int* :async_temporary_1;
+  core::int* :async_temporary_2;
+  core::List<core::int*>* :async_temporary_3;
+  core::int* :async_temporary_4;
   function :async_op([dynamic :result, dynamic :exception, dynamic :stack_trace]) → dynamic yielding 
     try {
       #L4:
@@ -288,13 +288,13 @@ static method conditionals() → dynamic /* originally async */ {
         core::bool* a = false;
         core::bool* b = true;
         :async_temporary_0 = (a || b) =={core::Object::==}{(core::Object) → core::bool} true;
-        if(:async_temporary_0)
+        if(_in::unsafeCast<core::bool*>(:async_temporary_0))
           ;
         else {
           [yield] let dynamic #t22 = asy::_awaitHelper(self::dummy(), :async_op_then, :async_op_error, :async_op) in null;
           :async_temporary_0 = :result as{TypeError,ForDynamic} core::bool* =={core::Object::==}{(core::Object) → core::bool} true;
         }
-        core::bool* c = :async_temporary_0;
+        core::bool* c = _in::unsafeCast<core::bool*>(:async_temporary_0);
         self::expect(true, c);
         if(a || b) {
           :async_temporary_1 = a;
@@ -426,8 +426,8 @@ static method controlFlow() → dynamic /* originally async */ {
   dynamic :saved_try_context_var3;
   dynamic :exception0;
   dynamic :stack_trace0;
-  dynamic :async_temporary_0;
-  dynamic :async_temporary_1;
+  core::List<dynamic>* :async_temporary_0;
+  core::List<dynamic>* :async_temporary_1;
   function :async_op([dynamic :result, dynamic :exception, dynamic :stack_trace]) → dynamic yielding 
     try {
       #L7:
diff --git a/pkg/front_end/testcases/general/bug33206.dart.weak.transformed.expect b/pkg/front_end/testcases/general/bug33206.dart.weak.transformed.expect
@@ -105,7 +105,7 @@ static method foo() → asy::Future<self::X*>* /* originally async */ {
   core::int* :await_jump_var = 0;
   dynamic :await_ctx_var;
   dynamic :saved_try_context_var0;
-  dynamic :async_temporary_0;
+  self::Y* :async_temporary_0;
   function :async_op([dynamic :result, dynamic :exception, dynamic :stack_trace]) → dynamic yielding 
     try {
       #L3:
diff --git a/pkg/front_end/testcases/general/issue40662.dart.weak.transformed.expect b/pkg/front_end/testcases/general/issue40662.dart.weak.transformed.expect
@@ -17,10 +17,10 @@ static method foo(core::int* x) → dynamic /* originally async */ {
   core::int* :await_jump_var = 0;
   dynamic :await_ctx_var;
   dynamic :saved_try_context_var0;
-  dynamic :async_temporary_0;
-  dynamic :async_temporary_1;
+  core::int* :async_temporary_0;
+  core::int* :async_temporary_1;
   core::List<core::int*>* :async_temporary_2;
-  dynamic :async_temporary_3;
+  core::int* :async_temporary_3;
   function :async_op([dynamic :result, dynamic :exception, dynamic :stack_trace]) → dynamic yielding 
     try {
       #L1:
@@ -35,7 +35,7 @@ static method foo(core::int* x) → dynamic /* originally async */ {
         else {
           :async_temporary_2 = null;
         }
-        :return_value = self::bar(_in::unsafeCast<core::int*>(:async_temporary_3), :async_temporary_2);
+        :return_value = self::bar(_in::unsafeCast<core::int*>(:async_temporary_3), _in::unsafeCast<core::List<core::int*>*>(:async_temporary_2));
         break #L1;
       }
       asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
@@ -88,4 +88,4 @@ static method expect(dynamic expected, dynamic actual) → dynamic {
 Extra constant evaluation status:
 Evaluated: InstanceInvocation @ org-dartlang-testcase:///issue40662.dart:8:10 -> IntConstant(-1)
 Evaluated: InstanceInvocation @ org-dartlang-testcase:///issue40662.dart:9:10 -> IntConstant(-1)
-Extra constant evaluation: evaluated: 93, effectively constant: 2
+Extra constant evaluation: evaluated: 94, effectively constant: 2
diff --git a/pkg/front_end/testcases/general/regression_flutter51828.dart.weak.transformed.expect b/pkg/front_end/testcases/general/regression_flutter51828.dart.weak.transformed.expect
@@ -91,7 +91,7 @@ static method main() → dynamic /* originally async */ {
   core::int* :await_jump_var = 0;
   dynamic :await_ctx_var;
   dynamic :saved_try_context_var0;
-  dynamic :async_temporary_0;
+  self::B* :async_temporary_0;
   dynamic :async_temporary_1;
   function :async_op([dynamic :result, dynamic :exception, dynamic :stack_trace]) → dynamic yielding 
     try {
diff --git a/pkg/front_end/testcases/inference/future_then_conditional.dart.weak.transformed.expect b/pkg/front_end/testcases/inference/future_then_conditional.dart.weak.transformed.expect
@@ -56,7 +56,7 @@ static method test() → void {
             [yield] let dynamic #t1 = asy::_awaitHelper(asy::Future::value<core::int*>(3), :async_op_then, :async_op_error, :async_op) in null;
             :async_temporary_0 = _in::unsafeCast<core::int*>(:result);
           }
-          :return_value = :async_temporary_0;
+          :return_value = _in::unsafeCast<core::int*>(:async_temporary_0);
           break #L1;
         }
         asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
diff --git a/pkg/front_end/testcases/inference/future_then_conditional_2.dart.weak.transformed.expect b/pkg/front_end/testcases/inference/future_then_conditional_2.dart.weak.transformed.expect
@@ -56,7 +56,7 @@ static method test() → void {
             [yield] let dynamic #t1 = asy::_awaitHelper(new self::MyFuture::value<core::int*>(3), :async_op_then, :async_op_error, :async_op) in null;
             :async_temporary_0 = _in::unsafeCast<core::int*>(:result);
           }
-          :return_value = :async_temporary_0;
+          :return_value = _in::unsafeCast<core::int*>(:async_temporary_0);
           break #L1;
         }
         asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
diff --git a/pkg/front_end/testcases/inference/future_then_conditional_3.dart.weak.transformed.expect b/pkg/front_end/testcases/inference/future_then_conditional_3.dart.weak.transformed.expect
@@ -56,7 +56,7 @@ static method test() → void {
             [yield] let dynamic #t1 = asy::_awaitHelper(asy::Future::value<core::int*>(3), :async_op_then, :async_op_error, :async_op) in null;
             :async_temporary_0 = _in::unsafeCast<core::int*>(:result);
           }
-          :return_value = :async_temporary_0;
+          :return_value = _in::unsafeCast<core::int*>(:async_temporary_0);
           break #L1;
         }
         asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
diff --git a/pkg/front_end/testcases/inference/future_then_conditional_4.dart.weak.transformed.expect b/pkg/front_end/testcases/inference/future_then_conditional_4.dart.weak.transformed.expect
@@ -56,7 +56,7 @@ static method test() → void {
             [yield] let dynamic #t1 = asy::_awaitHelper(new self::MyFuture::value<core::int*>(3), :async_op_then, :async_op_error, :async_op) in null;
             :async_temporary_0 = _in::unsafeCast<core::int*>(:result);
           }
-          :return_value = :async_temporary_0;
+          :return_value = _in::unsafeCast<core::int*>(:async_temporary_0);
           break #L1;
         }
         asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
diff --git a/pkg/front_end/testcases/inference/future_then_conditional_5.dart.weak.transformed.expect b/pkg/front_end/testcases/inference/future_then_conditional_5.dart.weak.transformed.expect
@@ -56,7 +56,7 @@ static method test() → void {
             [yield] let dynamic #t1 = asy::_awaitHelper(new self::MyFuture::value<core::int*>(3), :async_op_then, :async_op_error, :async_op) in null;
             :async_temporary_0 = _in::unsafeCast<core::int*>(:result);
           }
-          :return_value = :async_temporary_0;
+          :return_value = _in::unsafeCast<core::int*>(:async_temporary_0);
           break #L1;
         }
         asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
diff --git a/pkg/front_end/testcases/inference/future_then_conditional_6.dart.weak.transformed.expect b/pkg/front_end/testcases/inference/future_then_conditional_6.dart.weak.transformed.expect
@@ -56,7 +56,7 @@ static method test() → void {
             [yield] let dynamic #t1 = asy::_awaitHelper(asy::Future::value<core::int*>(3), :async_op_then, :async_op_error, :async_op) in null;
             :async_temporary_0 = _in::unsafeCast<core::int*>(:result);
           }
-          :return_value = :async_temporary_0;
+          :return_value = _in::unsafeCast<core::int*>(:async_temporary_0);
           break #L1;
         }
         asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
diff --git a/pkg/front_end/testcases/inference/future_then_ifNull.dart.weak.transformed.expect b/pkg/front_end/testcases/inference/future_then_ifNull.dart.weak.transformed.expect
@@ -57,7 +57,7 @@ static method test() → void {
           else {
             :async_temporary_0 = #t1;
           }
-          :return_value = :async_temporary_0;
+          :return_value = _in::unsafeCast<core::int*>(:async_temporary_0);
           break #L1;
         }
         asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
diff --git a/pkg/kernel/lib/transformations/async.dart b/pkg/kernel/lib/transformations/async.dart
@@ -115,21 +115,40 @@ class ExpressionLifter extends Transformer {
     return result;
   }
 
+  // Wraps VariableGet in an unsafeCast if `type` isn't dynamic.
+  Expression unsafeCastVariableGet(
+      VariableDeclaration variable, DartType type) {
+    if (type != const DynamicType()) {
+      return StaticInvocation(
+          continuationRewriter.helper.unsafeCast,
+          Arguments(<Expression>[VariableGet(variable)],
+              types: <DartType>[type]));
+    }
+    return VariableGet(variable);
+  }
+
   // Name an expression by emitting an assignment to a temporary variable.
   Expression name(Expression expr) {
-    // Allocate as dynamic as temps might be reused with different types.
-    VariableDeclaration temp =
-        allocateTemporary(nameIndex, const DynamicType());
-    statements.add(ExpressionStatement(VariableSet(temp, expr)));
-    // Type annotate the get via an unsafe cast since all temps are allocated
-    // as dynamic.
     DartType type = expr.getStaticType(_staticTypeContext);
-    return StaticInvocation(continuationRewriter.helper.unsafeCast,
-        Arguments(<Expression>[VariableGet(temp)], types: <DartType>[type]));
+    VariableDeclaration temp = allocateTemporary(nameIndex, type);
+    statements.add(ExpressionStatement(VariableSet(temp, expr)));
+    // Wrap in unsafeCast to make sure we pass type information even if we later
+    // have to re-type the temporary variable to dynamic.
+    return unsafeCastVariableGet(temp, type);
   }
 
   VariableDeclaration allocateTemporary(int index,
       [DartType type = const DynamicType()]) {
+    if (variables.length > index) {
+      // Re-type temporary to dynamic if we detect reuse with different type.
+      // Note: We should make sure all uses use `unsafeCast(...)` to pass their
+      // type information on, as that is lost otherwise.
+      if (variables[index].type != const DynamicType() &&
+          variables[index].type != type) {
+        variables[index].type = const DynamicType();
+      }
+      return variables[index];
+    }
     for (var i = variables.length; i <= index; i++) {
       variables.add(VariableDeclaration(":async_temporary_${i}", type: type));
     }
@@ -384,10 +403,9 @@ class ExpressionLifter extends Transformer {
     // so any statements it emits occur after in the accumulated list (that is,
     // so they occur before in the corresponding block).
     var rightBody = blockOf(rightStatements);
-    var result = allocateTemporary(
-        nameIndex,
-        _staticTypeContext.typeEnvironment.coreTypes
-            .boolRawType(_staticTypeContext.nonNullable));
+    final type = _staticTypeContext.typeEnvironment.coreTypes
+        .boolRawType(_staticTypeContext.nonNullable);
+    final result = allocateTemporary(nameIndex, type);
     final objectEquals = continuationRewriter.helper.coreTypes.objectEquals;
     rightBody.addStatement(new ExpressionStatement(new VariableSet(
         result,
@@ -402,7 +420,8 @@ class ExpressionLifter extends Transformer {
       then = new EmptyStatement();
       otherwise = rightBody;
     }
-    statements.add(new IfStatement(new VariableGet(result), then, otherwise));
+    statements.add(
+        new IfStatement(unsafeCastVariableGet(result, type), then, otherwise));
 
     final test = new EqualsCall(expr.left, new BoolLiteral(true),
         interfaceTarget: objectEquals,
@@ -414,7 +433,7 @@ class ExpressionLifter extends Transformer {
 
     ++nameIndex;
     seenAwait = seenAwait || rightAwait;
-    return new VariableGet(result);
+    return unsafeCastVariableGet(result, type);
   }
 
   TreeNode visitConditionalExpression(ConditionalExpression expr) {
@@ -455,15 +474,15 @@ class ExpressionLifter extends Transformer {
       });
     }
 
-    // If then or otherwise has emitted statements we will produce a temporary t
-    // and emit:
+    // If `then` or `otherwise` has emitted statements we will produce a
+    // temporary t and emit:
     //
     // if ([condition]) {
     //   t = [left];
     // } else {
     //   t = [right];
     // }
-    var result = allocateTemporary(nameIndex, expr.staticType);
+    final result = allocateTemporary(nameIndex, expr.staticType);
     var thenBody = blockOf(thenStatements);
     var otherwiseBody = blockOf(otherwiseStatements);
     thenBody.addStatement(
@@ -478,7 +497,7 @@ class ExpressionLifter extends Transformer {
 
     ++nameIndex;
     seenAwait = seenAwait || thenAwait || otherwiseAwait;
-    return new VariableGet(result);
+    return unsafeCastVariableGet(result, expr.staticType);
   }
 
   // Others.
diff --git a/pkg/vm/testcases/transformations/type_flow/transformer/async_await.dart.expect b/pkg/vm/testcases/transformations/type_flow/transformer/async_await.dart.expect
@@ -2,7 +2,6 @@ library #lib;
 import self as self;
 import "dart:core" as core;
 import "dart:async" as asy;
-import "dart:_internal" as _in;
 
 class A extends core::Object {
   synthetic constructor •() → self::A*
@@ -86,7 +85,7 @@ static method main() → dynamic /* originally async */ {
       {
         :async_temporary_0 = [@vm.inferred-type.metadata=#lib::A] self::foo();
         [yield] let dynamic #t1 = asy::_awaitHelper([@vm.inferred-type.metadata=dart.async::_Future<dynamic>] self::baz(), :async_op_then, :async_op_error, :async_op) in null;
-        [yield] let dynamic #t2 = asy::_awaitHelper([@vm.direct-call.metadata=#lib::A.bar??] [@vm.inferred-type.metadata=dart.async::_Future<dynamic> (receiver not int)] _in::unsafeCast<dynamic>(:async_temporary_0){dynamic}.bar(:result), :async_op_then, :async_op_error, :async_op) in null;
+        [yield] let dynamic #t2 = asy::_awaitHelper([@vm.direct-call.metadata=#lib::A.bar??] [@vm.inferred-type.metadata=dart.async::_Future<dynamic> (receiver not int)] :async_temporary_0{dynamic}.bar(:result), :async_op_then, :async_op_error, :async_op) in null;
         :result;
       }
       asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);
diff --git a/tests/language/vm/regress_flutter_85311_test.dart b/tests/language/vm/regress_flutter_85311_test.dart
@@ -0,0 +1,34 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// This is a regression test for the bug in
+// https://github.com/flutter/flutter/issues/51828.
+// Verifies that temporaries aren't incorrectly assigned types when they're
+// reused.
+
+import "package:expect/expect.dart";
+
+bool wasCalled = false;
+
+class Z {
+  bool operator ==(Object other) {
+    wasCalled = true;
+    return true;
+  }
+}
+
+class Y {
+  final dynamic v;
+  Y(this.v);
+}
+
+Future<bool> crash(dynamic y) async {
+  return (y.v == (await 7) || y == (await 9));
+}
+
+void main() async {
+  await crash(Y(Z()));
+
+  Expect.isTrue(wasCalled);
+}
diff --git a/tests/language_2/vm/regress_flutter_85311_test.dart b/tests/language_2/vm/regress_flutter_85311_test.dart

Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,7 @@ static method test() → void {`
`56`	`56`	`[yield] let dynamic #t1 = asy::_awaitHelper(asy::Future::value<core::int*>(3), :async_op_then, :async_op_error, :async_op) in null;`
`57`	`57`	`:async_temporary_0 = _in::unsafeCast<core::int*>(:result);`
`58`	`58`	`}`
`59`		`- :return_value = :async_temporary_0;`
	`59`	`+ :return_value = _in::unsafeCast<core::int*>(:async_temporary_0);`
`60`	`60`	`break #L1;`
`61`	`61`	`}`
`62`	`62`	`asy::_completeOnAsyncReturn(:async_future, :return_value, :is_sync);`