Merge pull request #74597 from beccadax/order-to-chaos

[NFC] [PrintAsClang] Add tiebreaking rule to sort

Author: beccadax

lib/PrintAsClang/ModuleContentsWriter.cpp

@@ -691,20 +691,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++,
@@ -716,12 +717,35 @@ class ModuleWriter {
         return Ascending;
 
       // 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.
@@ -742,16 +766,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());

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;

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