[opengoal] make none a child of object (#3001)

Previously, `object` and `none` were both top-level types. This made
decompilation rather messy as they have no LCA and resulted in a lot of
variables coming out as type `none` which is very very wrong and
additionally there were plenty of casts to `object`. This changes it so
`none` becomes a child of `object` (it is still represented by
`NullType` which remains unusable in compilation).

This change makes `object` the sole top-level type, and the type that
can represent *any* GOAL object. I believe this matches the original
GOAL built-in type structure. A function that has a return type of
`object` can now return an integer or a `none` at the same time.
However, keep in mind that the return value of `(none)` is still
undefined, just as before. This also makes a cast to `object`
meaningless in 90% of the situations it showed up in (as every single
thing is already an `object`) and the decompiler will no longer emit
them. Casts to `none` are also reduced. Yay!

Additionally, state handlers also don't get the final `(none)` printed
out anymore. The return type of a state handler is completely
meaningless outside the event handler (which is return type `object`
anyway) so there are no limitations on what the last form needs to be. I
did this instead of making them return `object` to trick the decompiler
into not trying to output a variable to be used as a return value
(internally, in the decompiler they still have return type `none`, but
they have `object` elsewhere).

Fixes #1703 
Fixes #830 
Fixes #928

common/type_system/Type.cpp

@@ -344,7 +344,7 @@ std::string Type::common_type_info_diff(const Type& other) const {
  * parents.
  */
 bool Type::has_parent() const {
-  return m_name != "object" && !m_parent.empty();
+  return m_name != "object";
 }
 
 /*!
@@ -485,7 +485,7 @@ std::string Type::diff(const Type& other) const {
 // Special Type for both "none" and "_type_" types
 // it's an error to try to do anything with Null.
 
-NullType::NullType(std::string name) : Type("", std::move(name), false, 0) {}
+NullType::NullType(std::string name) : Type("object", std::move(name), false, 0) {}
 
 bool NullType::is_reference() const {
   throw std::runtime_error("is_reference called on NullType");
@@ -496,7 +496,7 @@ int NullType::get_load_size() const {
 }
 
 bool NullType::get_load_signed() const {
-  throw std::runtime_error("get_load_size called on NullType");
+  throw std::runtime_error("get_load_signed called on NullType");
 }
 
 int NullType::get_size_in_memory() const {

common/type_system/Type.h

@@ -133,7 +133,7 @@ class Type {
   bool m_new_method_info_defined = false;
   bool m_generate_inspect = true;
 
-  std::string m_parent;  // the parent type (is empty for none and object)
+  std::string m_parent;  // the parent type (is empty for object)
   std::string m_name;
   bool m_allow_in_runtime = true;
   std::string m_runtime_name;

common/type_system/TypeSystem.cpp

@@ -84,7 +84,7 @@ Type* TypeSystem::add_type(const std::string& name, std::unique_ptr<Type> type)
   } else {
     // newly defined!
 
-    // none/object get to skip these checks because they are roots.
+    // objects get to skip these checks because it is the root
     if (name != "object" && name != "none" && name != "_type_" && name != "_varargs_") {
       if (m_forward_declared_types.find(type->get_parent()) != m_forward_declared_types.end()) {
         throw_typesystem_error(
@@ -1538,8 +1538,8 @@ void TypeSystem::builtin_structure_inherit(StructureType* st) {
   st->inherit(get_type_of_type<StructureType>(st->get_parent()));
 }
 
-bool TypeSystem::tc(const TypeSpec& expected, const TypeSpec& actual) const {
-  return typecheck_and_throw(expected, actual, "", false, false);
+bool TypeSystem::tc(const TypeSpec& less_specific, const TypeSpec& more_specific) const {
+  return typecheck_and_throw(less_specific, more_specific, "", false, false);
 }
 
 /*!
@@ -1715,10 +1715,6 @@ std::string TypeSystem::lca_base(const std::string& a, const std::string& b) con
     return a;
   }
 
-  if (a == "none" || b == "none") {
-    return "none";
-  }
-
   auto a_up = get_path_up_tree(a);
   auto b_up = get_path_up_tree(b);
 

common/type_system/TypeSystem.h

@@ -228,7 +228,7 @@ class TypeSystem {
                            bool print_on_error = true,
                            bool throw_on_error = true,
                            bool allow_type_alias = false) const;
-  bool tc(const TypeSpec& expected, const TypeSpec& actual) const;
+  bool tc(const TypeSpec& less_specific, const TypeSpec& more_specific) const;
   std::vector<std::string> get_path_up_tree(const std::string& type) const;
   int get_next_method_id(const Type* type) const;
 

common/type_system/state.cpp

@@ -68,8 +68,6 @@ TypeSpec get_state_handler_type(StateHandler kind, const TypeSpec& state_type) {
   TypeSpec result;
   switch (kind) {
     case StateHandler::CODE:
-      result = state_to_go_function(state_type, TypeSpec("none"));
-      break;
     case StateHandler::ENTER:
       result = state_to_go_function(state_type, TypeSpec("none"));
       break;
@@ -102,7 +100,7 @@ std::vector<std::string> get_state_handler_arg_names(StateHandler kind) {
     case StateHandler::EXIT:
       return {};
     case StateHandler::EVENT:
-      return {"proc", "arg1", "event-type", "event"};
+      return {"proc", "argc", "message", "block"};
     default:
       ASSERT(false);
   }

decompiler/Function/Function.h

@@ -188,6 +188,7 @@ class Function {
     std::string debug_form_string;
     bool print_debug_forms = false;
     bool expressions_succeeded = false;
+    bool skip_final_none = false;
   } ir2;
 
   std::optional<std::string> mips2c_output;

decompiler/IR2/Form.cpp

@@ -2962,7 +2962,7 @@ goos::Object DefstateElement::to_form_internal(const Env& env) const {
 }
 
 ////////////////////////////////
-// DefskelgroupElement
+// WithDmaBufferAddBucketElement
 ////////////////////////////////
 
 WithDmaBufferAddBucketElement::WithDmaBufferAddBucketElement(RegisterAccess dma_buf,

decompiler/analysis/expression_build.cpp

@@ -91,21 +91,31 @@ bool convert_to_expressions(
         needs_cast = true;
 
       } else {
-        bool found_early_return = false;
-        for (auto e : new_entries) {
-          e->apply([&](FormElement* elt) {
-            auto as_ret = dynamic_cast<ReturnElement*>(elt);
-            if (as_ret) {
-              found_early_return = true;
+        // note : a return type of "object" will accept ANYTHING as a return value
+        // "object" is the parent type of everything, including "none" (as of sep 2023)
+        // if a function wants to return an object, we can safely discard the cast
+        // since there is no possible level of polymorphism at this highest level.
+        if (f.type.last_arg() != TypeSpec("object")) {
+          bool found_early_return = false;
+          for (auto e : new_entries) {
+            e->apply([&](FormElement* elt) {
+              auto as_ret = dynamic_cast<ReturnElement*>(elt);
+              if (as_ret) {
+                found_early_return = true;
+              }
+            });
+            if (found_early_return) {
+              break;
             }
-          });
-          if (found_early_return) {
-            break;
           }
-        }
 
-        if (!found_early_return && f.type.last_arg() != return_type) {
-          needs_cast = true;
+          // the return value of this function is not an exact match
+          // we cast it to avoid complicated issues with polymorphism (e.g. methods)
+          // we don't run this if we find a (return statement because we do not handle
+          // type checking on those at the moment.
+          if (!found_early_return && f.type.last_arg() != return_type) {
+            needs_cast = true;
+          }
         }
       }
 
@@ -124,8 +134,10 @@ bool convert_to_expressions(
     } else {
       // or just get all the expressions
       new_entries = stack.rewrite(pool, f.ir2.env);
-      new_entries.push_back(
-          pool.alloc_element<GenericElement>(GenericOperator::make_fixed(FixedOperatorKind::NONE)));
+      if (!f.ir2.skip_final_none) {
+        new_entries.push_back(pool.alloc_element<GenericElement>(
+            GenericOperator::make_fixed(FixedOperatorKind::NONE)));
+      }
     }
 
     // if we are a totally empty function, insert a placeholder so we don't have to handle

decompiler/analysis/find_defstates.cpp

@@ -146,6 +146,9 @@ std::vector<DefstateElement::Entry> get_defstate_entries(
 
       // scary part - modify the function type!
       handler_func->type = get_state_handler_type(handler_kind, state_type);
+      // hack - lets pretend every handler (except event) returns none but remove the (none) at the
+      // end since the 'real' return type is object and thus anything is valid in the final form
+      handler_func->ir2.skip_final_none = true;
     } else if (handler_atom && handler_atom->is_sym_val()) {
       auto sym_type = env.dts->lookup_symbol_type(handler_atom->get_str());
       auto expected_type = get_state_handler_type(handler_kind, state_type);

decompiler/analysis/insert_lets.cpp

@@ -1147,13 +1147,13 @@ FormElement* rewrite_as_case_with_else(LetElement* in, const Env& env, FormPool&
   return pool.alloc_element<CaseElement>(in->entries().at(0).src, entries, cond->else_ir);
 }
 
-bool var_equal(const Env& env, const RegisterAccess& a, std::optional<RegisterAccess> b) {
+bool var_equal(const Env& env, RegisterAccess a, std::optional<RegisterAccess> b) {
   ASSERT(b);
   return env.get_variable_name_name_only(*b) == env.get_variable_name_name_only(a);
 }
 
 Form* match_ja_set(const Env& env,
-                   const RegisterAccess& ch_var,
+                   RegisterAccess ch_var,
                    const std::string& field_name,
                    int arr_idx,
                    Form* in,
@@ -1712,8 +1712,9 @@ FormElement* rewrite_attack_info(LetElement* in, const Env& env, FormPool& pool)
   enum AttackInfoFieldKind { DEFAULT, VECTOR, METERS, DEGREES };
 
   const static std::map<std::string, std::pair<int, AttackInfoFieldKind>> possible_args_jak1 = {
-      {"vector", {1, VECTOR}},    {"mode", {5, DEFAULT}},     {"shove-back", {6, DEFAULT}},
-      {"shove-up", {7, DEFAULT}}, {"control", {10, DEFAULT}}, {"angle", {11, DEFAULT}},
+      {"vector", {1, VECTOR}},      {"attacker", {3, DEFAULT}}, {"mode", {5, DEFAULT}},
+      {"shove-back", {6, DEFAULT}}, {"shove-up", {7, DEFAULT}}, {"control", {10, DEFAULT}},
+      {"angle", {11, DEFAULT}},
   };
   const static std::map<std::string, std::pair<int, AttackInfoFieldKind>> possible_args_jak2 = {
       {"vector", {1, VECTOR}},

decompiler/analysis/variable_naming.cpp

@@ -655,34 +655,23 @@ namespace {
 
 TP_Type lca_for_var_types(const TP_Type& existing,
                           const TP_Type& add,
-                          const DecompilerTypeSystem& dts,
-                          bool event_handler_hack) {
+                          const DecompilerTypeSystem& dts) {
   bool changed;
   auto normal = dts.tp_lca(existing, add, &changed);
-  if (!event_handler_hack || normal.typespec().base_type() != "none") {
-    return normal;
-  }
-  if (existing.typespec().base_type() == "none") {
-    return add;
-  } else if (add.typespec().base_type() == "none") {
-    return existing;
-  } else {
-    return normal;
-  }
+  return normal;
 }
 
 void update_var_info(VariableNames::VarInfo* info,
                      Register reg,
                      const TypeState& ts,
                      int var_id,
-                     const DecompilerTypeSystem& dts,
-                     bool event_handler_hack) {
+                     const DecompilerTypeSystem& dts) {
   auto& type = ts.get(reg);
   if (info->initialized) {
     ASSERT(info->reg_id.id == var_id);
     ASSERT(info->reg_id.reg == reg);
 
-    info->type = lca_for_var_types(info->type, type, dts, event_handler_hack);
+    info->type = lca_for_var_types(info->type, type, dts);
 
   } else {
     info->reg_id.id = var_id;
@@ -695,10 +684,9 @@ void update_var_info(VariableNames::VarInfo* info,
 
 bool merge_infos(VariableNames::VarInfo* info1,
                  VariableNames::VarInfo* info2,
-                 const DecompilerTypeSystem& dts,
-                 bool event_handler_hack) {
+                 const DecompilerTypeSystem& dts) {
   if (info1->initialized && info2->initialized) {
-    auto new_type = lca_for_var_types(info1->type, info2->type, dts, event_handler_hack);
+    auto new_type = lca_for_var_types(info1->type, info2->type, dts);
 
     info1->type = new_type;
     info2->type = new_type;
@@ -710,13 +698,12 @@ bool merge_infos(VariableNames::VarInfo* info1,
 void merge_infos(
     std::unordered_map<Register, std::vector<VariableNames::VarInfo>, Register::hash>& info1,
     std::unordered_map<Register, std::vector<VariableNames::VarInfo>, Register::hash>& info2,
-    const DecompilerTypeSystem& dts,
-    bool event_handler_hack) {
+    const DecompilerTypeSystem& dts) {
   for (auto& [reg, infos] : info1) {
     auto other = info2.find(reg);
     if (other != info2.end()) {
       for (size_t i = 0; i < std::min(other->second.size(), infos.size()); i++) {
-        merge_infos(&infos.at(i), &other->second.at(i), dts, event_handler_hack);
+        merge_infos(&infos.at(i), &other->second.at(i), dts);
       }
     }
   }
@@ -730,28 +717,6 @@ void merge_infos(
  * the none variables
  */
 void SSA::make_vars(const Function& function, const DecompilerTypeSystem& dts) {
-  bool event_handler_hack = false;
-
-  if (function.ir2.env.version == GameVersion::Jak2) {
-    event_handler_hack = function.guessed_name.is_event_handler() ||
-                         function.guessed_name.to_string() == "target-generic-event-handler" ||
-                         function.guessed_name.to_string() == "target-standard-event-handler" ||
-                         function.guessed_name.to_string() == "target-board-handler" ||
-                         function.guessed_name.to_string() == "(method 74 pegasus)" ||
-                         function.guessed_name.to_string() == "(method 74 crimson-guard-level)" ||
-                         function.guessed_name.to_string() == "widow-handler" ||
-                         function.guessed_name.to_string() == "(method 74 hal)" ||
-                         function.guessed_name.to_string() == "water-anim-event-handler" ||
-                         function.guessed_name.to_string() == "(method 74 civilian)" ||
-                         function.guessed_name.to_string() == "(method 74 crimson-guard)" ||
-                         function.guessed_name.to_string() == "metalkor-handler";
-  }
-
-  if (function.ir2.env.version == GameVersion::Jak1) {
-    event_handler_hack = function.guessed_name.is_event_handler() ||
-                         function.guessed_name.to_string() == "target-generic-event-handler";
-  }
-
   for (int block_id = 0; block_id < int(blocks.size()); block_id++) {
     const auto& block = blocks.at(block_id);
     const TypeState* init_types = &function.ir2.env.get_types_at_block_entry(block_id);
@@ -766,13 +731,13 @@ void SSA::make_vars(const Function& function, const DecompilerTypeSystem& dts) {
       if (instr.dst.has_value()) {
         auto var_id = map.var_id(*instr.dst);
         auto* info = &program_write_vars[instr.dst->reg()].at(var_id);
-        update_var_info(info, instr.dst->reg(), *end_types, var_id, dts, event_handler_hack);
+        update_var_info(info, instr.dst->reg(), *end_types, var_id, dts);
       }
 
       for (auto& src : instr.src) {
         auto var_id = map.var_id(src);
         auto* info = &program_read_vars[src.reg()].at(var_id);
-        update_var_info(info, src.reg(), *init_types, var_id, dts, event_handler_hack);
+        update_var_info(info, src.reg(), *init_types, var_id, dts);
       }
 
       init_types = end_types;
@@ -793,7 +758,7 @@ void SSA::make_vars(const Function& function, const DecompilerTypeSystem& dts) {
     }
   }
 
-  merge_infos(program_write_vars, program_read_vars, dts, event_handler_hack);
+  merge_infos(program_write_vars, program_read_vars, dts);
 
   // copy types from input argument coloring moves:
   for (auto& instr : blocks.at(0).ins) {