-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[SE-0364] Handle retroactive conformance for types and protocols from underlying modules #71302
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2566,6 +2566,20 @@ void | |
SourceFile::setImports(ArrayRef<AttributedImport<ImportedModule>> 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; | ||
} | ||
} | ||
Comment on lines
+2569
to
+2582
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than looping over the imports this way, you could modify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That does seem like a cleaner solution because it keeps the logic for identifying the underlying clang module more centralized. In the interest of time I think I'm going to land this solution first, but I will follow it up with what you've suggested - thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented here: #71330 |
||
} | ||
|
||
bool SourceFile::hasImportUsedPreconcurrency( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a little confused at first, but it looks like these are base class methods used by the existing diagnostic, so overriding them changes its behavior. Nice and clean. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, there are existing
ModuleDecl
methods that call these methods that I've overridden, and I think it was an oversight that thoseModuleDecl
methods were not functional on the module instance representing the compiled module for the job.