[dart2js] Prevent type expressions being hoisted above guards

Use a pinned HTypeKnown on the receiver of the HInstanceEnvironment to
pin the type expression. When the the receiver has a wider type that
is legal for the type expression, this keeps the type expression at a
location where it is valid. Type expressions can still be hoisted if
the HTypeKnown is redundant and optimized away.

Bug: #54329
Change-Id: Iae378f9d517e3e22deca198195cad9811cf37772
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/344841
Reviewed-by: Mayank Patke <fishythefish@google.com>
Commit-Queue: Stephen Adams <sra@google.com>

pkg/compiler/lib/src/ssa/type_builder.dart

@@ -250,6 +250,42 @@ abstract class TypeBuilder {
     if (usesInstanceParameters) {
       HInstruction target =
           builder.localsHandler.readThis(sourceInformation: sourceInformation);
+      // Add a HTypeKnown node to assert that 'this' is known to be a subtype of
+      // the declared type at this point.
+      //
+      // The pinned asserted type prevents the HInstanceEnvironment being
+      // hoisted to an illegal location. Consider:
+      //
+      //     class C<T> {
+      //       method() => List<T>;
+      //     }
+      //
+      //       final Object o = ...
+      //       while (...) {
+      //         if (o is C && something()) {
+      //           o.method();
+      //           // inlined, including:
+      //           //     t1 = HTypeKnown(C, o);
+      //           //     t2 = HInstanceEnvironment(t1)
+      //         }
+      //
+      // The inlined `method` accesses the instance type parameter, o.(C.T). The
+      // access would become illegal if hoisted out of the `if` statement. The
+      // HTypeKnown is pinned in the if-then branch, preventing the hoisting.
+      //
+      // It is often the case that hoisting _is_ legal. When hoisting is legal,
+      // the HTypeKnown is redundant (e.g. if the type of `o` is known to be `C`
+      // by some means, or there is no guarding condition). The redundant pinned
+      // HTypeKnown goes away (never inserted, or later optimized), no longer
+      // preventing the rest of the type expression from being hoisted.
+
+      HTypeKnown constrained =
+          HTypeKnown.pinned(builder.localsHandler.getTypeOfThis(), target);
+      if (!constrained.isRedundant(builder.closedWorld)) {
+        builder.add(constrained);
+        target = constrained;
+      }
+
       // TODO(sra): HInstanceEnvironment should probably take an interceptor to
       // allow the getInterceptor call to be reused.
       environment =

tests/web/regress/issue/54329a_test.dart

@@ -0,0 +1,38 @@
+// Copyright (c) 2024, 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.
+
+class DupSet<T> {
+  @pragma('dart2js:prefer-inline')
+  void add(T e) {}
+
+  @pragma('dart2js:never-inline')
+  bool contains(Object? e) {
+    return gI++ > 10; // always true.
+  }
+}
+
+int gI = 0;
+
+class Logic {}
+
+class Logic1 extends Logic {}
+
+void loop(List<Logic> receivers, DupSet<Logic>? drivenSignals) {
+  for (final receiver in receivers) {
+    if (drivenSignals != null && !drivenSignals.contains(receiver)) {
+      // In http://dartbug.com/54329 the inlined parametric covariant check `e
+      // as T` from `add` was hoisted out of the loop, above the `null` check.
+      drivenSignals.add(receiver);
+    }
+  }
+}
+
+main() {
+  gI = 100;
+  final ds = DupSet<Logic>();
+
+  loop([Logic()], ds);
+  loop([Logic()], null);
+  loop([Logic1()], DupSet<Logic1>());
+}

tests/web/regress/issue/54329b_test.dart

@@ -0,0 +1,38 @@
+// Copyright (c) 2024, 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.
+
+@pragma('dart2js:never-inline')
+bool foo() {
+  return gI++ > 10;
+}
+
+int gI = 0;
+
+abstract class Base {}
+
+class Group<T> extends Base {
+  @pragma('dart2js:prefer-inline')
+  void check(Object? o) => o as List<T>;
+}
+
+class Other extends Base {}
+
+void loop(List<num> receivers, Base driven) {
+  for (final receiver in receivers) {
+    if (driven is Group && foo()) {
+      // This is a version of the http://dartbug.com/54329 without a `null`
+      // check. The inlined check `e as List<T>` was hoisted above the loop. At
+      // that position, the type expression `List<T>` is invalid and throws an
+      // error in the rti library.
+      driven.check(receivers);
+    }
+  }
+}
+
+main() {
+  gI = 100;
+  loop(<num>[1], Group<num>());
+  loop(<int>[2, 3], Group<int>());
+  loop([], Other());
+}