Merge pull request github#12433 from github/alexdenisov+redsun82/tuple-mangling

Swift: properly identify types and declarations in trap files via mangling

Author: redsun82

swift/extractor/SwiftExtractor.cpp

@@ -14,7 +14,6 @@
 #include "swift/extractor/infra/file/Path.h"
 #include "swift/extractor/infra/SwiftLocationExtractor.h"
 #include "swift/extractor/infra/SwiftBodyEmissionStrategy.h"
-#include "swift/extractor/mangler/SwiftMangler.h"
 #include "swift/logging/SwiftAssert.h"
 
 using namespace codeql;
@@ -49,24 +48,20 @@ static void archiveFile(const SwiftExtractorConfiguration& config, swift::Source
   }
 }
 
-// TODO: This should be factored out/replaced with simplified version of custom mangling
-static std::string mangledDeclName(const swift::ValueDecl& decl) {
-  std::string_view moduleName = decl.getModuleContext()->getRealName().str();
-  // ASTMangler::mangleAnyDecl crashes when called on `ModuleDecl`
-  if (decl.getKind() == swift::DeclKind::Module) {
-    return std::string{moduleName};
-  }
-  swift::Mangle::ASTMangler mangler;
-  if (decl.getKind() == swift::DeclKind::TypeAlias) {
-    // In cases like this (when coming from PCM)
-    //  typealias CFXMLTree = CFTree
-    //  typealias CFXMLTreeRef = CFXMLTree
-    // mangleAnyDecl mangles both CFXMLTree and CFXMLTreeRef into 'So12CFXMLTreeRefa'
-    // which is not correct and causes inconsistencies. mangleEntity makes these two distinct
-    // prefix adds a couple of special symbols, we don't necessary need them
-    return mangler.mangleEntity(&decl);
+static std::string mangledDeclName(const swift::Decl& decl) {
+  // SwiftRecursiveMangler mangled name cannot be used directly as it can be too long for file names
+  // so we build some minimal human readable info for debuggability and append a hash
+  auto ret = decl.getModuleContext()->getRealName().str().str();
+  ret += '/';
+  ret += swift::Decl::getKindName(decl.getKind());
+  ret += '_';
+  if (auto valueDecl = llvm::dyn_cast<swift::ValueDecl>(&decl); valueDecl && valueDecl->hasName()) {
+    ret += valueDecl->getBaseName().userFacingName();
+    ret += '_';
   }
-  return mangler.mangleAnyDecl(&decl, /* prefix = */ false);
+  SwiftRecursiveMangler mangler;
+  ret += mangler.mangleDecl(decl).hash();
+  return ret;
 }
 
 static fs::path getFilename(swift::ModuleDecl& module,
@@ -76,20 +71,7 @@ static fs::path getFilename(swift::ModuleDecl& module,
     return resolvePath(primaryFile->getFilename());
   }
   if (lazyDeclaration) {
-    // this code will be thrown away in the near future
-    auto decl = llvm::dyn_cast<swift::ValueDecl>(lazyDeclaration);
-    CODEQL_ASSERT(decl, "not a ValueDecl");
-    auto mangled = mangledDeclName(*decl);
-    // mangled name can be too long to use as a file name, so we can't use it directly
-    mangled = picosha2::hash256_hex_string(mangled);
-    std::string ret;
-    ret += module.getRealName().str();
-    ret += '_';
-    ret += decl->getBaseName().userFacingName();
-    ret += '_';
-    // half a SHA2 is enough
-    ret += std::string_view(mangled).substr(0, mangled.size() / 2);
-    return ret;
+    return mangledDeclName(*lazyDeclaration);
   }
   // PCM clang module
   if (module.isNonSwiftModule()) {

swift/extractor/infra/BUILD.bazel

@@ -11,5 +11,6 @@ swift_cc_library(
         "//swift/extractor/trap",
         "//swift/logging",
         "//swift/third_party/swift-llvm-support",
+        "@picosha2",
     ],
 )

swift/extractor/infra/SwiftLocationExtractor.cpp

@@ -25,8 +25,8 @@ void SwiftLocationExtractor::attachLocationImpl(const swift::SourceManager& sour
   entry.file = fetchFileLabel(file);
   std::tie(entry.start_line, entry.start_column) = sourceManager.getLineAndColumnInBuffer(start);
   std::tie(entry.end_line, entry.end_column) = sourceManager.getLineAndColumnInBuffer(end);
-  SwiftMangledName locName{{"loc", entry.file, ":", entry.start_line, ":", entry.start_column, ":",
-                            entry.end_line, ":", entry.end_column}};
+  SwiftMangledName locName{"loc", entry.file,     ':', entry.start_line, ':', entry.start_column,
+                           ':',   entry.end_line, ':', entry.end_column};
   entry.id = trap.createTypedLabel<DbLocationTag>(locName);
   trap.emit(entry);
   trap.emit(LocatableLocationsTrap{locatableLabel, entry.id});
@@ -99,7 +99,7 @@ TrapLabel<FileTag> SwiftLocationExtractor::fetchFileLabel(const std::filesystem:
   }
 
   DbFile entry({});
-  entry.id = trap.createTypedLabel<DbFileTag>({{"file_", file.string()}});
+  entry.id = trap.createTypedLabel<DbFileTag>({"file_", file.string()});
   entry.name = file.string();
   trap.emit(entry);
   store[file] = entry.id;

swift/extractor/infra/SwiftMangledName.cpp

@@ -1,29 +1,25 @@
 #include "swift/extractor/infra/SwiftMangledName.h"
 #include "absl/strings/str_cat.h"
 
-namespace codeql {
+#include <picosha2.h>
 
-namespace {
-void appendPart(std::string& out, const std::string& prefix) {
-  out += prefix;
-}
+namespace codeql {
 
-void appendPart(std::string& out, UntypedTrapLabel label) {
-  absl::StrAppend(&out, "{", label.str(), "}");
+std::string SwiftMangledName::hash() const {
+  auto ret = picosha2::hash256_hex_string(value);
+  // half a hash is already enough for disambuiguation
+  ret.resize(ret.size() / 2);
+  return ret;
 }
 
-void appendPart(std::string& out, unsigned index) {
-  absl::StrAppend(&out, index);
+SwiftMangledName& SwiftMangledName::operator<<(UntypedTrapLabel label) & {
+  absl::StrAppend(&value, "{", label.str(), "}");
+  return *this;
 }
 
-}  // namespace
-
-std::string SwiftMangledName::str() const {
-  std::string out;
-  for (const auto& part : parts) {
-    std::visit([&](const auto& contents) { appendPart(out, contents); }, part);
-  }
-  return out;
+SwiftMangledName& SwiftMangledName::operator<<(unsigned int i) & {
+  absl::StrAppend(&value, i);
+  return *this;
 }
 
 }  // namespace codeql

swift/extractor/infra/SwiftMangledName.h

@@ -9,37 +9,52 @@
 
 namespace codeql {
 
-struct SwiftMangledName {
+class SwiftMangledName {
+ public:
   using Part = std::variant<UntypedTrapLabel, std::string, unsigned>;
 
-  std::vector<Part> parts;
+  explicit operator bool() const { return !value.empty(); }
 
-  explicit operator bool() const { return !parts.empty(); }
+  const std::string& str() const { return value; }
+  operator std::string_view() const { return value; }
 
-  std::string str() const;
+  std::string hash() const;
 
-  // streaming labels or ints into a SwiftMangledName just appends them
-  template <typename Tag>
-  SwiftMangledName& operator<<(TrapLabel<Tag> label) {
-    parts.emplace_back(label);
-    return *this;
+  // let's avoid copying as long as we don't need it
+  SwiftMangledName() = default;
+  SwiftMangledName(const SwiftMangledName&) = delete;
+  SwiftMangledName& operator=(const SwiftMangledName&) = delete;
+  SwiftMangledName(SwiftMangledName&&) = default;
+  SwiftMangledName& operator=(SwiftMangledName&&) = default;
+
+  template <typename... Args>
+  SwiftMangledName(Args&&... args) {
+    (operator<<(std::forward<Args>(args)), ...);
   }
 
-  SwiftMangledName& operator<<(unsigned i) {
-    parts.emplace_back(i);
-    return *this;
+  SwiftMangledName& operator<<(UntypedTrapLabel label) &;
+  SwiftMangledName& operator<<(unsigned i) &;
+
+  template <typename Tag>
+  SwiftMangledName& operator<<(TrapLabel<Tag> label) & {
+    return operator<<(static_cast<UntypedTrapLabel>(label));
   }
 
   // streaming string-like stuff will add a new part it only if strictly required, otherwise it will
   // append to the last part if it is a string
   template <typename T>
-  SwiftMangledName& operator<<(T&& arg) {
-    if (parts.empty() || !std::holds_alternative<std::string>(parts.back())) {
-      parts.emplace_back("");
-    }
-    std::get<std::string>(parts.back()) += std::forward<T>(arg);
+  SwiftMangledName& operator<<(T&& arg) & {
+    value += arg;
     return *this;
   }
+
+  template <typename T>
+  SwiftMangledName&& operator<<(T&& arg) && {
+    return std::move(operator<<(std::forward<T>(arg)));
+  }
+
+ private:
+  std::string value;
 };
 
 }  // namespace codeql