clang_generator: Use fully-qualified names, emit in source order

When generating headers from libraries that export functions as `extern "C"`
but still use namespaces (C-compatible C++ libraries), we want to generate
a Sandboxed API that includes fully-qualified namespace names as well.

In addition, we want the generated API to have the same source order as the
original library. Not only is this less surprising when reading the generated
code, it's also more accurate. Previously, we'd bundle all definitions in a
namespace and sort those alphabetically, but for code that relies on symbols
from another namespace to be available, generation will fail:

```c++
namespace zzz {
using entity_count_t = uint64_t;
}  // namespace zzz
namespace sheep_counter {
using sheep_count_t = :💤:entity_count_t;
extern "C" void IncreaseSheepCounter(sheep_count_t increment);
}  // namespace sheep_counter
```

PiperOrigin-RevId: 486586024
Change-Id: I419c9db8e9cb5b904364b353e2dc3d7f1030fab3

sandboxed_api/tools/clang_generator/BUILD.bazel

@@ -32,10 +32,9 @@ cc_library(
     ],
     copts = sapi_platform_copts(),
     deps = [
-        "//sandboxed_api/util:fileops",
         "//sandboxed_api/util:status",
-        "@com_google_absl//absl/container:btree",
         "@com_google_absl//absl/container:flat_hash_set",
+        "@com_google_absl//absl/container:node_hash_set",
         "@com_google_absl//absl/random",
         "@com_google_absl//absl/status",
         "@com_google_absl//absl/status:statusor",

sandboxed_api/tools/clang_generator/CMakeLists.txt

@@ -43,6 +43,7 @@ target_link_libraries(sapi_generator PUBLIC
   sapi::base
   absl::btree
   absl::flat_hash_set
+  absl::node_hash_set
   absl::random_random
   absl::status
   absl::statusor
@@ -73,6 +74,7 @@ if(BUILD_TESTING AND SAPI_BUILD_TESTING)
     emitter_test.cc
   )
   target_link_libraries(sapi_generator_test PRIVATE
+    absl::flat_hash_map
     absl::memory
     benchmark
     sapi::sapi

sandboxed_api/tools/clang_generator/emitter.cc

@@ -227,7 +227,8 @@ std::string PrintDecl(const clang::Decl* decl) {
 
 // Returns the spelling for a given declaration will be emitted to the final
 // header. This may rewrite declarations (like converting typedefs to using,
-// etc.).
+// etc.). Note that the resulting spelling will need to be wrapped inside a
+// namespace if the original declaration was inside one.
 std::string GetSpelling(const clang::Decl* decl) {
   // TODO(cblichmann): Make types nicer
   //   - Rewrite typedef to using
@@ -261,9 +262,11 @@ std::string GetParamName(const clang::ParmVarDecl* decl, int index) {
 
 absl::StatusOr<std::string> PrintFunctionPrototypeComment(
     const clang::FunctionDecl* decl) {
-  // TODO(cblichmann): Fix function pointers and anonymous namespace formatting
+  const clang::ASTContext& context = decl->getASTContext();
-  std::string out = absl::StrCat(decl->getDeclaredReturnType().getAsString(),
+
-                                 " ", decl->getQualifiedNameAsString(), "(");
+  std::string out = absl::StrCat(
+      MapQualTypeParameterForCxx(context, decl->getDeclaredReturnType()), " ",
+      decl->getQualifiedNameAsString(), "(");
 
   std::string print_separator;
   for (int i = 0; i < decl->getNumParams(); ++i) {
@@ -271,7 +274,8 @@ absl::StatusOr<std::string> PrintFunctionPrototypeComment(
 
     absl::StrAppend(&out, print_separator);
     print_separator = ", ";
-    absl::StrAppend(&out, param->getType().getAsString());
+    absl::StrAppend(&out,
+                    MapQualTypeParameterForCxx(context, param->getType()));
     if (std::string name = param->getName().str(); !name.empty()) {
       absl::StrAppend(&out, " ", name);
     }
@@ -357,8 +361,8 @@ absl::StatusOr<std::string> EmitFunction(const clang::FunctionDecl* decl) {
 }
 
 absl::StatusOr<std::string> EmitHeader(
-    const std::vector<std::string>& functions,
+    const std::vector<std::string>& function_definitions,
-    const Emitter::RenderedTypesMap& rendered_types,
+    const std::vector<const RenderedType*>& rendered_types,
     const GeneratorOptions& options) {
   std::string out;
   const std::string include_guard = GetIncludeGuard(options.out_file);
@@ -386,16 +390,23 @@ absl::StatusOr<std::string> EmitHeader(
   // Emit type dependencies
   if (!rendered_types.empty()) {
     absl::StrAppend(&out, "// Types this API depends on\n");
-    for (const auto& [ns_name, types] : rendered_types) {
+    std::string last_ns_name;
-      if (!ns_name.empty()) {
+    for (const RenderedType* rt : rendered_types) {
-        absl::StrAppend(&out, "namespace ", ns_name, " {\n");
+      const auto& [ns_name, spelling] = *rt;
-      }
+      if (last_ns_name != ns_name) {
-      for (const auto& type : types) {
+        if (!last_ns_name.empty()) {
-        absl::StrAppend(&out, type, ";\n");
+          absl::StrAppend(&out, "}  // namespace ", last_ns_name, "\n\n");
-      }
+        }
-      if (!ns_name.empty()) {
+        if (!ns_name.empty()) {
-        absl::StrAppend(&out, "}  // namespace ", ns_name, "\n\n");
+          absl::StrAppend(&out, "namespace ", ns_name, " {\n");
+        }
+        last_ns_name = ns_name;
       }
+
+      absl::StrAppend(&out, spelling, ";\n");
+    }
+    if (!last_ns_name.empty()) {
+      absl::StrAppend(&out, "}  // namespace ", last_ns_name, "\n\n");
     }
   }
 
@@ -411,7 +422,7 @@ absl::StatusOr<std::string> EmitHeader(
   // TODO(cblichmann): Make the "Api" suffix configurable or at least optional.
   absl::StrAppendFormat(&out, kClassHeaderTemplate,
                         absl::StrCat(options.name, "Api"));
-  absl::StrAppend(&out, absl::StrJoin(functions, "\n"));
+  absl::StrAppend(&out, absl::StrJoin(function_definitions, "\n"));
   absl::StrAppend(&out, kClassFooterTemplate);
 
   // Close out the header: close namespace (if needed) and end include guard
@@ -435,6 +446,15 @@ void Emitter::EmitType(clang::QualType qual) {
     return;
   }
 
+  // Skip types defined in system headers.
+  // TODO(cblichmann): Instead of this and the hard-coded entities below, we
+  //                   should map add the correct (system) headers to the
+  //                   generated output.
+  if (decl->getASTContext().getSourceManager().isInSystemHeader(
+          decl->getBeginLoc())) {
+    return;
+  }
+
   const std::vector<std::string> ns_path = GetNamespacePath(decl);
   std::string ns_name;
   if (!ns_path.empty()) {
@@ -446,18 +466,28 @@ void Emitter::EmitType(clang::QualType qual) {
       return;
     }
     if (ns_root == "absl") {
-      // Skip types from Abseil that we already include in the generated
+      // Skip Abseil internal namespaces
+      if (ns_path.size() > 1 && absl::EndsWith(ns_path[1], "_internal")) {
+        return;
+      }
+      // Skip types from Abseil that will already be included in the generated
       // header.
       if (auto name = ToStringView(decl->getName());
           name == "StatusCode" || name == "StatusToStringMode" ||
-          name == "CordMemoryAccounting") {
+          name == "CordMemoryAccounting" || name == "string_view" ||
+          name == "LogSeverity" || name == "LogEntry" || name == "Span" ||
+          name == "Time") {
         return;
       }
     }
     ns_name = absl::StrJoin(ns_path, "::");
   }
 
-  rendered_types_[ns_name].push_back(GetSpelling(decl));
+  std::string spelling = GetSpelling(decl);
+  if (const auto& [it, inserted] = rendered_types_.emplace(ns_name, spelling);
+      inserted) {
+    rendered_types_ordered_.push_back(&*it);
+  }
 }
 
 void Emitter::AddTypesFiltered(const QualTypeSet& types) {
@@ -467,16 +497,18 @@ void Emitter::AddTypesFiltered(const QualTypeSet& types) {
 }
 
 absl::Status Emitter::AddFunction(clang::FunctionDecl* decl) {
-  SAPI_ASSIGN_OR_RETURN(std::string function, EmitFunction(decl));
+  if (rendered_functions_.insert(decl->getQualifiedNameAsString()).second) {
-  functions_.push_back(function);
+    SAPI_ASSIGN_OR_RETURN(std::string function, EmitFunction(decl));
+    rendered_functions_ordered_.push_back(function);
+  }
   return absl::OkStatus();
 }
 
 absl::StatusOr<std::string> Emitter::EmitHeader(
     const GeneratorOptions& options) {
-  SAPI_ASSIGN_OR_RETURN(
+  SAPI_ASSIGN_OR_RETURN(const std::string header,
-      const std::string header,
+                        ::sapi::EmitHeader(rendered_functions_ordered_,
-      ::sapi::EmitHeader(functions_, rendered_types_, options));
+                                           rendered_types_ordered_, options));
   return internal::ReformatGoogleStyle(options.out_file, header);
 }
 

sandboxed_api/tools/clang_generator/emitter.h

@@ -18,7 +18,8 @@
 #include <string>
 #include <vector>
 
-#include "absl/container/btree_map.h"
+#include "absl/container/flat_hash_set.h"
+#include "absl/container/node_hash_set.h"
 #include "absl/status/status.h"
 #include "absl/status/statusor.h"
 #include "absl/strings/string_view.h"
@@ -37,13 +38,28 @@ absl::StatusOr<std::string> ReformatGoogleStyle(const std::string& filename,
 
 class GeneratorOptions;
 
+class RenderedType {
+ public:
+  RenderedType(std::string ns_name, std::string spelling)
+      : ns_name(std::move(ns_name)), spelling(std::move(spelling)) {}
+
+  bool operator==(const RenderedType& other) const {
+    return ns_name == other.ns_name && spelling == other.spelling;
+  }
+
+  template <typename H>
+  friend H AbslHashValue(H h, RenderedType rt) {
+    return H::combine(std::move(h), rt.ns_name, rt.spelling);
+  }
+
+  std::string ns_name;
+  std::string spelling;
+};
+
 // Responsible for emitting the actual textual representation of the generated
 // Sandboxed API header.
 class Emitter {
  public:
-  using RenderedTypesMap =
-      absl::btree_map<std::string, std::vector<std::string>>;
-
   // Adds the set of previously collected types to the emitter, recording the
   // spelling of each one. Types that are not supported by the current
   // generator settings or that are unwanted/unnecessary are skipped. Filtered
@@ -61,11 +77,21 @@ class Emitter {
   void EmitType(clang::QualType qual);
 
  protected:
-  // Maps namespace to a list of spellings for types
+  // Stores namespaces and a list of spellings for types. Keeps track of types
-  RenderedTypesMap rendered_types_;
+  // that have been rendered so far. Using a node_hash_set for pointer
+  // stability.
+  absl::node_hash_set<RenderedType> rendered_types_;
 
-  // Functions for sandboxed API, including their bodies
+  // A vector to preserve the order of type declarations needs to be preserved.
-  std::vector<std::string> functions_;
+  std::vector<const RenderedType*> rendered_types_ordered_;
+
+  // Fully qualified names of functions for the sandboxed API. Keeps track of
+  // functions that have been rendered so far.
+  absl::flat_hash_set<std::string> rendered_functions_;
+
+  // Rendered function bodies, as a vector to preserve source order. This is
+  // not strictly necessary, but makes the output look less surprising.
+  std::vector<std::string> rendered_functions_ordered_;
 };
 
 // Constructs an include guard name for the given filename. The name is of the

sandboxed_api/tools/clang_generator/emitter_test.cc

@@ -38,8 +38,19 @@ using ::testing::StrNe;
 
 class EmitterForTesting : public Emitter {
  public:
-  using Emitter::functions_;
+  std::vector<std::string> SpellingsForNS(const std::string& ns_name) {
-  using Emitter::rendered_types_;
+    std::vector<std::string> result;
+    for (const RenderedType* rt : rendered_types_ordered_) {
+      if (rt->ns_name == ns_name) {
+        result.push_back(rt->spelling);
+      }
+    }
+    return result;
+  }
+
+  const std::vector<std::string>& GetRenderedFunctions() {
+    return rendered_functions_ordered_;
+  }
 };
 
 class EmitterTest : public FrontendActionTest {};
@@ -55,7 +66,7 @@ TEST_F(EmitterTest, BasicFunctionality) {
                         std::make_unique<GeneratorAction>(emitter, options)),
       IsOk());
 
-  EXPECT_THAT(emitter.functions_, SizeIs(1));
+  EXPECT_THAT(emitter.GetRenderedFunctions(), SizeIs(1));
 
   absl::StatusOr<std::string> header = emitter.EmitHeader(options);
   EXPECT_THAT(header, IsOk());
@@ -83,9 +94,9 @@ TEST_F(EmitterTest, RelatedTypes) {
       IsOk());
 
   // Types from "std" should be skipped
-  EXPECT_THAT(emitter.rendered_types_["std"], IsEmpty());
+  EXPECT_THAT(emitter.SpellingsForNS("std"), IsEmpty());
 
-  EXPECT_THAT(UglifyAll(emitter.rendered_types_[""]),
+  EXPECT_THAT(UglifyAll(emitter.SpellingsForNS("")),
               ElementsAre("typedef enum { kRed, kGreen, kBlue } Color",
                           "struct Channel {"
                           " Color color;"
@@ -107,7 +118,7 @@ TEST_F(EmitterTest, NestedStruct) {
           std::make_unique<GeneratorAction>(emitter, GeneratorOptions())),
       IsOk());
 
-  EXPECT_THAT(UglifyAll(emitter.rendered_types_[""]),
+  EXPECT_THAT(UglifyAll(emitter.SpellingsForNS("")),
               ElementsAre("struct A {"
                           " struct B { int number; };"
                           " B b;"
@@ -126,7 +137,7 @@ TEST_F(EmitterTest, NestedAnonymousStruct) {
           std::make_unique<GeneratorAction>(emitter, GeneratorOptions())),
       IsOk());
 
-  EXPECT_THAT(UglifyAll(emitter.rendered_types_[""]),
+  EXPECT_THAT(UglifyAll(emitter.SpellingsForNS("")),
               ElementsAre("struct A {"
                           " struct { int number; } b;"
                           " int data; }"));
@@ -145,7 +156,7 @@ TEST_F(EmitterTest, ParentNotCollected) {
           std::make_unique<GeneratorAction>(emitter, GeneratorOptions())),
       IsOk());
 
-  EXPECT_THAT(UglifyAll(emitter.rendered_types_[""]),
+  EXPECT_THAT(UglifyAll(emitter.SpellingsForNS("")),
               ElementsAre("struct A {"
                           " struct B { int number; };"
                           " B b;"
@@ -161,7 +172,7 @@ TEST_F(EmitterTest, RemoveQualifiers) {
           std::make_unique<GeneratorAction>(emitter, GeneratorOptions())),
       IsOk());
 
-  EXPECT_THAT(UglifyAll(emitter.rendered_types_[""]),
+  EXPECT_THAT(UglifyAll(emitter.SpellingsForNS("")),
               ElementsAre("struct A { int data; }"));
 }
 
@@ -173,7 +184,7 @@ TEST_F(EmitterTest, StructByValueSkipsFunction) {
              extern "C" int Structize(A a);)",
           std::make_unique<GeneratorAction>(emitter, GeneratorOptions())),
       IsOk());
-  EXPECT_THAT(emitter.functions_, IsEmpty());
+  EXPECT_THAT(emitter.GetRenderedFunctions(), IsEmpty());
 }
 
 TEST_F(EmitterTest, ReturnStructByValueSkipsFunction) {
@@ -184,7 +195,7 @@ TEST_F(EmitterTest, ReturnStructByValueSkipsFunction) {
              extern "C" A Structize();)",
           std::make_unique<GeneratorAction>(emitter, GeneratorOptions())),
       IsOk());
-  EXPECT_THAT(emitter.functions_, IsEmpty());
+  EXPECT_THAT(emitter.GetRenderedFunctions(), IsEmpty());
 }
 
 TEST_F(EmitterTest, TypedefStructByValueSkipsFunction) {
@@ -195,7 +206,7 @@ TEST_F(EmitterTest, TypedefStructByValueSkipsFunction) {
              extern "C" int Structize(A a);)",
           std::make_unique<GeneratorAction>(emitter, GeneratorOptions())),
       IsOk());
-  EXPECT_THAT(emitter.functions_, IsEmpty());
+  EXPECT_THAT(emitter.GetRenderedFunctions(), IsEmpty());
 }
 
 TEST(IncludeGuard, CreatesRandomizedGuardForEmptyFilename) {

sandboxed_api/tools/clang_generator/types.cc

@@ -16,6 +16,8 @@
 
 #include "absl/strings/str_cat.h"
 #include "absl/strings/str_format.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/QualTypeNames.h"
 #include "clang/AST/Type.h"
 
 namespace sapi {
@@ -38,7 +40,7 @@ void TypeCollector::CollectRelatedTypes(clang::QualType qual) {
   }
 
   if (const auto* typedef_type = qual->getAs<clang::TypedefType>()) {
-    auto* typedef_decl = typedef_type->getDecl();
+    clang::TypedefNameDecl* typedef_decl = typedef_type->getDecl();
     if (!typedef_decl->getAnonDeclWithTypedefName()) {
       // Do not collect anonymous enums/structs as those are handled when
       // emitting them via their parent typedef/using declaration.
@@ -215,8 +217,12 @@ std::string MapQualType(const clang::ASTContext& context,
   return absl::StrCat("::sapi::v::Int /* aka '", qual.getAsString(), "' */");
 }
 
-std::string MapQualTypeParameterForCxx(const clang::ASTContext& /*context*/,
+std::string MapQualTypeParameterForCxx(const clang::ASTContext& context,
                                        clang::QualType qual) {
+  if (const auto* typedef_type = qual->getAs<clang::TypedefType>()) {
+    clang::TypedefNameDecl* typedef_decl = typedef_type->getDecl();
+    return typedef_decl->getQualifiedNameAsString();
+  }
   if (const auto* builtin = qual->getAs<clang::BuiltinType>()) {
     if (builtin->getKind() == clang::BuiltinType::Bool) {
       return "bool";  // _Bool -> bool
@@ -225,7 +231,8 @@ std::string MapQualTypeParameterForCxx(const clang::ASTContext& /*context*/,
     // - long long -> uint64_t
     // - ...
   }
-  return qual.getAsString();
+  return clang::TypeName::getFullyQualifiedName(qual, context,
+                                                context.getPrintingPolicy());
 }
 
 std::string MapQualTypeParameter(const clang::ASTContext& context,

sandboxed_api/tools/clang_generator/types.h

@@ -37,8 +37,7 @@ inline bool IsSimple(clang::QualType qual) {
 }
 
 inline bool IsPointerOrReference(clang::QualType qual) {
-  return qual->isPointerType() || qual->isMemberPointerType() ||
+  return qual->isAnyPointerType() || qual->isReferenceType();
-         qual->isLValueReferenceType() || qual->isRValueReferenceType();
 }
 
 class TypeCollector {
@@ -70,6 +69,11 @@ class TypeCollector {
 // If no mapping can be found, "int" is assumed.
 std::string MapQualType(const clang::ASTContext& context, clang::QualType qual);
 
+// Maps a qualified type to a fully qualified C++ type name. Transforms C-only
+// constructs such as _Bool to bool.
+std::string MapQualTypeParameterForCxx(const clang::ASTContext& context,
+                                       clang::QualType qual);
+
 // Maps a qualified type used as a function parameter to a type name compatible
 // with the generated Sandboxed API.
 std::string MapQualTypeParameter(const clang::ASTContext& context,