[SE-0364] Handle retroactive conformance for types and protocols from underlying modules #71302
Conversation
tshortli
force-pushed
the
suppress-retroactive-warning-for-types-from-underlying-module
branch
2 times, most recently
from
4387480 to
dc56003
Compare
tshortli
force-pushed
the
suppress-retroactive-warning-for-types-from-underlying-module
branch
from
dc56003 to
c709291
Compare
|
@swift-ci please test |
… 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 swiftlang#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
tshortli
force-pushed
the
suppress-retroactive-warning-for-types-from-underlying-module
branch
from
c709291 to
8846f4e
Compare
|
@swift-ci please smoke test |
| ModuleDecl *getUnderlyingModuleIfOverlay() const override { | ||
| return ImportedUnderlyingModule; | ||
| } | ||
|
|
||
| const clang::Module *getUnderlyingClangModule() const override { |
There was a problem hiding this comment.
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.
Right, there are existing ModuleDecl methods that call these methods that I've overridden, and I think it was an oversight that those ModuleDecl methods were not functional on the module instance representing the compiled module for the job.
|
|
||
| // 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
Rather than looping over the imports this way, you could modify ImportResolver::getModule() to save off the underlying clang module in a new member variable of ImportResolver (there's a separate code path to look it up—look for the use of getClangModuleLoader()) and then have performImportResolution() set that on the SourceFile immediately before/after calling setImports(). Would that be a cleaner solution?
There was a problem hiding this comment.
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.
Implemented here: #71330
It is intended to work for both of them, yes. But in case this was not clear, the diagnostics were not behaving correctly in either case. |
|
@swift-ci please smoke test Linux |
tshortli
deleted the
suppress-retroactive-warning-for-types-from-underlying-module
branch
As recommended in feedback on swiftlang#71302, cache the underlying clang module after loading it in `ImportResolver`, rather than filtering it out of the overall set of resolved imports. This is more efficient and results in less duplicated code that must identify the underlying clang module.
Follow up to swiftlang#71302. Resolves rdar://122590301
Follow up to swiftlang#71302. Resolves rdar://122590301
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 #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 returnnullptrand 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 ofSourceFiles.Resolves rdar://121478556