[NFC] [PrintAsClang] Add tiebreaking rule to sort

beccadax · beccadax · commit a3a074cba4e2 · 2024-06-21T23:46:46.000-07:00
Because the underlying API for fetching top-level decls returns them in an unspecified order, PrintAsClang sorts the decls before printing them to make the output order more stable. However, the rules currently implemented have at least one known defect (they compare only the unqualified name of a nested class, so two nested classes with the same Swift name sort in an arbitrary order), and there are likely many more.

Add a fallback rule which sorts declarations by their mangled name; this should at least distinguish all non-colliding ValueDecls from each other, albeit according to fairly opaque criteria. Additionally add a rule to help distinguish extensions with very similar content, and tweak other logic so that the comparison function is less likely to give up early rather than continuing to look for a usable difference.

Fixes rdar://129485103.
diff --git a/lib/PrintAsClang/ModuleContentsWriter.cpp b/lib/PrintAsClang/ModuleContentsWriter.cpp
@@ -680,20 +680,21 @@ class ModuleWriter {
         // Sort top level functions with the same C++ name by their location to
         // have stable sorting that depends on users source but not on the
         // compiler invocation.
+        // FIXME: This is pretty suspect; PrintAsClang sometimes operates on
+        //        serialized modules which don't have SourceLocs, so this sort
+        //        rule may be applied in some steps of a build but not others.
         if ((*rhs)->getLoc().isValid() && (*lhs)->getLoc().isValid()) {
-          std::string rhsLoc, lhsLoc;
-          auto getLocText = [](const AbstractFunctionDecl *afd) {
+          auto getLocText = [](const Decl *afd) {
             std::string res;
             llvm::raw_string_ostream os(res);
             afd->getLoc().print(os, afd->getASTContext().SourceMgr);
             return std::move(os.str());
           };
-          if (getLocText(cast<AbstractFunctionDecl>(*lhs)) <
-              getLocText(cast<AbstractFunctionDecl>(*rhs)))
-            return Descending;
-          return Ascending;
+
+          result = getLocText(*rhs).compare(getLocText(*lhs));
+          if (result != 0)
+            return result;
         }
-        return result;
       }
 
       // A function and a global variable can have the same name in C++,
@@ -705,12 +706,35 @@ class ModuleWriter {
         return -1;
 
       // Prefer value decls to extensions.
-      assert(!(isa<ValueDecl>(*lhs) && isa<ValueDecl>(*rhs)));
       if (isa<ValueDecl>(*lhs) && !isa<ValueDecl>(*rhs))
         return Descending;
       if (!isa<ValueDecl>(*lhs) && isa<ValueDecl>(*rhs))
         return Ascending;
 
+      // Last-ditch ValueDecl tiebreaker: Compare mangled names. This captures
+      // *tons* of context and detail missed by the previous checks, but the
+      // resulting sort makes little sense to humans.
+      // FIXME: It'd be nice to share the mangler or even memoize mangled names,
+      //        but we'd have to stop using `llvm::array_pod_sort()` so that we
+      //        could capture some outside state.
+      Mangle::ASTMangler mangler;
+      auto getMangledName = [&](const Decl *D) {
+        auto VD = dyn_cast<ValueDecl>(D);
+        if (!VD && isa<ExtensionDecl>(D))
+          VD = cast<ExtensionDecl>(D)->getExtendedNominal();
+        if (!VD)
+          return std::string();
+        return mangler.mangleAnyDecl(VD, /*prefix=*/true,
+                                     /*respectOriginallyDefinedIn=*/true);
+      };
+      result = getMangledName(*rhs).compare(getMangledName(*lhs));
+      if (result != 0)
+        return result;
+
+      // Mangled names ought to distinguish all value decls, leaving only
+      // extensions of the same nominal type beyond this point.
+      assert(isa<ExtensionDecl>(*lhs) && isa<ExtensionDecl>(*rhs));
+
       // Break ties in extensions by putting smaller extensions last (in reverse
       // order).
       // FIXME: This will end up taking linear time.
@@ -731,16 +755,48 @@ class ModuleWriter {
 
       // If that fails, arbitrarily pick the extension whose protocols are
       // alphabetically first.
-      auto mismatch =
-        std::mismatch(lhsProtos.begin(), lhsProtos.end(), rhsProtos.begin(),
-                      [] (const ProtocolDecl *nextLHSProto,
-                          const ProtocolDecl *nextRHSProto) {
-        return nextLHSProto->getName() != nextRHSProto->getName();
-      });
-      if (mismatch.first == lhsProtos.end())
-        return Equivalent;
-      StringRef lhsProtoName = (*mismatch.first)->getName().str();
-      return lhsProtoName.compare((*mismatch.second)->getName().str());
+      {
+        auto mismatch =
+          std::mismatch(lhsProtos.begin(), lhsProtos.end(), rhsProtos.begin(),
+                        [] (const ProtocolDecl *nextLHSProto,
+                            const ProtocolDecl *nextRHSProto) {
+          return nextLHSProto->getName() != nextRHSProto->getName();
+        });
+        if (mismatch.first != lhsProtos.end()) {
+          StringRef lhsProtoName = (*mismatch.first)->getName().str();
+          return lhsProtoName.compare((*mismatch.second)->getName().str());
+        }
+      }
+
+      // Still nothing? Fine, we'll pick the one with the alphabetically first
+      // member instead.
+      {
+        auto mismatch =
+          std::mismatch(cast<ExtensionDecl>(*lhs)->getMembers().begin(),
+                        cast<ExtensionDecl>(*lhs)->getMembers().end(),
+                        cast<ExtensionDecl>(*rhs)->getMembers().begin(),
+                        [] (const Decl *nextLHSDecl, const Decl *nextRHSDecl) {
+          if (isa<ValueDecl>(nextLHSDecl) && isa<ValueDecl>(nextRHSDecl)) {
+            return cast<ValueDecl>(nextLHSDecl)->getName() !=
+                     cast<ValueDecl>(nextRHSDecl)->getName();
+          }
+          return isa<ValueDecl>(nextLHSDecl) != isa<ValueDecl>(nextRHSDecl);
+        });
+        if (mismatch.first != cast<ExtensionDecl>(*lhs)->getMembers().end()) {
+          auto *lhsMember = dyn_cast<ValueDecl>(*mismatch.first),
+               *rhsMember = dyn_cast<ValueDecl>(*mismatch.second);
+          if (!rhsMember && lhsMember)
+            return Descending;
+          if (lhsMember && !rhsMember)
+            return Ascending;
+          if (lhsMember && rhsMember)
+            return rhsMember->getName().compare(lhsMember->getName());
+        }
+      }
+
+      // Hopefully two extensions with identical conformances and member names
+      // will be interchangeable enough not to matter.
+      return Equivalent;
     });
 
     assert(declsToWrite.empty());
diff --git a/test/PrintAsObjC/classes.swift b/test/PrintAsObjC/classes.swift
@@ -14,8 +14,8 @@
 
 // RUN: %target-swift-frontend(mock-sdk: -sdk %S/../Inputs/clang-importer-sdk -I %t) -emit-module -I %S/Inputs/custom-modules -o %t %s -disable-objc-attr-requires-foundation-module
 // RUN: %target-swift-frontend(mock-sdk: -sdk %S/../Inputs/clang-importer-sdk -I %t) -parse-as-library %t/classes.swiftmodule -typecheck -I %S/Inputs/custom-modules -emit-objc-header-path %t/classes.h -import-objc-header %S/../Inputs/empty.h -disable-objc-attr-requires-foundation-module
-// RUN: %FileCheck %s < %t/classes.h
-// RUN: %FileCheck --check-prefix=NEGATIVE %s < %t/classes.h
+// RUN: %FileCheck %s --input-file %t/classes.h
+// RUN: %FileCheck --check-prefix=NEGATIVE %s --input-file %t/classes.h
 // RUN: %check-in-clang -I %S/Inputs/custom-modules/ %t/classes.h
 // RUN: not %check-in-clang -I %S/Inputs/custom-modules/ -fno-modules -Qunused-arguments %t/classes.h
 // RUN: %check-in-clang -I %S/Inputs/custom-modules/ -fno-modules -Qunused-arguments %t/classes.h -include CoreFoundation.h -include objc_generics.h -include SingleGenericClass.h -include CompatibilityAlias.h
@@ -427,6 +427,28 @@ class MyObject : NSObject {}
   class ImplicitObjCInner : A1 {}
 }
 
+// CHECK-LABEL: @interface NestedCollision1Identical
+// CHECK-NEXT: - (void)before
+// CHECK-NEXT: init
+// CHECK-NEXT: @end
+// CHECK: @interface NestedCollision2Identical
+// CHECK-NEXT: - (void)after
+// CHECK-NEXT: init
+// CHECK-NEXT: @end
+
+// We're intentionally declaring NestedCollision2 before NestedCollision1 to
+// make sure they're being sorted based on their names, not their source order.
+@objc @objcMembers class NestedCollision2 {
+  @objc(NestedCollision2Identical) @objcMembers class Identical: NSObject {
+    @objc func after() {}
+  }
+}
+@objc @objcMembers class NestedCollision1 {
+  @objc(NestedCollision1Identical) @objcMembers class Identical: NSObject {
+    @objc func before() {}
+  }
+}
+
 // CHECK-LABEL: @class Inner2;
 // CHECK-LABEL: @interface NestedMembers
 // CHECK-NEXT: @property (nonatomic, strong) Inner2 * _Nullable ref2;
diff --git a/test/PrintAsObjC/extensions.swift b/test/PrintAsObjC/extensions.swift
@@ -76,6 +76,27 @@ extension A5 {
   var notObjC: NotObjC { return NotObjC() }
 }
 
+// Check that two otherwise tied extensions will print in alphabetical
+// order by first member with a differing Swift name.
+
+// CHECK-LABEL: @interface A6
+@objc class A6 {}
+
+extension A6 {
+  @objc(skippedBool:) func skipped(_: Bool) {}
+  @objc func def() {}
+}
+extension A6 {
+  @objc(skippedInt:) func skipped(_: Int) {}
+  @objc func abc() {}
+}
+// CHECK: @interface A6 (SWIFT_EXTENSION(extensions))
+// CHECK: - (void)skippedInt:
+// CHECK: - (void)abc
+// CHECK: @interface A6 (SWIFT_EXTENSION(extensions))
+// CHECK: - (void)skippedBool:
+// CHECK: - (void)def
+
 // CHECK-LABEL: @interface CustomName{{$}}
 // CHECK-NEXT: init
 // CHECK-NEXT: @end
