From 099c4152e05e84700a4635f8c8b9d3997615c047 Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Fri, 21 Mar 2025 13:32:46 -0400 Subject: [PATCH 1/2] AST: Use the right module name in TypeDecl::compare() The last step in building a generic signature is to sort the requirements. Requirements are sorted by comparing their subject types. If two requirements have the same subject type, which can only happen with conformance requirements, we break the tie by comparing protocol declarations. We compare protocol declarations using TypeDecl::compare(), which is a shortlex order on the components of the fully qualified name of a protocol (eg, Swift.Sequence, etc.) While this order is part of the ABI, it has not been updated over the years for several important changes: - It did not handle module aliases; if we import a module via an alias, we should use the real module name to compare protocols, and not the aliased name. This produced inconsistent results if the same module was imported under different names, which can happen with module interface files that use module aliases. - It did not handle the -module-abi-name flag. Changing the ABI name of a module changes how we mangle protocol names, and the order should match the mangling. This change uses the correct module names for comparison to match what the mangler does. Of course this is an ABI break, however it should have limited impact since module aliases and -module-abi-name are not widely used. Fixes rdar://147441890. --- lib/AST/Decl.cpp | 37 +++++++++++++++++-- .../conformance_requirement_order.swift | 24 ++++++++++++ ...rmance_requirement_order_concurrency.swift | 10 +++++ 3 files changed, 68 insertions(+), 3 deletions(-) create mode 100644 test/SILGen/conformance_requirement_order.swift create mode 100644 test/SILGen/conformance_requirement_order_concurrency.swift diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index 4728e8567a6e7..9f75a4273859f 100644 --- a/lib/AST/Decl.cpp +++ b/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" @@ -5365,9 +5366,39 @@ 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 { + // 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, and for the compiler build of SwiftSyntax. + auto *module = decl->getDeclContext()->getParentModule(); + if (module->getRealName().str() == SWIFT_CONCURRENCY_NAME || + module->getABIName().str() == "CompilerSwiftSyntax" || + module->getName() == module->getABIName()) { + return module->getRealName().str(); + } + return module->getABIName().str(); + }; + + if (int result = getModuleNameForOrder(type1).compare(getModuleNameForOrder(type2))) return result; } diff --git a/test/SILGen/conformance_requirement_order.swift b/test/SILGen/conformance_requirement_order.swift new file mode 100644 index 0000000000000..4573253428d06 --- /dev/null +++ b/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) (@in_guaranteed T) -> () { + func requirementOrderHorse(_: T) {} +#endif \ No newline at end of file diff --git a/test/SILGen/conformance_requirement_order_concurrency.swift b/test/SILGen/conformance_requirement_order_concurrency.swift new file mode 100644 index 0000000000000..198868249388d --- /dev/null +++ b/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) (@guaranteed T) -> () { +func requirementOrderConcurrency(_: T) {} \ No newline at end of file From 2dc2cd028735e466aa10d3de1ca11446d5d57cfc Mon Sep 17 00:00:00 2001 From: Slava Pestov Date: Fri, 21 Mar 2025 13:33:04 -0400 Subject: [PATCH 2/2] AST: Also respect @_originallyDefinedIn in TypeDecl::compare() If a protocol was moved from one module to another, we need to continue using the old module name when comparing it against other protocols. --- lib/AST/Decl.cpp | 7 +++++++ ...e_requirement_order_originally_defined_in.swift | 14 ++++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 test/SILGen/conformance_requirement_order_originally_defined_in.swift diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index 9f75a4273859f..a615cc03dc5e7 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -5370,6 +5370,13 @@ int TypeDecl::compare(const TypeDecl *type1, const TypeDecl *type2) { // 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. // diff --git a/test/SILGen/conformance_requirement_order_originally_defined_in.swift b/test/SILGen/conformance_requirement_order_originally_defined_in.swift new file mode 100644 index 0000000000000..31264f5701897 --- /dev/null +++ b/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) (@in_guaranteed T) -> () { +public func requirementOrderWithOriginallyDefinedIn(_: T) {} \ No newline at end of file