Enforce explicit factory for DexTypeDomain creation

Summary: Enforce more clear separation of the `DexTypeDomain` construction to avoid confusions - Use `create_nullable` when nullness info is unknown; Typedef annotation use cases go to this one - Use `create_not_null` only for object allocation when we are absolutely sure about the nullness of the object - Use `create_for_anno` only for special case of the Typedef annotation propagation when no other type info is present With this setup, we tighten up the possible combinations of different components in `DexTypeDomain`. We only infer `NOT_NULL` when it is correct to do so. - Update tests with more conservative type info Reviewed By: NTillmann Differential Revision: D51824964 fbshipit-source-id: cfc3fbe176a579d058bbf06dac6f8425f87d0de6
kami-lang · Dec 7, 2023 · f1976ac · f1976ac
1 parent 43f23bd
commit f1976ac
Show file tree

Hide file tree

Showing 10 changed files with 194 additions and 159 deletions.
diff --git a/libredex/DexTypeEnvironment.h b/libredex/DexTypeEnvironment.h
@@ -284,16 +284,7 @@ class DexTypeDomain final
   // catch this. So we insert a redundant '= default'.
   DexTypeDomain() = default;
 
-  explicit DexTypeDomain(const DexType* dex_type,
-                         const DexAnnoType* annotation = nullptr)
-      : ReducedProductAbstractDomain(
-            std::make_tuple(NullnessDomain(NOT_NULL),
-                            SingletonDexTypeDomain(dex_type),
-                            SmallSetDexTypeDomain(dex_type),
-                            (annotation && annotation->m_type)
-                                ? TypedefAnnotationDomain(annotation->m_type)
-                                : TypedefAnnotationDomain())) {}
-
+ private:
   explicit DexTypeDomain(const DexType* dex_type,
                          const Nullness nullness,
                          bool is_dex_type_exact,
@@ -315,12 +306,35 @@ class DexTypeDomain final
             annotation->m_type ? TypedefAnnotationDomain(annotation->m_type)
                                : TypedefAnnotationDomain())) {}
 
+ public:
   static void reduce_product(
       std::tuple<NullnessDomain,
                  SingletonDexTypeDomain,
                  SmallSetDexTypeDomain,
                  TypedefAnnotationDomain>& /* product */) {}
 
+  static DexTypeDomain create_nullable(
+      const DexType* dex_type, const DexAnnoType* annotation = nullptr) {
+    return DexTypeDomain(dex_type, NN_TOP, false, annotation);
+  }
+
+  /*
+   * !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+   * Only create not_null domain when you are absolutely sure about the nullness
+   * of the value!!!
+   * !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
+   * Usually it means the value is created via new-instance or alike. If the
+   * nullness info is incorrect from the source, it could lead to incorrect
+   * analysis result.
+   */
+  static DexTypeDomain create_not_null(const DexType* dex_type) {
+    return DexTypeDomain(dex_type, NOT_NULL, true);
+  }
+
+  static DexTypeDomain create_for_anno(const DexAnnoType* annotation) {
+    return DexTypeDomain(annotation);
+  }
+
   static DexTypeDomain null() { return DexTypeDomain(IS_NULL); }
 
   NullnessDomain get_nullness() const { return get<0>(); }
@@ -357,6 +371,7 @@ class DexTypeDomain final
   const SmallSetDexTypeDomain& get_set_domain() const { return get<2>(); }
 
   const sparta::PatriciaTreeSet<const DexType*>& get_type_set() const {
+    always_assert(!get<2>().is_top() && !get<2>().is_bottom());
     return get<2>().get_types();
   }
 

diff --git a/libredex/TypeInference.cpp b/libredex/TypeInference.cpp
@@ -186,7 +186,7 @@ void set_integral(
     const boost::optional<const DexType*>& annotation = boost::none) {
   state->set_type(reg, TypeDomain(IRType::INT));
   const auto anno = DexAnnoType(annotation);
-  DexTypeDomain dex_type_domain = DexTypeDomain(&anno);
+  DexTypeDomain dex_type_domain = DexTypeDomain::create_for_anno(&anno);
   state->set_dex_type(reg, dex_type_domain);
 }
 
@@ -200,15 +200,30 @@ void set_scalar(TypeEnvironment* state, reg_t reg) {
   state->reset_dex_type(reg);
 }
 
-void set_reference(
+void set_reference(TypeEnvironment* state,
+                   reg_t reg,
+                   const boost::optional<const DexType*>& dex_type_opt,
+                   bool is_not_null = false) {
+  state->set_type(reg, TypeDomain(IRType::REFERENCE));
+  auto* dex_type = dex_type_opt ? *dex_type_opt : nullptr;
+  always_assert(!is_not_null || dex_type != nullptr);
+  const DexTypeDomain dex_type_domain =
+      is_not_null ? DexTypeDomain::create_not_null(dex_type)
+                  : DexTypeDomain::create_nullable(dex_type);
+  state->set_dex_type(reg, dex_type_domain);
+}
+
+// The presence of DexAnnoType implies Nullable
+void set_reference_with_anno(
     TypeEnvironment* state,
     reg_t reg,
     const boost::optional<const DexType*>& dex_type_opt,
-    const boost::optional<const DexType*>& annotation = boost::none) {
+    const boost::optional<const DexType*>& annotation) {
   state->set_type(reg, TypeDomain(IRType::REFERENCE));
-  auto dex_type = dex_type_opt ? *dex_type_opt : nullptr;
+  auto* dex_type = dex_type_opt ? *dex_type_opt : nullptr;
   const auto anno = DexAnnoType(annotation);
-  const DexTypeDomain dex_type_domain = DexTypeDomain(dex_type, &anno);
+  const DexTypeDomain dex_type_domain =
+      DexTypeDomain::create_nullable(dex_type, &anno);
   state->set_dex_type(reg, dex_type_domain);
 }
 
@@ -338,8 +353,8 @@ const DexType* merge_dex_types(const DexTypeIt& begin,
           return t1;
         }
 
-        DexTypeDomain d1(t1);
-        DexTypeDomain d2(t2);
+        DexTypeDomain d1 = DexTypeDomain::create_nullable(t1);
+        DexTypeDomain d2 = DexTypeDomain::create_nullable(t2);
         d1.join_with(d2);
 
         auto maybe_dextype = d1.get_dex_type();
@@ -439,15 +454,15 @@ void TypeInference::refine_scalar(TypeEnvironment* state, reg_t reg) const {
               /* expected */ IRType::SCALAR);
   const boost::optional<const DexType*> annotation = state->get_annotation(reg);
   const auto anno = DexAnnoType(annotation);
-  DexTypeDomain dex_type_domain = DexTypeDomain(&anno);
+  DexTypeDomain dex_type_domain = DexTypeDomain::create_for_anno(&anno);
   state->set_dex_type(reg, dex_type_domain);
 }
 
 void TypeInference::refine_integral(TypeEnvironment* state, reg_t reg) const {
   refine_type(state, reg, /* expected */ IRType::INT);
   const boost::optional<const DexType*> annotation = state->get_annotation(reg);
   const auto anno = DexAnnoType(annotation);
-  DexTypeDomain dex_type_domain = DexTypeDomain(&anno);
+  DexTypeDomain dex_type_domain = DexTypeDomain::create_for_anno(&anno);
   state->set_dex_type(reg, dex_type_domain);
 }
 
@@ -540,12 +555,13 @@ void TypeInference::run(bool is_static,
         // If the method is not static, the first parameter corresponds to
         // `this`.
         first_param = false;
-        set_reference(&init_state, insn->dest(), declaring_type, annotation);
+        set_reference_with_anno(&init_state, insn->dest(), declaring_type,
+                                annotation);
       } else {
         // This is a regular parameter of the method.
         always_assert(sig_it != args->end());
         const DexType* type = *sig_it;
-        set_reference(&init_state, insn->dest(), type, annotation);
+        set_reference_with_anno(&init_state, insn->dest(), type, annotation);
         ++sig_it;
       }
       break;
@@ -813,12 +829,14 @@ void TypeInference::analyze_instruction(const IRInstruction* insn,
     break;
   }
   case OPCODE_NEW_INSTANCE: {
-    set_reference(current_state, RESULT_REGISTER, insn->get_type());
+    set_reference(current_state, RESULT_REGISTER, insn->get_type(),
+                  /* is_not_null */ true);
     break;
   }
   case OPCODE_NEW_ARRAY: {
     refine_int(current_state, insn->src(0));
-    set_reference(current_state, RESULT_REGISTER, insn->get_type());
+    set_reference(current_state, RESULT_REGISTER, insn->get_type(),
+                  /* is_not_null */ true);
     break;
   }
   case OPCODE_FILLED_NEW_ARRAY: {
@@ -835,7 +853,8 @@ void TypeInference::analyze_instruction(const IRInstruction* insn,
         refine_scalar(current_state, insn->src(i));
       }
     }
-    set_reference(current_state, RESULT_REGISTER, insn->get_type());
+    set_reference(current_state, RESULT_REGISTER, insn->get_type(),
+                  /* is_not_null */ true);
     break;
   }
   case OPCODE_FILL_ARRAY_DATA: {
@@ -1035,16 +1054,17 @@ void TypeInference::analyze_instruction(const IRInstruction* insn,
         get_typedef_anno_from_member(insn->get_field());
     always_assert(insn->has_field());
     const auto field = insn->get_field();
-    set_reference(current_state, RESULT_REGISTER, field->get_type(),
-                  annotation);
+    set_reference_with_anno(current_state, RESULT_REGISTER, field->get_type(),
+                            annotation);
     break;
   }
   case OPCODE_IPUT: {
     const DexType* type = insn->get_field()->get_type();
     if (!m_annotations.empty()) {
       auto annotation = current_state->get_annotation(insn->src(0));
       const DexAnnoType anno = DexAnnoType(annotation);
-      const DexTypeDomain dex_type_domain = DexTypeDomain(type, &anno);
+      const DexTypeDomain dex_type_domain =
+          DexTypeDomain::create_nullable(type, &anno);
       current_state->set_dex_type(insn->src(1), dex_type_domain);
     }
     if (type::is_float(type)) {
@@ -1086,7 +1106,8 @@ void TypeInference::analyze_instruction(const IRInstruction* insn,
       const auto anno = DexAnnoType(annotation);
       auto type = current_state->get_dex_type(insn->src(1));
       auto dex_type = type ? *type : nullptr;
-      const DexTypeDomain dex_type_domain = DexTypeDomain(dex_type, &anno);
+      const DexTypeDomain dex_type_domain =
+          DexTypeDomain::create_nullable(dex_type, &anno);
       current_state->set_dex_type(insn->src(1), dex_type_domain);
     }
     refine_reference(current_state, insn->src(0));
@@ -1236,7 +1257,8 @@ void TypeInference::analyze_instruction(const IRInstruction* insn,
     if (type::is_object(return_type)) {
       boost::optional<const DexType*> annotation =
           get_typedef_anno_from_member(dex_method);
-      set_reference(current_state, RESULT_REGISTER, return_type, annotation);
+      set_reference_with_anno(current_state, RESULT_REGISTER, return_type,
+                              annotation);
       break;
     }
     if (type::is_integral(return_type)) {

diff --git a/service/type-analysis/LocalTypeAnalyzer.cpp b/service/type-analysis/LocalTypeAnalyzer.cpp
@@ -76,13 +76,15 @@ bool RegisterTypeAnalyzer::analyze_const(const IRInstruction* insn,
 
 bool RegisterTypeAnalyzer::analyze_const_string(const IRInstruction*,
                                                 DexTypeEnvironment* env) {
-  env->set(RESULT_REGISTER, DexTypeDomain(type::java_lang_String()));
+  env->set(RESULT_REGISTER,
+           DexTypeDomain::create_not_null(type::java_lang_String()));
   return true;
 }
 
 bool RegisterTypeAnalyzer::analyze_const_class(const IRInstruction*,
                                                DexTypeEnvironment* env) {
-  env->set(RESULT_REGISTER, DexTypeDomain(type::java_lang_Class()));
+  env->set(RESULT_REGISTER,
+           DexTypeDomain::create_not_null(type::java_lang_Class()));
   return true;
 }
 
@@ -97,12 +99,7 @@ bool RegisterTypeAnalyzer::analyze_aget(const IRInstruction* insn,
   always_assert_log(type::is_array(*array_type), "Wrong array type %s in %s",
                     SHOW(*array_type), SHOW(insn));
   const auto ctype = type::get_array_component_type(*array_type);
-  auto cls = type_class(ctype);
-  bool is_type_exact = cls && !cls->is_external() && is_final(cls);
-  // is_type_exact is to decide whether to populate the
-  // small-set-dex-type-domain, which should only hold exact (non-interface)
-  // class (and possibly java.lang.Throwable, but we ignore that here).
-  env->set(RESULT_REGISTER, DexTypeDomain(ctype, NN_TOP, is_type_exact));
+  env->set(RESULT_REGISTER, DexTypeDomain::create_nullable(ctype));
   return true;
 }
 
@@ -122,26 +119,27 @@ bool RegisterTypeAnalyzer::analyze_move_exception(const IRInstruction* insn,
                                                   DexTypeEnvironment* env) {
   // We don't know where to grab the type of the just-caught exception.
   // Simply set to j.l.Throwable here.
-  env->set(insn->dest(), DexTypeDomain(type::java_lang_Throwable()));
+  env->set(insn->dest(),
+           DexTypeDomain::create_nullable(type::java_lang_Throwable()));
   return true;
 }
 
 bool RegisterTypeAnalyzer::analyze_new_instance(const IRInstruction* insn,
                                                 DexTypeEnvironment* env) {
-  env->set(RESULT_REGISTER, DexTypeDomain(insn->get_type()));
+  env->set(RESULT_REGISTER, DexTypeDomain::create_not_null(insn->get_type()));
   return true;
 }
 
 bool RegisterTypeAnalyzer::analyze_new_array(const IRInstruction* insn,
                                              DexTypeEnvironment* env) {
   // Skip array element nullness domains.
-  env->set(RESULT_REGISTER, DexTypeDomain(insn->get_type()));
+  env->set(RESULT_REGISTER, DexTypeDomain::create_not_null(insn->get_type()));
   return true;
 }
 
 bool RegisterTypeAnalyzer::analyze_filled_new_array(const IRInstruction* insn,
                                                     DexTypeEnvironment* env) {
-  env->set(RESULT_REGISTER, DexTypeDomain(insn->get_type()));
+  env->set(RESULT_REGISTER, DexTypeDomain::create_not_null(insn->get_type()));
   return true;
 }
 

diff --git a/service/type-analysis/WholeProgramState.cpp b/service/type-analysis/WholeProgramState.cpp
@@ -66,10 +66,11 @@ void set_encoded_values(const DexClass* cls, DexTypeEnvironment* env) {
       env->set(sfield, DexTypeDomain::null());
     } else if (sfield->get_type() == type::java_lang_String() &&
                value->evtype() == DEVT_STRING) {
-      env->set(sfield, DexTypeDomain(type::java_lang_String()));
+      env->set(sfield,
+               DexTypeDomain::create_not_null(type::java_lang_String()));
     } else if (sfield->get_type() == type::java_lang_Class() &&
                value->evtype() == DEVT_TYPE) {
-      env->set(sfield, DexTypeDomain(type::java_lang_Class()));
+      env->set(sfield, DexTypeDomain::create_not_null(type::java_lang_Class()));
     } else {
       env->set(sfield, DexTypeDomain::top());
     }
@@ -219,9 +220,8 @@ std::string WholeProgramState::show_method(const DexMethod* m) {
 void WholeProgramState::setup_known_method_returns() {
   for (auto& p : STATIC_METHOD_TO_TYPE_MAP) {
     auto method = DexMethod::make_method(p.first);
-    auto type =
-        DexTypeDomain(DexType::make_type(DexString::make_string(p.second)),
-                      NOT_NULL, /* is_dex_type_exact */ true);
+    auto type = DexTypeDomain::create_not_null(
+        DexType::make_type(DexString::make_string(p.second)));
     m_known_method_returns.insert(std::make_pair(method, type));
   }
 }

diff --git a/test/common/TypeAnalysisTestBase.h b/test/common/TypeAnalysisTestBase.h
@@ -47,18 +47,19 @@ class TypeAnalysisTestBase : public RedexIntegrationTest {
 
   DexTypeDomain get_type_domain(const std::string& type_name) {
     std::string full_name = "Lcom/facebook/redextest/" + type_name + ";";
-    return DexTypeDomain(DexType::make_type(DexString::make_string(full_name)));
-  }
-
-  DexTypeDomain get_type_domain_simple(const std::string& type_name) {
-    return DexTypeDomain(DexType::make_type(DexString::make_string(type_name)));
+    return DexTypeDomain::create_not_null(
+        DexType::make_type(DexString::make_string(full_name)));
   }
 
   DexTypeDomain get_type_domain_simple(const std::string& type_name,
-                                       const Nullness nullness,
-                                       bool is_dex_type_exact) {
-    return DexTypeDomain(DexType::make_type(DexString::make_string(type_name)),
-                         nullness, is_dex_type_exact);
+                                       bool is_not_null = false) {
+    if (is_not_null) {
+      return DexTypeDomain::create_not_null(
+          DexType::make_type(DexString::make_string(type_name)));
+    }
+
+    return DexTypeDomain::create_nullable(
+        DexType::make_type(DexString::make_string(type_name)));
   }
 
   DexType* get_type_simple(const std::string& type_name) {

diff --git a/test/integ/GlobalTypeAnalysisTest.cpp b/test/integ/GlobalTypeAnalysisTest.cpp
@@ -59,28 +59,28 @@ TEST_F(GlobalTypeAnalysisTest, ConstsAndAGETTest) {
 
   auto meth_pass_string = get_method("TestB;.passString", "Ljava/lang/String;",
                                      "Ljava/lang/String;");
-  EXPECT_EQ(wps.get_return_type(meth_pass_string),
-            get_type_domain_simple("Ljava/lang/String;"));
+  EXPECT_EQ(
+      wps.get_return_type(meth_pass_string),
+      get_type_domain_simple("Ljava/lang/String;", /* is_not_null */ true));
 
   auto meth_pass_class =
       get_method("TestB;.passClass", "Ljava/lang/Class;", "Ljava/lang/Class;");
-  EXPECT_EQ(wps.get_return_type(meth_pass_class),
-            get_type_domain_simple("Ljava/lang/Class;"));
+  EXPECT_EQ(
+      wps.get_return_type(meth_pass_class),
+      get_type_domain_simple("Ljava/lang/Class;", /* is_not_null */ true));
 
   auto meth_array_comp = get_method("TestB;.getStringArrayComponent",
                                     "[Ljava/lang/String;",
                                     "Ljava/lang/String;");
   EXPECT_EQ(wps.get_return_type(meth_array_comp),
-            get_type_domain_simple("Ljava/lang/String;", Nullness::NN_TOP,
-                                   /* is_dex_type_exact */ false));
+            get_type_domain_simple("Ljava/lang/String;"));
 
   auto meth_nested_array_comp =
       get_method("TestB;.getNestedStringArrayComponent",
                  "[[Ljava/lang/String;",
                  "[Ljava/lang/String;");
   EXPECT_EQ(wps.get_return_type(meth_nested_array_comp),
-            get_type_domain_simple("[Ljava/lang/String;", Nullness::NN_TOP,
-                                   /* is_dex_type_exact */ false));
+            get_type_domain_simple("[Ljava/lang/String;"));
 }
 
 TEST_F(GlobalTypeAnalysisTest, NullableFieldTypeTest) {

diff --git a/test/integ/TypeInferenceTest.cpp b/test/integ/TypeInferenceTest.cpp
@@ -256,14 +256,9 @@ TEST_F(TypeInferenceTest, test_join_with_interface) {
     }
     auto ret_type = exit_env.get_type_domain(insn->src(0));
     EXPECT_TRUE(ret_type.get_dex_type());
-    const auto& type_set = ret_type.get_type_set();
     EXPECT_EQ(*ret_type.get_dex_type(),
               DexType::get_type("Lcom/facebook/redextest/I;"));
-    EXPECT_EQ(type_set.size(), 2);
-    EXPECT_TRUE(
-        type_set.contains(DexType::get_type("Lcom/facebook/redextest/I;")));
-    EXPECT_TRUE(
-        type_set.contains(DexType::get_type("Lcom/facebook/redextest/C;")));
+    EXPECT_TRUE(ret_type.get_set_domain().is_top());
   }
 }