Update synthetic fields check for Kotlin EnumEntries

Summary: - Restore the disabled Kotlin enum test w/ `EnumEntries` disabled - Add `support_kt_19_enum_entries` option to enable a new safe enum check logic path to accommodate the new structure of Kotlin 1.9 enums Reviewed By: beicy Differential Revision: D53681655 fbshipit-source-id: 0bc947904c876e17dac6fcf66b2e2cd89b5a6f0d
facebook · Mar 16, 2024 · bea7f55 · bea7f55
1 parent 5963a4f
commit bea7f55
Show file tree

Hide file tree

Showing 6 changed files with 74 additions and 9 deletions.
diff --git a/opt/optimize_enums/EnumUpcastAnalysis.h b/opt/optimize_enums/EnumUpcastAnalysis.h
@@ -31,6 +31,10 @@ bool is_enum_valueof(const DexMethodRef* method);
  */
 bool is_enum_values(const DexMethodRef* method);
 
+constexpr const char* KT_ENUM_ENTRIES_TYPE = "Lkotlin/enums/EnumEntries;";
+constexpr const char* ENUM_VALUES_FIELD = "$VALUES";
+constexpr const char* ENUM_ENTRIES_FIELD = "$ENTRIES";
+
 // Store possible types for a register although we only care about Object, Enum
 // and Enum's subtypes.
 using EnumTypes = sparta::PatriciaTreeSetAbstractDomain<DexType*>;

diff --git a/opt/optimize_enums/OptimizeEnums.cpp b/opt/optimize_enums/OptimizeEnums.cpp
@@ -375,6 +375,7 @@ class OptimizeEnums {
       PassManager& mgr,
       int max_enum_size,
       bool skip_sanity_check,
+      const bool support_kt_19_enum_entries,
       const std::vector<DexType*>& allowlist,
       ConfigFiles& conf,
       std::unordered_map<UnsafeType, size_t>& unsafe_counts) {
@@ -396,8 +397,8 @@ class OptimizeEnums {
      * fields, and virtual methods are safe.
      */
 
-    auto is_safe_enum = [this, &base_enum_check](const DexClass* cls,
-                                                 UnsafeTypes& utypes) {
+    auto is_safe_enum = [this, &base_enum_check, support_kt_19_enum_entries](
+                            const DexClass* cls, UnsafeTypes& utypes) {
       if (!base_enum_check(cls)) {
         return false;
       }
@@ -411,8 +412,14 @@ class OptimizeEnums {
       if (!cls->get_interfaces()->empty()) {
         utypes.insert(UnsafeType::kHasInterfaces);
       }
-      if (!only_one_static_synth_field(cls)) {
-        utypes.insert(UnsafeType::kMoreThanOneSynthField);
+      if (support_kt_19_enum_entries) {
+        if (!only_has_expected_static_synth_fields(cls)) {
+          utypes.insert(UnsafeType::kUnexpectedSynthFields);
+        }
+      } else {
+        if (!only_one_static_synth_field(cls)) {
+          utypes.insert(UnsafeType::kMoreThanOneSynthField);
+        }
       }
 
       const auto& ctors = cls->get_ctors();
@@ -584,6 +591,52 @@ class OptimizeEnums {
     return true;
   }
 
+  bool only_has_expected_static_synth_fields(const DexClass* cls) {
+    const auto synth_access = optimize_enums::synth_access();
+    const auto enum_entries_type =
+        DexType::get_type(optimize_enums::KT_ENUM_ENTRIES_TYPE);
+    always_assert(enum_entries_type != nullptr);
+    std::set<std::string> expected_fields = {ENUM_VALUES_FIELD,
+                                             ENUM_ENTRIES_FIELD};
+    for (auto field : cls->get_sfields()) {
+      if (!check_required_access_flags(synth_access, field->get_access())) {
+        continue;
+      }
+      auto fname = field->get_name()->str_copy();
+      if (expected_fields.count(fname)) {
+        expected_fields.erase(fname);
+        if (fname == ENUM_ENTRIES_FIELD &&
+            field->get_type() != enum_entries_type) {
+          TRACE(ENUM, 2, "Unexpected synthetic field %s", SHOW(field));
+          return false;
+        }
+      } else {
+        TRACE(ENUM, 2, "Unexpected synthetic field %s", SHOW(field));
+        return false;
+      }
+    }
+    if (expected_fields.empty()) {
+      return true;
+    }
+    if (expected_fields.size() == 1 &&
+        *expected_fields.begin() == ENUM_ENTRIES_FIELD) {
+      return true;
+    }
+
+    if (traceEnabled(ENUM, 2)) {
+      std::ostringstream s;
+      for (const auto& field : cls->get_sfields()) {
+        if (!check_required_access_flags(synth_access, field->get_access())) {
+          continue;
+        }
+        s << field->get_name()->str_copy() << ", ";
+      }
+      TRACE(ENUM, 2, "Unexpected synthetic fields found on %s %s", SHOW(cls),
+            s.str().c_str());
+    }
+    return false;
+  }
+
   /**
    * Returns true if the constructor invokes `Enum.<init>`, sets its instance
    * fields, and then returns. We want to make sure there are no side affects.
@@ -867,6 +920,8 @@ void OptimizeEnumsPass::bind_config() {
        "enum fields, try to erase them without considering reference equality "
        "of the enum objects. Do not add enums to the allowlist!");
   bind("skip_sanity_check", false, m_skip_sanity_check, "May skip some check.");
+  bind("support_kt_19_enum_entries", false, m_support_kt_19_enum_entries,
+       "Try to optimize Kotlin 1.9 Enums with EnumEntries feature.");
 }
 
 void OptimizeEnumsPass::run_pass(DexStoresVector& stores,
@@ -875,9 +930,9 @@ void OptimizeEnumsPass::run_pass(DexStoresVector& stores,
   OptimizeEnums opt_enums(stores, conf);
   opt_enums.remove_redundant_generated_classes();
   std::unordered_map<UnsafeType, size_t> unsafe_counts;
-  opt_enums.replace_enum_with_int(mgr, m_max_enum_size, m_skip_sanity_check,
-                                  m_enum_to_integer_allowlist, conf,
-                                  unsafe_counts);
+  opt_enums.replace_enum_with_int(
+      mgr, m_max_enum_size, m_skip_sanity_check, m_support_kt_19_enum_entries,
+      m_enum_to_integer_allowlist, conf, unsafe_counts);
   opt_enums.remove_enum_generated_methods();
   opt_enums.stats(mgr);
   for (auto& p : unsafe_counts) {

diff --git a/opt/optimize_enums/OptimizeEnums.h b/opt/optimize_enums/OptimizeEnums.h
@@ -31,6 +31,7 @@ class OptimizeEnumsPass : public Pass {
  private:
   int m_max_enum_size;
   bool m_skip_sanity_check;
+  bool m_support_kt_19_enum_entries;
   std::vector<DexType*> m_enum_to_integer_allowlist;
 };
 

diff --git a/opt/optimize_enums/OptimizeEnumsUnsafeType.h b/opt/optimize_enums/OptimizeEnumsUnsafeType.h
@@ -19,6 +19,7 @@ enum class UnsafeType {
   kCannotDelete,
   kHasInterfaces,
   kMoreThanOneSynthField,
+  kUnexpectedSynthFields,
   kMultipleCtors,
   kComplexCtor,
   kUnrenamableDmethod,
@@ -57,6 +58,9 @@ inline std::ostream& operator<<(std::ostream& os, const UnsafeType& u) {
   case UnsafeType::kMoreThanOneSynthField:
     os << "MoreThanOneSynthField";
     break;
+  case UnsafeType::kUnexpectedSynthFields:
+    os << "UnexpectedSynthFields";
+    break;
   case UnsafeType::kMultipleCtors:
     os << "MultipleCtors";
     break;

diff --git a/test/instr/EnumTransformTest.config b/test/instr/EnumTransformTest.config
@@ -7,7 +7,8 @@
   },
   "OptimizeEnumsPass" : {
     "max_enum_size" : 4,
-    "break_reference_equality_allowlist" : ["Lredex/PURE_SCORE;"]
+    "break_reference_equality_allowlist" : ["Lredex/PURE_SCORE;"],
+    "support_kt_19_enum_entries" : true
   },
   "ir_type_checker": {
     "run_after_passes" : [

diff --git a/test/instr/EnumTransformTest.kt b/test/instr/EnumTransformTest.kt
@@ -18,7 +18,7 @@ import org.junit.Test
 // POSTCHECK: (PUBLIC, STATIC, FINAL) f0:java.lang.Integer
 // POSTCHECK: (PUBLIC, STATIC, FINAL) f1:java.lang.Integer
 // POSTCHECK: (PUBLIC, STATIC, FINAL) f2:java.lang.Integer
-// POSTCHECK: (PUBLIC, STATIC, FINAL) f3:java.lang.Integer
+// POSTCHECK-NOT: (PUBLIC, STATIC, FINAL) f3:java.lang.Integer
 // POSTCHECK-NOT: (PUBLIC, STATIC, FINAL) f4:java.lang.Integer
 
 // Simple case is optimized.