From 448bb422254d6afd1ffc1f54c72d281b162d38a7 Mon Sep 17 00:00:00 2001 From: Joe Groff Date: Wed, 16 Aug 2023 20:23:48 -0700 Subject: [PATCH] Narrow `@_originallyDefinedIn` check in NominalTypeDecl::isResilient to protocols. And add a comment explaining why this check is only relevant to protocols, and shouldn't be copied to other similar-looking isResilient methods on other types. According to 25376025ae442d6b9212ff207f944ed3d9e959e4, which introduced the original `@_originallyDefinedIn` check, this check was necessary to maintain ABI compatibility when protocol declarations are moved, but in all other cases, we should treat declarations as resilient when they are defined in other library-evolution-enabled modules, even when the declaration was originally defined in the current module. --- lib/AST/Decl.cpp | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index a484fca087e4d..f1e6c1b958669 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -4783,11 +4783,31 @@ bool NominalTypeDecl::isResilient(ModuleDecl *M, case ResilienceExpansion::Minimal: return isResilient(); case ResilienceExpansion::Maximal: - // We consider this decl belongs to the module either it's currently - // defined in this module or it's originally defined in this module, which - // is specified by @_originallyDefinedIn - return M != getModuleContext() && !isOriginallyDefinedIn(this, M) && - isResilient(); + // We can access declarations from the same module + // non-resiliently in a maximal context. + if (M == getModuleContext()) { + return false; + } + // If a protocol is originally declared in the current module, then we + // directly expose protocol witness tables and their contents for any + // conformances in the same module as symbols. If the protocol later + // moves, then we need to preserve those extra symbols from the home + // module by treating the protocol as if it was still defined in the same + // module. + // + // This logic does not and should not generally extend to other kinds of + // declaration. If a declaration moves to a new module with library + // evolution enabled, then even the original module has to access it + // according to the library evolution ABI. This is an ABI compatibility + // hack only for protocols. If you see other variations of `isResilient` + // that don't check `isOriginallyDefinedIn`, they are probably correct. + if (isa(this) + && isOriginallyDefinedIn(this, M)) { + return false; + } + // Otherwise, we have to access the declaration resiliently if it's + // resilient anywhere. + return isResilient(); } llvm_unreachable("bad resilience expansion"); }