AST: Fix a problem with the protocol order

lib/AST/Decl.cpp

@@ -15,6 +15,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "swift/Strings.h"
 #include "swift/AST/Decl.h"
 #include "swift/AST/ASTContext.h"
 #include "swift/AST/ASTMangler.h"
@@ -5348,9 +5349,46 @@ int TypeDecl::compare(const TypeDecl *type1, const TypeDecl *type2) {
 
   // Prefer module names earlier in the alphabet.
   if (dc1->isModuleScopeContext() && dc2->isModuleScopeContext()) {
-    auto module1 = dc1->getParentModule();
-    auto module2 = dc2->getParentModule();
-    if (int result = module1->getName().str().compare(module2->getName().str()))
+    // For protocol declarations specifically, the order here is part
+    // of the ABI, and so we must take care to get the correct module
+    // name for the comparison.
+    auto getModuleNameForOrder = [&](const TypeDecl *decl) -> StringRef {
+      // Respect @_originallyDefinedIn on the type itself, so that
+      // moving a protocol across modules does not change its
+      // position in the order.
+      auto alternateName = decl->getAlternateModuleName();
+      if (!alternateName.empty())
+        return alternateName;
+
+      // This used to just call getName(), which caused accidental ABI breaks
+      // when a module is imported under different aliases.
+      //
+      // The new behavior matches the behavior in the mangler:
+      //
+      // - getName() returns the name of the module as its imported by the
+      //   client, which might be an alias different from the real name.
+      // - getRealName() returns the real name after desugaring aliases.
+      // - getABIName() returns the name set with the -module-abi-name flag, or
+      //   **getName()** if the flag is not set.
+      //
+      // Thus, the correct module name to use for mangling and ordering is
+      // getRealName(), unless getABIName() is distinct from getName(), in
+      // which case we use getABIName().
+      //
+      // FIXME: This should be fixed after auditing callers of getABIName().
+      //
+      // However, to maintain ABI compatibility, we ignore the ABI name for the
+      // _Concurrency module, which is "Swift", and still use the real name in
+      // this case.
+      auto *module = decl->getDeclContext()->getParentModule();
+      if (module->getRealName().str() == SWIFT_CONCURRENCY_NAME ||
+          module->getName() == module->getABIName()) {
+        return module->getRealName().str();
+      }
+      return module->getABIName().str();
+    };
+
+    if (int result = getModuleNameForOrder(type1).compare(getModuleNameForOrder(type2)))
       return result;
   }
 

test/SILGen/conformance_requirement_order.swift

@@ -0,0 +1,24 @@
+// RUN: %empty-directory(%t)
+
+// Name: SwiftHorse, RealName: Horse, ABIName: SwiftHorse
+
+// RUN: %target-swift-frontend -emit-module %s -module-name Horse -D LIB -emit-module-path %t/Horse.swiftmodule
+// RUN: %target-swift-emit-silgen %s -module-name main -I %t -module-alias SwiftHorse=Horse | %FileCheck %s
+
+// Name: SwiftHorse, RealName: SwiftHorse, ABIName: Horse
+
+// RUN: %target-swift-frontend -emit-module %s -module-name SwiftHorse -module-abi-name Horse -D LIB -emit-module-path %t/SwiftHorse.swiftmodule
+// RUN: %target-swift-emit-silgen %s -module-name main -I %t | %FileCheck %s
+
+#if LIB
+  public protocol Equine {}
+#else
+  import SwiftHorse
+
+  // Make sure we use the module's real name, and not its alias name, so that
+  // we always have Horse.Equine < Swift.Equatable. If this doesn't hold, the
+  // two requirements in the mangling will be flipped.
+
+  // CHECK-LABEL: sil hidden [ossa] @$s4main21requirementOrderHorseyyx0D06EquineRzSQRzlF : $@convention(thin) <T where T : Equine, T : Equatable> (@in_guaranteed T) -> () {
+  func requirementOrderHorse<T: Equine & Equatable>(_: T) {}
+#endif

test/SILGen/conformance_requirement_order_concurrency.swift

@@ -0,0 +1,10 @@
+// RUN: %target-swift-emit-silgen %s -module-name main | %FileCheck %s
+
+// See conformance_requirement_order.swift for the general case.
+//
+// For the _Concurrency module, the mistake is baked into the ABI. For example,
+// we mangle the below type as Swift.Executor, but order it as if it were
+// _Concurrency.Executor.
+
+// CHECK-LABEL: sil hidden [ossa] @$s4main27requirementOrderConcurrencyyyxSTRzScFRzlF : $@convention(thin) <T where T : Sequence, T : Executor> (@guaranteed T) -> () {
+func requirementOrderConcurrency<T: Executor & Sequence>(_: T) {}

test/SILGen/conformance_requirement_order_originally_defined_in.swift

@@ -0,0 +1,14 @@
+// RUN: %target-swift-emit-silgen %s -module-name main | %FileCheck %s
+
+// REQUIRES: OS=macosx
+
+@_originallyDefinedIn(module: "AnimalKit", macOS 10.10)
+@available(macOS 10.9, *)
+public protocol Ungulate {}
+
+@_originallyDefinedIn(module: "HorseKit", macOS 10.10)
+@available(macOS 10.9, *)
+public protocol Horse {}
+
+// CHECK-LABEL: sil [ossa] @$s4main39requirementOrderWithOriginallyDefinedInyyx9AnimalKit8UngulateRz05HorseI00K0RzlF : $@convention(thin) <T where T : Ungulate, T : Horse> (@in_guaranteed T) -> () {
+public func requirementOrderWithOriginallyDefinedIn<T: Ungulate & Horse>(_: T) {}