incomplete: name type in compiler errors

Summary:

We have a good type representation in the Type Graph of an incomplete type and
the underlying type that represents. However, this incomplete type still ends
up in the generated code as `void` which loses information. For example, a
container that can't contain void may fail to compile because it was
initialised with `void` but really its because the type it was supposed to be
initialised with (say, `Foo`) had incomplete debug information.

This change identifies that a type is incomplete in the output by generating it
as an incomplete type `struct Incomplete<struct Foo>`. This allows us to name
the type correctly in the TreeBuilder output and filter for incomplete types,
as well as getting appropriate compiler errors if it mustn't be incomplete.

Test Plan:
- CI
- Added a unit test to namegen.
- Enabled and added an extra pointers_incomplete test.

This change is tricky to test because it isn't really user visible. The types
still use their `inputName` which is unchanged in any successful output - this
change is used so the compiler fails with a more detailed error.

oi/CodeGen.cpp

@@ -60,6 +60,7 @@ using type_graph::EnforceCompatibility;
 using type_graph::Enum;
 using type_graph::Flattener;
 using type_graph::IdentifyContainers;
+using type_graph::Incomplete;
 using type_graph::KeyCapture;
 using type_graph::Member;
 using type_graph::NameGen;
@@ -415,8 +416,6 @@ void addStandardGetSizeFuncDecls(std::string& code) {
     template<typename T>
     void getSizeType(/*const*/ T* s_ptr, size_t& returnArg);
 
-    void getSizeType(/*const*/ void *s_ptr, size_t& returnArg);
-
     template <typename T, int N>
     void getSizeType(const OIArray<T,N>& container, size_t& returnArg);
   )";
@@ -438,6 +437,12 @@ void addStandardGetSizeFuncDefs(std::string& code) {
     template<typename T>
     void getSizeType(/*const*/ T* s_ptr, size_t& returnArg)
     {
+      if constexpr (!oi_is_complete<T>) {
+        JLOG("incomplete ptr @");
+        JLOGPTR(s_ptr);
+        StoreData((uintptr_t)(s_ptr), returnArg);
+        return;
+      } else {
         JLOG("ptr val @");
         JLOGPTR(s_ptr);
         StoreData((uintptr_t)(s_ptr), returnArg);
@@ -448,12 +453,6 @@ void addStandardGetSizeFuncDefs(std::string& code) {
           StoreData(0, returnArg);
         }
       }
-
-    void getSizeType(/*const*/ void *s_ptr, size_t& returnArg)
-    {
-      JLOG("void ptr @");
-      JLOGPTR(s_ptr);
-      StoreData((uintptr_t)(s_ptr), returnArg);
     }
 
     template <typename T, int N>

oi/OITraceCode.cpp

@@ -182,3 +182,19 @@ bool isStorageInline(const auto& c) {
   return (uintptr_t)std::data(c) < (uintptr_t)(&c + sizeof(c)) &&
          (uintptr_t)std::data(c) >= (uintptr_t)&c;
 }
+
+namespace OIInternal {
+namespace {
+
+template <typename T>
+struct Incomplete;
+
+template <typename T>
+constexpr bool oi_is_complete = true;
+template <>
+constexpr bool oi_is_complete<void> = false;
+template <typename T>
+constexpr bool oi_is_complete<Incomplete<T>> = false;
+
+}  // namespace
+}  // namespace OIInternal

oi/type_graph/DrgnExporter.cpp

@@ -100,6 +100,8 @@ drgn_type* DrgnExporter::visit(Container& c) {
     if (auto* p = dynamic_cast<const Primitive*>(&paramType);
         p && p->kind() == Primitive::Kind::Void) {
       return drgnType;
+    } else if (auto* i = dynamic_cast<const Incomplete*>(&paramType)) {
+      return drgnType;
     }
   }
 

oi/type_graph/NameGen.cpp

@@ -219,4 +219,27 @@ void NameGen::visit(CaptureKeys& c) {
   c.regenerateName();
 }
 
+void NameGen::visit(Incomplete& i) {
+  constexpr std::string_view kPrefix{"Incomplete<struct "};
+
+  RecursiveVisitor::visit(i);
+
+  std::string_view inputName = i.inputName();
+
+  std::string name;
+  name.reserve(kPrefix.size() + inputName.size() + 2);
+  name = kPrefix;
+
+  for (const unsigned char c : inputName) {
+    if (std::isalnum(c)) {
+      name += c;
+    } else {
+      name += '_';
+    }
+  }
+  name += ">";
+
+  i.setName(name);
+}
+
 }  // namespace oi::detail::type_graph

oi/type_graph/NameGen.h

@@ -49,6 +49,7 @@ class NameGen final : public RecursiveVisitor {
   void visit(Reference& r) override;
   void visit(DummyAllocator& d) override;
   void visit(CaptureKeys& d) override;
+  void visit(Incomplete& i) override;
 
   static const inline std::string AnonPrefix = "__oi_anon";
 

oi/type_graph/Printer.cpp

@@ -37,7 +37,9 @@ void Printer::print(const Type& type) {
 }
 
 void Printer::visit(const Incomplete& i) {
-  prefix();
+  if (prefix(i))
+    return;
+
   out_ << "Incomplete";
   if (auto underlyingType = i.underlyingType()) {
     out_ << std::endl;

oi/type_graph/TopoSorter.cpp

@@ -139,6 +139,10 @@ void TopoSorter::visit(CaptureKeys& c) {
   sortedTypes_.push_back(c);
 }
 
+void TopoSorter::visit(Incomplete& i) {
+  sortedTypes_.push_back(i);
+}
+
 /*
  * A type graph may contain cycles, so we need to slightly tweak the standard
  * topological sorting algorithm. Cycles can only be introduced by certain

oi/type_graph/TopoSorter.h

@@ -48,6 +48,7 @@ class TopoSorter : public RecursiveVisitor {
   void visit(Pointer& p) override;
   void visit(Primitive& p) override;
   void visit(CaptureKeys& p) override;
+  void visit(Incomplete& i) override;
 
  private:
   std::unordered_set<Type*> visited_;

oi/type_graph/Types.cpp

@@ -35,8 +35,6 @@ namespace oi::detail::type_graph {
 OI_TYPE_LIST
 #undef X
 
-const std::string Incomplete::kName = "void";
-
 std::string Primitive::getName(Kind kind) {
   switch (kind) {
     case Kind::Int8:

oi/type_graph/Types.h

@@ -29,6 +29,8 @@
  * debugging.
  */
 
+#include <algorithm>
+#include <cctype>
 #include <cstddef>
 #include <map>
 #include <optional>
@@ -211,29 +213,33 @@ struct TemplateParam {
  */
 class Incomplete : public Type {
  public:
-  Incomplete(Type& underlyingType) : underlyingType_(underlyingType) {
+  Incomplete(NodeId id, Type& underlyingType)
+      : id_(id), underlyingType_(underlyingType) {
   }
 
-  Incomplete(std::string underlyingTypeName)
+  Incomplete(NodeId id, std::string underlyingTypeName)
-      : underlyingType_(std::move(underlyingTypeName)) {
+      : id_(id), underlyingType_(std::move(underlyingTypeName)) {
   }
 
-  static inline constexpr bool has_node_id = false;
+  static inline constexpr bool has_node_id = true;
 
   DECLARE_ACCEPT
 
   const std::string& name() const override {
-    return kName;
+    return name_;
   }
 
   std::string_view inputName() const override {
-    if (std::holds_alternative<std::string>(underlyingType_)) {
+    return std::visit(
-      return std::get<std::string>(underlyingType_);
+        [](const auto& el) -> std::string_view {
+          using T = std::decay_t<decltype(el)>;
+          if constexpr (std::is_same_v<T, std::string>) {
+            return el;
+          } else {
+            return el.get().inputName();
           }
-
+        },
-    return std::get<std::reference_wrapper<Type>>(underlyingType_)
+        underlyingType_);
-        .get()
-        .inputName();
   }
 
   size_t size() const override {
@@ -245,7 +251,11 @@ class Incomplete : public Type {
   }
 
   NodeId id() const override {
-    return -1;
+    return id_;
+  }
+
+  void setName(std::string name) {
+    name_ = std::move(name);
   }
 
   std::optional<std::reference_wrapper<Type>> underlyingType() const {
@@ -257,8 +267,9 @@ class Incomplete : public Type {
   }
 
  private:
+  NodeId id_ = -1;
   std::variant<std::string, std::reference_wrapper<Type>> underlyingType_;
-  static const std::string kName;
+  std::string name_ = "void";
 };
 
 /*

test/TypeGraphParser.cpp

@@ -215,10 +215,11 @@ Type& TypeGraphParser::parseType(std::string_view& input, size_t rootIndent) {
       auto nameEndPos = line.find(']', nameStartPos);
       auto underlyingTypeName =
           line.substr(nameStartPos, nameEndPos - nameStartPos);
-      type = &typeGraph_.makeType<Incomplete>(std::string(underlyingTypeName));
+      type =
+          &typeGraph_.makeType<Incomplete>(id, std::string(underlyingTypeName));
     } else {
       auto& underlyingType = parseType(input, indent + 2);
-      type = &typeGraph_.makeType<Incomplete>(underlyingType);
+      type = &typeGraph_.makeType<Incomplete>(id, underlyingType);
     }
   } else if (nodeTypeName == "Class" || nodeTypeName == "Struct" ||
              nodeTypeName == "Union") {

test/integration/pointers_incomplete.toml

@@ -55,7 +55,6 @@ definitions = '''
     }]'''
 
   [cases.unique_ptr]
-    oil_skip = 'not implemented for treebuilder v2' # https://github.com/facebookexperimental/object-introspection/issues/299
     param_types = ["const std::unique_ptr<IncompleteType, decltype(&incomplete_type_deleter)>&"]
     setup = '''
       auto raw_ptr = static_cast<IncompleteType*>(::operator new(5));
@@ -63,28 +62,29 @@ definitions = '''
         raw_ptr, &incomplete_type_deleter);
     '''
     expect_json = '[{"staticSize":16, "dynamicSize":0, "NOT":"members"}]'
+    expect_json_v2 = '[{"staticSize":16, "exclusiveSize":16}]'
   [cases.unique_ptr_null]
-    oil_skip = 'not implemented for treebuilder v2' # https://github.com/facebookexperimental/object-introspection/issues/299
     param_types = ["const std::unique_ptr<IncompleteType, decltype(&incomplete_type_deleter)>&"]
     setup = '''
       return std::unique_ptr<IncompleteType, decltype(&incomplete_type_deleter)>(
         nullptr, &incomplete_type_deleter);
     '''
     expect_json = '[{"staticSize":16, "dynamicSize":0, "NOT":"members"}]'
+    expect_json_v2 = '[{"staticSize":16, "exclusiveSize":16}]'
 
   [cases.shared_ptr]
-    oil_skip = 'not implemented for treebuilder v2' # https://github.com/facebookexperimental/object-introspection/issues/300
     param_types = ["const std::shared_ptr<IncompleteType>&"]
     setup = '''
       auto raw_ptr = static_cast<IncompleteType*>(::operator new(5));
       return std::shared_ptr<IncompleteType>(raw_ptr , &incomplete_type_deleter);
     '''
     expect_json = '[{"staticSize":16, "dynamicSize":0, "NOT":"members"}]'
+    expect_json_v2 = '[{"staticSize":16, "exclusiveSize":16}]'
   [cases.shared_ptr_null]
-    oil_skip = 'not implemented for treebuilder v2' # https://github.com/facebookexperimental/object-introspection/issues/300
     param_types = ["const std::shared_ptr<IncompleteType>"]
     setup = "return nullptr;"
     expect_json = '[{"staticSize":16, "dynamicSize":0, "NOT":"members"}]'
+    expect_json_v2 = '[{"staticSize":16, "exclusiveSize":16}]'
 
   [cases.containing_struct]
     oil_disable = "oil can't chase raw pointers safely"
@@ -116,3 +116,27 @@ definitions = '''
         }
       ]
     }]'''
+
+  [cases.containing_struct_no_follow]
+    param_types = ["const IncompleteTypeContainer&"]
+    setup = "return IncompleteTypeContainer{};"
+    expect_json = '''[{
+      "staticSize": 88,
+      "members": [
+        { "name": "ptrundef", "staticSize": 8 },
+        { "name": "__makePad1", "staticSize": 1 },
+        { "name": "shundef", "staticSize": 16 },
+        { "name": "__makePad2", "staticSize": 1 },
+        { "name": "shoptundef",
+          "staticSize": 24,
+          "length": 0,
+          "capacity": 1
+        },
+        { "name": "__makePad3", "staticSize": 1 },
+        { "name": "optundef",
+          "staticSize": 16,
+          "length": 0,
+          "capacity": 1
+        }
+      ]
+    }]'''

test/test_drgn_parser.cpp

@@ -426,8 +426,8 @@ TEST_F(DrgnParserTest, PointerNoFollow) {
 
 TEST_F(DrgnParserTest, PointerIncomplete) {
   test("oid_test_case_pointers_incomplete_raw", R"(
-[0] Pointer
+[1] Pointer
-      Incomplete: [IncompleteType]
+[0]   Incomplete: [IncompleteType]
 )");
 }
 

test/test_flattener.cpp

@@ -1007,7 +1007,7 @@ TEST(FlattenerTest, IncompleteParent) {
        R"(
 [0] Class: MyClass (size: 4)
       Parent (offset: 0)
-        Incomplete: [IncompleteParent]
+[1]     Incomplete: [IncompleteParent]
 )",
        R"(
 [0] Class: MyClass (size: 4)

test/test_name_gen.cpp

@@ -501,3 +501,16 @@ TEST(NameGenTest, AnonymousMembers) {
   EXPECT_EQ(myclass.members[0].inputName, "__oi_anon_0");
   EXPECT_EQ(myclass.members[1].inputName, "__oi_anon_1");
 }
+
+TEST(NameGenTest, IncompleteTypes) {
+  auto myincompletevector = Incomplete{0, "std::vector<int>"};
+
+  auto myint = Primitive{Primitive::Kind::Int32};
+  auto myincompleteint = Incomplete{1, myint};
+
+  NameGen nameGen;
+  nameGen.generateNames({myincompletevector, myincompleteint});
+
+  EXPECT_EQ(myincompletevector.name(), "Incomplete<struct std__vector_int_>");
+  EXPECT_EQ(myincompleteint.name(), "Incomplete<struct int32_t>");
+}

test/test_remove_members.cpp

@@ -191,7 +191,7 @@ TEST(RemoveMembersTest, Incomplete) {
       Member: a (offset: 0)
         Primitive: int32_t
       Member: b (offset: 4)
-        Incomplete: [MyIncompleteType]
+[1]     Incomplete: [MyIncompleteType]
       Member: c (offset: 8)
         Primitive: int32_t
 )",

types/shrd_ptr_type.toml

@@ -21,7 +21,7 @@ void getSizeType(const %1%<T> &s_ptr, size_t& returnArg)
 {
     SAVE_SIZE(sizeof(%1%<T>));
 
-    if constexpr (!std::is_void<T>::value) {
+    if constexpr (oi_is_complete<T>) {
         SAVE_DATA((uintptr_t)(s_ptr.get()));
 
         if (s_ptr && ctx.pointers.add((uintptr_t)(s_ptr.get()))) {
@@ -37,7 +37,7 @@ void getSizeType(const %1%<T> &s_ptr, size_t& returnArg)
 traversal_func = """
 auto tail = returnArg.write((uintptr_t)container.get());
 
-if constexpr (std::is_void<T0>::value) {
+if constexpr (!oi_is_complete<T0>) {
   return tail.template delegate<0>(std::identity());
 } else {
   bool do_visit = container && ctx.pointers.add((uintptr_t)container.get());
@@ -85,7 +85,7 @@ el.container_stats.emplace(result::Element::ContainerStats {
 });
 
 // Must be in a `if constexpr` or the compiler will complain about make_field<Ctx, void>
-if constexpr (!std::is_void<T0>::value) {
+if constexpr (oi_is_complete<T0>) {
   if (sum.index == 1) {
     static constexpr auto element = make_field<Ctx, T0>("ptr_val");
     stack_ins(element);

types/uniq_ptr_type.toml

@@ -22,7 +22,7 @@ void getSizeType(const %1%<T,Deleter> &u_ptr, size_t& returnArg)
 {
     SAVE_SIZE(sizeof(%1%<T,Deleter>));
 
-    if constexpr (!std::is_void<T>::value) {
+    if constexpr (oi_is_complete<T>) {
         SAVE_DATA((uintptr_t)(u_ptr.get()));
 
         if (u_ptr && ctx.pointers.add((uintptr_t)(u_ptr.get()))) {
@@ -38,7 +38,7 @@ void getSizeType(const %1%<T,Deleter> &u_ptr, size_t& returnArg)
 traversal_func = """
 auto tail = returnArg.write((uintptr_t)container.get());
 
-if constexpr (std::is_void<T0>::value) {
+if constexpr (!oi_is_complete<T0>) {
   return tail.template delegate<0>(std::identity());
 } else {
   bool do_visit = container && ctx.pointers.add((uintptr_t)container.get());
@@ -71,7 +71,7 @@ el.container_stats.emplace(result::Element::ContainerStats {
 });
 
 // Must be in a `if constexpr` or the compiler will complain about make_field<Ctx, void>
-if constexpr (!std::is_void<T0>::value) {
+if constexpr (oi_is_complete<T0>) {
   if (sum.index == 1) {
     static constexpr auto element = make_field<Ctx, T0>("ptr_val");
     stack_ins(element);