Merge pull request #19186 from jrose-apple/full-reverse

When sorting imports for uniquing purposes, use full module names

Author: jrose-apple

include/swift/AST/Module.h

@@ -151,6 +151,45 @@ class ModuleDecl : public DeclContext, public TypeDecl {
     }
   };
 
+  /// Produces the components of a given module's full name in reverse order.
+  ///
+  /// For a Swift module, this will only ever have one component, but an
+  /// imported Clang module might actually be a submodule.
+  class ReverseFullNameIterator {
+  public:
+    // Make this look like a valid STL iterator.
+    using difference_type = int;
+    using value_type = StringRef;
+    using pointer = StringRef *;
+    using reference = StringRef;
+    using iterator_category = std::forward_iterator_tag;
+
+  private:
+    PointerUnion<const ModuleDecl *, const /* clang::Module */ void *> current;
+  public:
+    ReverseFullNameIterator() = default;
+    explicit ReverseFullNameIterator(const ModuleDecl *M);
+    explicit ReverseFullNameIterator(const clang::Module *clangModule) {
+      current = clangModule;
+    }
+
+    StringRef operator*() const;
+    ReverseFullNameIterator &operator++();
+
+    friend bool operator==(ReverseFullNameIterator left,
+                           ReverseFullNameIterator right) {
+      return left.current == right.current;
+    }
+    friend bool operator!=(ReverseFullNameIterator left,
+                           ReverseFullNameIterator right) {
+      return !(left == right);
+    }
+
+    /// This is a convenience function that writes the entire name, in forward
+    /// order, to \p out.
+    void printForward(raw_ostream &out) const;
+  };
+
 private:
   /// If non-NULL, a plug-in that should be used when performing external
   /// lookups.
@@ -490,6 +529,15 @@ class ModuleDecl : public DeclContext, public TypeDecl {
   /// Returns the associated clang module if one exists.
   const clang::Module *findUnderlyingClangModule() const;
 
+  /// Returns a generator with the components of this module's full,
+  /// hierarchical name.
+  ///
+  /// For a Swift module, this will only ever have one component, but an
+  /// imported Clang module might actually be a submodule.
+  ReverseFullNameIterator getReverseFullModuleName() const {
+    return ReverseFullNameIterator(this);
+  }
+
   SourceRange getSourceRange() const { return SourceRange(); }
 
   static bool classof(const DeclContext *DC) {

lib/AST/Module.cpp

@@ -1005,14 +1005,65 @@ bool ModuleDecl::isSameAccessPath(AccessPathTy lhs, AccessPathTy rhs) {
   });
 }
 
+ModuleDecl::ReverseFullNameIterator::ReverseFullNameIterator(
+    const ModuleDecl *M) {
+  assert(M);
+  // Note: This will look through overlays as well, but that's fine for name
+  // generation purposes. The point of an overlay is to
+  if (auto *clangModule = M->findUnderlyingClangModule())
+    current = clangModule;
+  else
+    current = M;
+}
+
+StringRef ModuleDecl::ReverseFullNameIterator::operator*() const {
+  assert(current && "all name components exhausted");
+
+  if (auto *swiftModule = current.dyn_cast<const ModuleDecl *>())
+    return swiftModule->getName().str();
+
+  auto *clangModule =
+      static_cast<const clang::Module *>(current.get<const void *>());
+  return clangModule->Name;
+}
+
+ModuleDecl::ReverseFullNameIterator &
+ModuleDecl::ReverseFullNameIterator::operator++() {
+  if (!current)
+    return *this;
+
+  if (auto *swiftModule = current.dyn_cast<const ModuleDecl *>()) {
+    current = nullptr;
+    return *this;
+  }
+
+  auto *clangModule =
+      static_cast<const clang::Module *>(current.get<const void *>());
+  if (clangModule->Parent)
+    current = clangModule->Parent;
+  else
+    current = nullptr;
+  return *this;
+}
+
+void
+ModuleDecl::ReverseFullNameIterator::printForward(raw_ostream &out) const {
+  SmallVector<StringRef, 8> elements(*this, {});
+  swift::interleave(swift::reversed(elements),
+                    [&out](StringRef next) { out << next; },
+                    [&out] { out << '.'; });
+}
+
 void
 ModuleDecl::removeDuplicateImports(SmallVectorImpl<ImportedModule> &imports) {
   std::sort(imports.begin(), imports.end(),
             [](const ImportedModule &lhs, const ImportedModule &rhs) -> bool {
     // Arbitrarily sort by name to get a deterministic order.
-    // FIXME: Submodules don't get sorted properly here.
-    if (lhs.second != rhs.second)
-      return lhs.second->getName().str() < rhs.second->getName().str();
+    if (lhs.second != rhs.second) {
+      return std::lexicographical_compare(
+          lhs.second->getReverseFullModuleName(), {},
+          rhs.second->getReverseFullModuleName(), {});
+    }
     using AccessPathElem = std::pair<Identifier, SourceLoc>;
     return std::lexicographical_compare(lhs.first.begin(), lhs.first.end(),
                                         rhs.first.begin(), rhs.first.end(),

lib/ClangImporter/ClangImporter.cpp

@@ -1451,12 +1451,8 @@ void ClangImporter::collectSubModuleNames(
     if (!submodule)
       return;
   }
-  auto submoduleNameLength = submodule->getFullModuleName().length();
-  for (auto sub : submodule->submodules()) {
-    std::string full = sub->getFullModuleName();
-    full.erase(0, submoduleNameLength + 1);
-    names.push_back(std::move(full));
-  }
+  for (auto sub : submodule->submodules())
+    names.push_back(sub->Name);
 }
 
 bool ClangImporter::isModuleImported(const clang::Module *M) {

lib/FrontendTool/TextualInterfaceGeneration.cpp

@@ -64,10 +64,7 @@ static void printImports(raw_ostream &out, ModuleDecl *M) {
     if (publicImportSet.count(import))
       out << "@_exported ";
     out << "import ";
-    if (auto *clangModule = import.second->findUnderlyingClangModule())
-      out << clangModule->getFullModuleName();
-    else
-      out << import.second->getName();
+    import.second->getReverseFullModuleName().printForward(out);
 
     // Write the access path we should be honoring but aren't.
     // (See diagnoseScopedImports above.)

lib/PrintAsObjC/PrintAsObjC.cpp

@@ -2654,15 +2654,6 @@ class ModuleWriter {
     return import == importer->getImportedHeaderModule();
   }
 
-  static void getClangSubmoduleReversePath(SmallVectorImpl<StringRef> &path,
-                                           const clang::Module *module) {
-    // FIXME: This should be an API on clang::Module.
-    do {
-      path.push_back(module->Name);
-      module = module->Parent;
-    } while (module);
-  }
-
   static int compareImportModulesByName(const ImportModuleTy *left,
                                         const ImportModuleTy *right) {
     auto *leftSwiftModule = left->dyn_cast<ModuleDecl *>();
@@ -2692,10 +2683,10 @@ class ModuleWriter {
     assert(rightClangModule->isSubModule() &&
            "top-level modules should use a normal swift::ModuleDecl");
 
-    SmallVector<StringRef, 8> leftReversePath;
-    SmallVector<StringRef, 8> rightReversePath;
-    getClangSubmoduleReversePath(leftReversePath, leftClangModule);
-    getClangSubmoduleReversePath(rightReversePath, rightClangModule);
+    SmallVector<StringRef, 8> leftReversePath(
+        ModuleDecl::ReverseFullNameIterator(leftClangModule), {});
+    SmallVector<StringRef, 8> rightReversePath(
+        ModuleDecl::ReverseFullNameIterator(rightClangModule), {});
 
     assert(leftReversePath != rightReversePath &&
            "distinct Clang modules should not have the same full name");
@@ -2734,11 +2725,7 @@ class ModuleWriter {
         assert(clangModule->isSubModule() &&
                "top-level modules should use a normal swift::ModuleDecl");
         out << "@import ";
-        SmallVector<StringRef, 8> submoduleNames;
-        getClangSubmoduleReversePath(submoduleNames, clangModule);
-        interleave(submoduleNames.rbegin(), submoduleNames.rend(),
-                   [&out](StringRef next) { out << next; },
-                   [&out] { out << "."; });
+        ModuleDecl::ReverseFullNameIterator(clangModule).printForward(out);
         out << ";\n";
       }
     }

lib/Serialization/Serialization.cpp

@@ -37,9 +37,6 @@
 #include "swift/ClangImporter/ClangModule.h"
 #include "swift/Serialization/SerializationOptions.h"
 #include "swift/Strings.h"
-
-#include "clang/Basic/Module.h"
-
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Bitcode/BitstreamWriter.h"
@@ -1032,22 +1029,12 @@ void Serializer::writeDocHeader() {
 using ImportPathBlob = llvm::SmallString<64>;
 static void flattenImportPath(const ModuleDecl::ImportedModule &import,
                               ImportPathBlob &out) {
-  ArrayRef<FileUnit *> files = import.second->getFiles();
-  if (auto clangModule = dyn_cast<ClangModuleUnit>(files.front())) {
-    // FIXME: This is an awful hack to handle Clang submodules.
-    // Once Swift has a native notion of submodules, this can go away.
-    const clang::Module *submodule = clangModule->getClangModule();
-    SmallVector<StringRef, 4> submoduleNames;
-    do {
-      submoduleNames.push_back(submodule->Name);
-      submodule = submodule->Parent;
-    } while (submodule);
-    interleave(submoduleNames.rbegin(), submoduleNames.rend(),
-               [&out](StringRef next) { out.append(next); },
-               [&out] { out.push_back('\0'); });
-  } else {
-    out.append(import.second->getName().str());
-  }
+  SmallVector<StringRef, 4> reverseSubmoduleNames(
+      import.second->getReverseFullModuleName(), {});
+
+  interleave(reverseSubmoduleNames.rbegin(), reverseSubmoduleNames.rend(),
+             [&out](StringRef next) { out.append(next); },
+             [&out] { out.push_back('\0'); });
 
   if (import.first.empty())
     return;

test/ModuleInterface/Inputs/imports-submodule-order/module.modulemap

@@ -0,0 +1,7 @@
+module X {
+  explicit module Submodule {}
+}
+
+module Y {
+  explicit module Submodule {}
+}

test/ModuleInterface/imports-submodule-order.swift

@@ -0,0 +1,22 @@
+// RUN: %target-swift-frontend -emit-interface-path - -emit-module -o /dev/null %s -I %S/Inputs/imports-submodule-order/ | %FileCheck %s
+// RUN: %target-swift-frontend -emit-interface-path - -emit-module -o /dev/null %s -I %S/Inputs/imports-submodule-order/ -D XY | %FileCheck %s
+
+#if XY
+@_exported import X.Submodule
+@_exported import Y.Submodule
+
+#else
+@_exported import Y.Submodule
+@_exported import X.Submodule
+
+#endif
+
+// The order below is not alphabetical, just deterministic given a set of
+// imports across any number of files and in any order.
+
+// CHECK-NOT: import
+// CHECK: import X.Submodule{{$}}
+// CHECK-NEXT: import Y.Submodule{{$}}
+// CHECK-NEXT: import X{{$}}
+// CHECK-NEXT: import Y{{$}}
+// CHECK-NOT: import

tools/SourceKit/lib/SwiftLang/SwiftDocSupport.cpp

@@ -416,21 +416,9 @@ static bool initDocEntityInfo(const Decl *D,
     case DeclContextKind::FileUnit: {
       if (auto *CD = D->getClangDecl()) {
         if (auto *M = CD->getImportedOwningModule()) {
-          const clang::Module *Root = M->getTopLevelModule();
-
-          // If Root differs from the owning module, then the owning module is
-          // a sub-module.
-          if (M != Root) {
+          if (M->isSubModule()) {
             llvm::raw_svector_ostream OS(Info.SubModuleName);
-            llvm::SmallVector<StringRef, 4> Names;
-
-            // Climb up and collect sub-module names.
-            for (auto Current = M; Current != Root; Current = Current->Parent) {
-              Names.insert(Names.begin(), Current->Name);
-            }
-            OS << Root->Name;
-            std::for_each(Names.begin(), Names.end(),
-                          [&](StringRef N) { OS << "." << N; });
+            ModuleDecl::ReverseFullNameIterator(M).printForward(OS);
           }
         }
       }