From 8846f4e6913e80b91d41df97763d0ed6bb903149 Mon Sep 17 00:00:00 2001 From: Allan Shortlidge Date: Wed, 31 Jan 2024 18:28:20 -0800 Subject: [PATCH] [SE-0364] Handle retroactive conformance for types and protocols from underlying modules. SE-0364 was implemented to discourage "retroactive" conformances that might conflict with conformances that could be introduced by other modules in the future. These diagnostics should not apply to conformances that involve types and protocols imported from the underlying clang module of a Swift module since the two modules are assumed to be developed in tandem by the same owners, despite technically being separate modules from the perspective of the compiler. The diagnostics implemented in https://github.com/apple/swift/pull/36068 were designed to take underlying clang modules into account. However, the implementation assumed that `ModuleDecl::getUnderlyingModuleIfOverlay()` would behave as expected when called on the Swift module being compiled. Unfortunately, it would always return `nullptr` and thus conformances involving the underlying clang module are being diagnosed unexpectedly. The fix is to make `ModuleDecl::getUnderlyingModuleIfOverlay()` behave as expected when it is made up of `SourceFile`s. Resolves rdar://121478556 --- include/swift/AST/SourceFile.h | 14 ++++++ lib/AST/Module.cpp | 14 ++++++ lib/AST/ModuleDependencies.cpp | 2 - .../swift-modules/ObjectiveC.swift | 2 +- ...conformances_underlying_clang_module.swift | 49 +++++++++++++++++++ 5 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 test/Sema/extension_retroactive_conformances_underlying_clang_module.swift diff --git a/include/swift/AST/SourceFile.h b/include/swift/AST/SourceFile.h index f247013cfc6b6..e40b4e52a8b53 100644 --- a/include/swift/AST/SourceFile.h +++ b/include/swift/AST/SourceFile.h @@ -144,6 +144,9 @@ class SourceFile final : public FileUnit { /// This is \c None until it is filled in by the import resolution phase. llvm::Optional>> Imports; + /// The underlying clang module, if imported in this file. + ModuleDecl *ImportedUnderlyingModule = nullptr; + /// Which imports have made use of @preconcurrency. llvm::SmallDenseSet> PreconcurrencyImportsUsed; @@ -693,6 +696,17 @@ class SourceFile final : public FileUnit { return isScriptMode() || hasMainDecl(); } + ModuleDecl *getUnderlyingModuleIfOverlay() const override { + return ImportedUnderlyingModule; + } + + const clang::Module *getUnderlyingClangModule() const override { + if (!ImportedUnderlyingModule) + return nullptr; + + return ImportedUnderlyingModule->findUnderlyingClangModule(); + } + /// Get the root refinement context for the file. The root context may be /// null if the context hierarchy has not been built yet. Use /// TypeChecker::getOrBuildTypeRefinementContext() to get a built diff --git a/lib/AST/Module.cpp b/lib/AST/Module.cpp index 4da2e42b75abe..30386b3c08382 100644 --- a/lib/AST/Module.cpp +++ b/lib/AST/Module.cpp @@ -2566,6 +2566,20 @@ void SourceFile::setImports(ArrayRef> imports) { assert(!Imports && "Already computed imports"); Imports = getASTContext().AllocateCopy(imports); + + // Find and cache the import of the underlying module, if present. + auto parentModuleName = getParentModule()->getName(); + for (auto import : imports) { + if (!import.options.contains(ImportFlags::Exported)) + continue; + + auto importedModule = import.module.importedModule; + if (importedModule->getName() == parentModuleName && + importedModule->findUnderlyingClangModule()) { + ImportedUnderlyingModule = import.module.importedModule; + break; + } + } } bool SourceFile::hasImportUsedPreconcurrency( diff --git a/lib/AST/ModuleDependencies.cpp b/lib/AST/ModuleDependencies.cpp index f383654fa6e69..50717699b9683 100644 --- a/lib/AST/ModuleDependencies.cpp +++ b/lib/AST/ModuleDependencies.cpp @@ -438,8 +438,6 @@ swift::dependencies::checkImportNotTautological(const ImportPath::Module moduleP filename, modulePath.front().Item); return false; - - return false; } void SwiftDependencyTracker::addCommonSearchPathDeps( diff --git a/test/Inputs/clang-importer-sdk/swift-modules/ObjectiveC.swift b/test/Inputs/clang-importer-sdk/swift-modules/ObjectiveC.swift index f6071839a99a9..278fb9d340760 100644 --- a/test/Inputs/clang-importer-sdk/swift-modules/ObjectiveC.swift +++ b/test/Inputs/clang-importer-sdk/swift-modules/ObjectiveC.swift @@ -82,7 +82,7 @@ public func ~=(x: NSObject, y: NSObject) -> Bool { return true } -extension NSObject : @retroactive Equatable, @retroactive Hashable { +extension NSObject : Equatable, Hashable { public static func == (lhs: NSObject, rhs: NSObject) -> Bool { return lhs.isEqual(rhs) } diff --git a/test/Sema/extension_retroactive_conformances_underlying_clang_module.swift b/test/Sema/extension_retroactive_conformances_underlying_clang_module.swift new file mode 100644 index 0000000000000..64c946c00940e --- /dev/null +++ b/test/Sema/extension_retroactive_conformances_underlying_clang_module.swift @@ -0,0 +1,49 @@ +// RUN: %empty-directory(%t) +// RUN: split-file %s %t +// RUN: %target-swift-frontend -swift-version 5 %t/OtherLibrary.swift -emit-module -module-name OtherLibrary -o %t +// RUN: %target-swift-frontend -typecheck %t/Library.swift -module-name Library -verify -swift-version 5 -import-underlying-module -I %t +// RUN: %target-swift-frontend -typecheck %t/Library.swift -module-name Library -verify -swift-version 5 -DEXPLICIT_IMPORT -I %t + +// REQUIRES: objc_interop + +//--- module.modulemap + +module Library { + header "Library.h" +} + +//--- Library.h + +@import Foundation; + +@interface UnderlyingLibraryClass : NSObject +@end + +@protocol UnderlyingLibraryProtocol +@end + +//--- OtherLibrary.swift + +public class OtherLibraryClass {} +public protocol OtherLibraryProtocol {} + +//--- Library.swift + +#if EXPLICIT_IMPORT +@_exported import Library +#endif +import OtherLibrary + +public class LibraryClass {} +public protocol LibraryProtocol {} + +extension LibraryClass: UnderlyingLibraryProtocol {} +extension LibraryClass: LibraryProtocol {} +extension LibraryClass: OtherLibraryProtocol {} +extension UnderlyingLibraryClass: OtherLibraryProtocol {} +extension UnderlyingLibraryClass: LibraryProtocol {} +extension UnderlyingLibraryClass: UnderlyingLibraryProtocol {} +extension OtherLibraryClass: UnderlyingLibraryProtocol {} +extension OtherLibraryClass: LibraryProtocol {} +extension OtherLibraryClass: OtherLibraryProtocol {} // expected-warning {{extension declares a conformance of imported type 'OtherLibraryClass' to imported protocol 'OtherLibraryProtocol'}} +// expected-note @-1 {{add '@retroactive' to silence this warning}} {{30-50=@retroactive OtherLibraryProtocol}}