From acd2ad41b66187564b65d93cd70538c01727c720 Mon Sep 17 00:00:00 2001
From: Nate Biggs <natebiggs@google.com>
Date: Fri, 14 Jul 2023 20:46:17 +0000
Subject: [PATCH] [dart2js] Union old and new type when refining in type
 inference.

This change doesn't seem to have a significant impact on most compilation results:
- Golem results show no significant difference in microbenchmarks.
- For a medium and large app tested, while we see a small change in the actual inference results, the generated code is identical before/after this change.
- Timing and memory usage on internal compilations seem comparable before/after this change.

Note: This replaces the need for any notion of "invalid" refines so I will clean up that code in a follow up change.

Change-Id: I2a293eacd944fc17ee2dab97d3d947c042b4038f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/313720
Commit-Queue: Nate Biggs <natebiggs@google.com>
Reviewed-by: Mayank Patke <fishythefish@google.com>
---
 pkg/compiler/lib/src/inferrer/engine.dart     | 12 +++-
 ...0868787_test.dart => 290868787a_test.dart} |  0
 tests/web/regress/issue/290868787b_test.dart  | 57 +++++++++++++++++++
 3 files changed, 68 insertions(+), 1 deletion(-)
 rename tests/web/regress/issue/{290868787_test.dart => 290868787a_test.dart} (100%)
 create mode 100644 tests/web/regress/issue/290868787b_test.dart

diff --git a/pkg/compiler/lib/src/inferrer/engine.dart b/pkg/compiler/lib/src/inferrer/engine.dart
index 48709b7593e..ca33199880c 100644
--- a/pkg/compiler/lib/src/inferrer/engine.dart
+++ b/pkg/compiler/lib/src/inferrer/engine.dart
@@ -782,7 +782,17 @@ class InferrerEngine {
       _progress.showProgress('Inferred ', _overallRefineCount, ' types.');
       TypeInformation info = _workQueue.remove();
       AbstractValue oldType = info.type;
-      AbstractValue newType = info.refine(this);
+
+      // In order to ensure that types are always getting wider we union the old
+      // and new refined types. This ensures that we do not get caught in any
+      // refinement loops that can arise from horizontal or downward
+      // (i.e. narrowing) moves in the type lattice.
+      //
+      // This tends to produce comparable results to not doing the union and
+      // often with fewer refine steps since the natural progression of these
+      // refines is to take us up the lattice anyway.
+      AbstractValue newType =
+          abstractValueDomain.union(oldType, info.refine(this));
       // Check that refinement has not accidentally changed the type.
       assert(oldType == info.type);
       if (info.abandonInferencing) info.doNotEnqueue = true;
diff --git a/tests/web/regress/issue/290868787_test.dart b/tests/web/regress/issue/290868787a_test.dart
similarity index 100%
rename from tests/web/regress/issue/290868787_test.dart
rename to tests/web/regress/issue/290868787a_test.dart
diff --git a/tests/web/regress/issue/290868787b_test.dart b/tests/web/regress/issue/290868787b_test.dart
new file mode 100644
index 00000000000..e816cf4d11e
--- /dev/null
+++ b/tests/web/regress/issue/290868787b_test.dart
@@ -0,0 +1,57 @@
+// Copyright (c) 2023, 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.
+
+abstract class A {
+  A get a;
+}
+
+class B implements A {
+  @override
+  C get a => C();
+}
+
+class C implements A {
+  @override
+  B get a => B();
+}
+
+class D implements B, C {
+  @override
+  H get a => H();
+}
+
+class E implements B, C {
+  @override
+  H get a => H();
+}
+
+class F implements B, C {
+  @override
+  H get a => H();
+}
+
+class G implements B, C {
+  @override
+  H get a => H();
+}
+
+class H implements B, C {
+  @override
+  H get a => H();
+}
+
+void foo(int n, A a) {
+  if (n > 0) {
+    foo(n - 1, E());
+    foo(n - 1, F());
+    foo(n - 1, G());
+    foo(n - 1, a.a);
+  }
+}
+
+void main() {
+  B();
+  C();
+  foo(2, D());
+}