Unnamed unions WIP. About to change MemberInfo format, which will require…

Unnamed unions WIP.  About to change MemberInfo format, which will require re-bootstrapping the code generator.

c++/src/capnp/compiler/capnpc-c++.c++

@@ -133,7 +133,10 @@ void makeSubMemberInfoTable(const StructSchema::Member& member,
     case schema::StructNode::Member::Body::FIELD_MEMBER:
       break;
     case schema::StructNode::Member::Body::UNION_MEMBER:
-      makeMemberInfoTable(1 + member.getIndex(), member.asUnion().getMembers(), info);
+      // Only create a sub-table if the union is named.
+      if (member.getProto().getName().size() > 0) {
+        makeMemberInfoTable(1 + member.getIndex(), member.asUnion().getMembers(), info);
+      }
       break;
     case schema::StructNode::Member::Body::GROUP_MEMBER:
       makeMemberInfoTable(1 + member.getIndex(), member.asGroup().getMembers(), info);
@@ -145,9 +148,40 @@ void makeSubMemberInfoTable(const EnumSchema::Enumerant& member,
 void makeSubMemberInfoTable(const InterfaceSchema::Method& member,
                             kj::Vector<capnp::_::RawSchema::MemberInfo>& info) {}
 
+void enumerateScope(const StructSchema::MemberList& members,
+                    kj::Vector<StructSchema::Member>& vec) {
+  // Given a member list, flatten all members of the scope into one vector.  This basically means
+  // copying all the members to the vector, except that unnamed unions are flattened.
+
+  for (auto member: members) {
+    vec.add(member);
+    if (member.getProto().getName().size() == 0) {
+      // Flatten unnamed union.
+      enumerateScope(member.asUnion().getMembers(), vec);
+    }
+  }
+}
+
+void enumerateScope(const EnumSchema::EnumerantList& members,
+                    kj::Vector<EnumSchema::Enumerant>& vec) {
+  for (auto member: members) {
+    vec.add(member);
+  }
+}
+
+void enumerateScope(const InterfaceSchema::MethodList& members,
+                    kj::Vector<InterfaceSchema::Method>& vec) {
+  for (auto member: members) {
+    vec.add(member);
+  }
+}
+
 template <typename MemberList>
 void makeMemberInfoTable(uint parent, MemberList&& members,
                          kj::Vector<capnp::_::RawSchema::MemberInfo>& info) {
+  kj::Vector<kj::Decay<decltype(members[0])>> sortedMembers(members.size());
+  enumerateScope(members, sortedMembers);
+
   auto sorted = KJ_MAP(members, m) { return m; };
   std::sort(sorted.begin(), sorted.end(), OrderByName());
 
@@ -408,17 +442,25 @@ private:
     kj::String unionSet, unionCheck;
     KJ_IF_MAYBE(u, member.getContainingUnion()) {
       auto unionProto = u->getProto();
-      kj::String unionTitleCase = toTitleCase(unionProto.getName());
+      kj::StringPtr unionScope;
+      kj::String ownUnionScope;
+      if (unionProto.getName().size() > 0) {
+        ownUnionScope = kj::str(toTitleCase(unionProto.getName()), "::");
+        unionScope = ownUnionScope;
+      } else {
+        // Anonymous union.
+        unionScope = scope;
+      }
       auto discrimOffset = unionProto.getBody().getUnionMember().getDiscriminantOffset();
 
       kj::String upperCase = toUpperCase(proto.getName());
       unionCheck = kj::str(
-          "  KJ_IREQUIRE(which() == ", unionTitleCase, "::", upperCase, ",\n"
+          "  KJ_IREQUIRE(which() == ", unionScope, upperCase, ",\n"
           "              \"Must check which() before get()ing a union member.\");\n");
       unionSet = kj::str(
-          "  _builder.setDataField<", unionTitleCase, "::Which>(\n"
+          "  _builder.setDataField<", unionScope, "Which>(\n"
           "      ", discrimOffset, " * ::capnp::ELEMENTS, ",
-                    unionTitleCase, "::", upperCase, ");\n");
+                    unionScope, upperCase, ");\n");
     }
 
     uint offset = field.getOffset();
@@ -804,10 +846,10 @@ private:
             kj::mv(subText.innerTypeReaderBuilderDefs),
 
             kj::strTree(
-                "inline Which which() const;\n",
+                "  inline Which which() const;\n",
                 kj::mv(subText.readerMethodDecls)),
             kj::strTree(
-                "inline Which which();\n",
+                "  inline Which which();\n",
                 kj::mv(subText.builderMethodDecls)),
             kj::strTree(
                 "inline ", containingType, "::Which ", containingType, "::Reader::which() const {\n"

c++/src/capnp/compiler/node-translator.c++

@@ -543,8 +543,20 @@ schema::Node::Reader NodeTranslator::finish() {
   return wipNode.getReader();
 }
 
+class NodeTranslator::DuplicateNameDetector {
+public:
+  inline explicit DuplicateNameDetector(const ErrorReporter& errorReporter)
+      : errorReporter(errorReporter) {}
+  void check(List<Declaration>::Reader nestedDecls, Declaration::Body::Which parentKind);
+
+private:
+  const ErrorReporter& errorReporter;
+  std::map<kj::StringPtr, LocatedText::Reader> names;
+};
+
 void NodeTranslator::compileNode(Declaration::Reader decl, schema::Node::Builder builder) {
-  checkMembers(decl.getNestedDecls(), decl.getBody().which());
+  DuplicateNameDetector(errorReporter)
+      .check(decl.getNestedDecls(), decl.getBody().which());
 
   kj::StringPtr targetsFlagName;
 
@@ -585,21 +597,25 @@ void NodeTranslator::compileNode(Declaration::Reader decl, schema::Node::Builder
   builder.adoptAnnotations(compileAnnotationApplications(decl.getAnnotations(), targetsFlagName));
 }
 
-void NodeTranslator::checkMembers(
+void NodeTranslator::DuplicateNameDetector::check(
     List<Declaration>::Reader nestedDecls, Declaration::Body::Which parentKind) {
-  std::map<uint, Declaration::Reader> ordinals;
-  std::map<kj::StringPtr, LocatedText::Reader> names;
-
   for (auto decl: nestedDecls) {
     {
       auto name = decl.getName();
       auto nameText = name.getValue();
       auto insertResult = names.insert(std::make_pair(nameText, name));
       if (!insertResult.second) {
-        errorReporter.addErrorOn(
-            name, kj::str("'", nameText, "' is already defined in this scope."));
-        errorReporter.addErrorOn(
-            insertResult.first->second, kj::str("'", nameText, "' previously defined here."));
+        if (nameText.size() == 0 && decl.getBody().which() == Declaration::Body::UNION_DECL) {
+          errorReporter.addErrorOn(
+              name, kj::str("An unnamed union is already defined in this scope."));
+          errorReporter.addErrorOn(
+              insertResult.first->second, kj::str("Previously defined here."));
+        } else {
+          errorReporter.addErrorOn(
+              name, kj::str("'", nameText, "' is already defined in this scope."));
+          errorReporter.addErrorOn(
+              insertResult.first->second, kj::str("'", nameText, "' previously defined here."));
+        }
       }
     }
 
@@ -645,6 +661,18 @@ void NodeTranslator::checkMembers(
             errorReporter.addErrorOn(decl, "This declaration can only appear in structs.");
             break;
         }
+
+        // Struct members may have nested decls.  We need to check those here, because no one else
+        // is going to do it.
+        if (decl.getName().getValue().size() == 0) {
+          // Unnamed union.  Check members as if they are in the same scope.
+          check(decl.getNestedDecls(), decl.getBody().which());
+        } else {
+          // Children are in their own scope.
+          DuplicateNameDetector(errorReporter)
+              .check(decl.getNestedDecls(), decl.getBody().which());
+        }
+
         break;
 
       default:

c++/src/capnp/compiler/node-translator.h

@@ -118,10 +118,6 @@ private:
 
   void compileNode(Declaration::Reader decl, schema::Node::Builder builder);
 
-  void checkMembers(List<Declaration>::Reader nestedDecls, Declaration::Body::Which parentKind);
-  // Check the given member list for errors, including detecting duplicate names and detecting
-  // out-of-place declarations.
-
   void disallowNested(List<Declaration>::Reader nestedDecls);
   // Complain if the nested decl list is non-empty.
 
@@ -130,6 +126,7 @@ private:
   void compileAnnotation(Declaration::Annotation::Reader decl,
                          schema::AnnotationNode::Builder builder);
 
+  class DuplicateNameDetector;
   class DuplicateOrdinalDetector;
   class StructLayout;
   class StructTranslator;

c++/src/capnp/compiler/parser.c++

@@ -675,8 +675,21 @@ CapnpParser::CapnpParser(Orphanage orphanageParam, const ErrorReporter& errorRep
       }));
 
   parsers.unionDecl = arena.copy(p::transform(
-      p::sequence(p::optional(identifier), p::optional(parsers.ordinal), keyword("union"),
-                  p::many(parsers.annotation)),
+      // Hacky:  The first branch of this oneOf() can correctly match named unions as well as
+      //   anonymous unions that have an ordinal, but fails to match anonymous unions wil no
+      //   ordinal because the "union" keyword is matched as an identifier and interpreted as
+      //   the name, and then parsing fails after that.  So, we have the second branch which
+      //   just matches the "union" keyword alone, and injects dummy null values for the
+      //   name and ordinal.
+      p::oneOf(
+          p::sequence(
+              p::optional(identifier), p::optional(parsers.ordinal), keyword("union"),
+              p::many(parsers.annotation)),
+          p::sequence(
+              keyword("union"),
+              p::optional([](ParserInput&) -> kj::Maybe<Located<Text::Reader>> { return nullptr; }),
+              p::optional([](ParserInput&) -> kj::Maybe<Orphan<LocatedInteger>> {return nullptr; }),
+              p::many(parsers.annotation))),
       [this](kj::Maybe<Located<Text::Reader>>&& name,
              kj::Maybe<Orphan<LocatedInteger>>&& ordinal,
              kj::Array<Orphan<Declaration::AnnotationApplication>>&& annotations)

c++/src/capnp/encoding-test.c++

@@ -396,6 +396,40 @@ TEST(Encoding, UnionDefault) {
   }
 }
 
+TEST(Encoding, UnnamedUnion) {
+  MallocMessageBuilder builder;
+  auto root = builder.getRoot<test::TestUnnamedUnion>();
+  EXPECT_EQ(test::TestUnnamedUnion::FOO, root.which());
+
+  root.setBar(321);
+  EXPECT_EQ(test::TestUnnamedUnion::BAR, root.which());
+  EXPECT_EQ(test::TestUnnamedUnion::BAR, root.asReader().which());
+  EXPECT_EQ(321, root.getBar());
+  EXPECT_EQ(321, root.asReader().getBar());
+  EXPECT_DEBUG_ANY_THROW(root.getFoo());
+  EXPECT_DEBUG_ANY_THROW(root.asReader().getFoo());
+
+  root.setFoo(123);
+  EXPECT_EQ(test::TestUnnamedUnion::FOO, root.which());
+  EXPECT_EQ(test::TestUnnamedUnion::FOO, root.asReader().which());
+  EXPECT_EQ(123, root.getFoo());
+  EXPECT_EQ(123, root.asReader().getFoo());
+  EXPECT_DEBUG_ANY_THROW(root.getBar());
+  EXPECT_DEBUG_ANY_THROW(root.asReader().getBar());
+
+  KJ_IF_MAYBE(u, Schema::from<test::TestUnnamedUnion>().getUnnamedUnion()) {
+    // The discriminant is allocated between allocating the first and second members.
+    EXPECT_EQ(1, u->getProto().getBody().getUnionMember().getDiscriminantOffset());
+    EXPECT_EQ(0, u->getMemberByName("foo").getProto().getBody().getFieldMember().getOffset());
+    EXPECT_EQ(1, u->getMemberByName("bar").getProto().getBody().getFieldMember().getOffset());
+
+    // The union receives the ordinal of its first member, since it does not explicitly declare one.
+    EXPECT_EQ(1, u->getProto().getOrdinal());
+  } else {
+    ADD_FAILURE() << "getUnnamedUnion() should have returned non-null.";
+  }
+}
+
 // =======================================================================================
 
 TEST(Encoding, ListDefaults) {

c++/src/capnp/generated-header-support.h

@@ -164,8 +164,18 @@ struct RawSchema {
   // TODO(someday):  Make this a hashtable.
 
   struct MemberInfo {
-    uint16_t unionIndex;  // 0 = not in a union, >0 = parent union's index + 1
-    uint16_t index;       // index of the member
+    uint16_t unionIndex;
+    // 0 = not in a union, >0 = one plus the index of the union within the scope indicated by
+    // scopeOrdinal.
+
+//    uint16_t scopeOrdinal;
+//    // One plus the ordinal number of the parent scope of this member when looking up by name.
+//    // Zero represents the top-level scope.
+
+    uint16_t index;
+    // Index of the member within its scope.  If the index is greater than the number of elements
+    // in the scope, then the member is in an unnamed union, and its index within that union is
+    // `index` - (number of members in the outer scope).
   };
 
   const MemberInfo* membersByName;

c++/src/capnp/schema-loader.c++

@@ -241,11 +241,6 @@ private:
                 uint dataSizeInBits, uint pointerCount,
                 uint unionIndex, uint index) {
     validateMemberName(member.getName(), unionIndex, index);
-
-    VALIDATE_SCHEMA(member.getOrdinal() < sawOrdinal.size() &&
-                    !sawOrdinal[member.getOrdinal()],
-                    "Invalid ordinal.");
-    sawOrdinal[member.getOrdinal()] = true;
     VALIDATE_SCHEMA(member.getCodeOrder() < sawCodeOrder.size() &&
                     !sawCodeOrder[member.getCodeOrder()],
                     "Invalid codeOrder.");
@@ -253,6 +248,11 @@ private:
 
     switch (member.getBody().which()) {
       case schema::StructNode::Member::Body::FIELD_MEMBER: {
+        VALIDATE_SCHEMA(member.getOrdinal() < sawOrdinal.size() &&
+                        !sawOrdinal[member.getOrdinal()],
+                        "Invalid ordinal.", member.getOrdinal());
+        sawOrdinal[member.getOrdinal()] = true;
+
         auto field = member.getBody().getFieldMember();
 
         uint fieldBits;
@@ -272,6 +272,8 @@ private:
                         "Schema invalid: Union discriminant out-of-bounds.");
 
         auto uMembers = u.getMembers();
+        VALIDATE_SCHEMA(uMembers.size() >= 2, "Union must have at least two members.");
+
         KJ_STACK_ARRAY(bool, uSawCodeOrder, uMembers.size(), 32, 256);
         memset(uSawCodeOrder.begin(), 0, uSawCodeOrder.size() * sizeof(uSawCodeOrder[0]));
 
@@ -281,8 +283,24 @@ private:
           VALIDATE_SCHEMA(
               uMember.getBody().which() == schema::StructNode::Member::Body::FIELD_MEMBER,
               "Union members must be fields.");
+
+          uint subUnionIndex;
+          if (member.getName().size() == 0) {
+            subUnionIndex = unionIndex;
+          } else {
+            subUnionIndex = index + 1;
+          }
           validate(uMember, uSawCodeOrder, sawOrdinal, dataSizeInBits, pointerCount,
-                   index + 1, subIndex++);
+                   subUnionIndex, subIndex++);
+        }
+
+        // Union ordinal may match the ordinal of its first member, meaning it was unspecified in
+        // the schema file.  Otherwise, it must be unique.
+        if (member.getOrdinal() != uMembers[0].getOrdinal()) {
+          VALIDATE_SCHEMA(member.getOrdinal() < sawOrdinal.size() &&
+                          !sawOrdinal[member.getOrdinal()],
+                          "Invalid ordinal.", member.getOrdinal());
+          sawOrdinal[member.getOrdinal()] = true;
         }
         break;
       }

c++/src/capnp/schema-test.c++

@@ -270,6 +270,12 @@ TEST(Schema, Lists) {
   }
 }
 
+TEST(Schema, UnnamedUnion) {
+  StructSchema schema = Schema::from<test::TestUnnamedUnion>();
+
+  EXPECT_TRUE(schema.findMemberByName("") == nullptr);
+}
+
 }  // namespace
 }  // namespace _ (private)
 }  // namespace capnp

c++/src/capnp/schema.c++

@@ -129,6 +129,10 @@ StructSchema::MemberList StructSchema::getMembers() const {
 }
 
 kj::Maybe<StructSchema::Member> StructSchema::findMemberByName(kj::StringPtr name) const {
+  if (name.size() == 0) {
+    return nullptr;
+  }
+
   return findSchemaMemberByName(raw, name, 0, getMembers());
 }
 
@@ -140,6 +144,11 @@ StructSchema::Member StructSchema::getMemberByName(kj::StringPtr name) const {
   }
 }
 
+kj::Maybe<StructSchema::Union> StructSchema::getUnnamedUnion() const {
+  return findSchemaMemberByName(raw, "", 0, getMembers()).map(
+      [](Member member) { return member.asUnion(); });
+}
+
 kj::Maybe<StructSchema::Union> StructSchema::Member::getContainingUnion() const {
   if (unionIndex == 0) return nullptr;
   return parent.getMembers()[unionIndex - 1].asUnion();

c++/src/capnp/schema.h

@@ -121,12 +121,23 @@ public:
   class MemberList;
 
   MemberList getMembers() const;
+  // List top-level members of this struct.  This list will contain top-level unions (including
+  // the anonymous one, if any), but will not contain the members of those unions (even if the
+  // union is anonymous).
 
   kj::Maybe<Member> findMemberByName(kj::StringPtr name) const;
+  // Find the member with the given name, or return null if there is no such member.  If the struct
+  // contains an unnamed union, then this will find members of that union in addition to members
+  // of the outer struct, since they exist in the same namespace, even though the union's members
+  // are not listed by `getMembers()`.  Therefore, you may want to check `getContainingUnion()` on
+  // the result.
 
   Member getMemberByName(kj::StringPtr name) const;
   // Like findMemberByName() but throws an exception on failure.
 
+  kj::Maybe<Union> getUnnamedUnion() const;
+  // If the struct contains an unnamed union, return it.
+
 private:
   StructSchema(const _::RawSchema* raw): Schema(raw) {}
   template <typename T> static inline StructSchema fromImpl() {

c++/src/capnp/test.capnp

@@ -383,3 +383,14 @@ struct TestStructUnion {
     object @2 :TestObject;
   }
 }
+
+struct TestUnnamedUnion {
+  before @0 :Void;
+
+  union {
+    foo @1 :UInt16;
+    bar @2 :UInt32;
+  }
+
+  after @3 :Void;
+}