J2CL: Add extra guardrails (#6171)

The patch puts a new guardrail that will only hoist the field
if it is initialized with the owner class.

The constant hoisting optimization in J2CL pass relies on the
assumption that clinit that will initialize the field will be
executed before the read of the field. That means the field
that is optimized is within the same class:

class Foo {
  public static final Object field = new Object();
}

Although it is possible to observe the initial value, that is
not intention of the developer (which the point of the
optimization).

However can also see a similar pattern in following:

class Foo {
  public static Object field;
}

class Zoo {
  static {
    Foo.field = new Object();
  }
}

Currently the pass also optimizes it as well since the field
is only initialized once and by a clinit. However Zoo clinit
is not guaranteed to be run before Foo.field access so it is
less safe to speculate on the intention of the developer here
hence it is not worth the risk.

FWIW, we haven't seen this issue. But this is something we
are also guarding in Closure Compiler so I decided it is
worthwhile to do here as well.

1 files changed, 17 insertions, 4 deletions

diff --git a/src/passes/J2CLOpts.cpp b/src/passes/J2CLOpts.cpp
index b1fb8bb1f..f6821b80f 100644
--- a/src/passes/J2CLOpts.cpp
+++ b/src/passes/J2CLOpts.cpp
@@ -67,7 +67,7 @@ private:
     // Avoid optimizing class initialization condition variable itself. If we
     // were optimizing it then it would become "true" and would defeat its
     // functionality and the clinit would never trigger during runtime.
-    if (name.startsWith("f_$initialized__")) {
+    if (name.startsWith("$class-initialized@")) {
       return;
     }
     assignmentCounts[name]++;
@@ -90,13 +90,14 @@ public:
     if (!isOnceFunction(curr)) {
       return;
     }
+    Name enclosingClassName = getEnclosingClass(curr->name);
     int optimizedBefore = optimized;
     if (auto* block = curr->body->dynCast<Block>()) {
       for (auto*& expr : block->list) {
-        maybeHoistConstant(expr);
+        maybeHoistConstant(expr, enclosingClassName);
       }
     } else {
-      maybeHoistConstant(curr->body);
+      maybeHoistConstant(curr->body, enclosingClassName);
     }
 
     if (optimized != optimizedBefore) {
@@ -111,7 +112,7 @@ public:
   }
 
 private:
-  void maybeHoistConstant(Expression* expr) {
+  void maybeHoistConstant(Expression* expr, Name enclosingClassName) {
     auto set = expr->dynCast<GlobalSet>();
     if (!set) {
       return;
@@ -123,6 +124,14 @@ private:
       return;
     }
 
+    if (getEnclosingClass(set->name) != enclosingClassName) {
+      // Only hoist fields initialized by its own class.
+      // If it is only initialized once but by another class (although it is
+      // very uncommon / edge scenario) then we cannot be sure if the clinit was
+      // triggered before the field access so it is better to leave it alone.
+      return;
+    }
+
     if (!GlobalUtils::canInitializeGlobal(*getModule(), set->value)) {
       // It is not a valid constant expression so cannot be hoisted to
       // global init.
@@ -138,6 +147,10 @@ private:
     optimized++;
   }
 
+  Name getEnclosingClass(Name name) {
+    return Name(name.str.substr(name.str.find_last_of('@')));
+  }
+
   AssignmentCountMap& assignmentCounts;
 };