From 83030a9c639ab06053194df581a9e43af3e95a8b Mon Sep 17 00:00:00 2001 From: Holly Borla Date: Fri, 23 Jun 2023 18:44:30 -0700 Subject: [PATCH 1/2] [Macros] Don't visit macro-generated extensions in 'visitAuxiliaryDecls'. Macro-generated extensions are hoisted to file scope, because extensions are not valid in nested scopes. Callers of 'visitAuxiliaryDecls' assume that the auxiliary decls are in the same decl context as the original, which is clearly not the case for extensions, and it leads to issues like visiting extension at the wrong time during SILGen. The extensions are already added to the top-level decls, so we don't need to visit them as auxiliary decls, and we can type-check macro-expanded decls at the end of visitation in TypeCheckDeclPrimary. --- include/swift/AST/Decl.h | 6 ++++ lib/AST/Decl.cpp | 37 +++++++++++++-------- lib/AST/Module.cpp | 3 +- lib/IRGen/GenDecl.cpp | 5 --- lib/Index/Index.cpp | 3 ++ lib/SILGen/SILGen.cpp | 4 --- lib/SILGen/SILGenType.cpp | 5 --- lib/Sema/TypeCheckDeclPrimary.cpp | 5 +++ lib/Sema/TypeCheckMacros.cpp | 8 ++++- test/Index/index_macros.swift | 28 +++++++++------- test/Macros/macro_expand_conformances.swift | 6 ++++ 11 files changed, 67 insertions(+), 43 deletions(-) diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h index 556ee0f456948..a945d5cec7b92 100644 --- a/include/swift/AST/Decl.h +++ b/include/swift/AST/Decl.h @@ -924,6 +924,12 @@ class alignas(1 << DeclAlignInBits) Decl : public ASTAllocated { bool visitFreestandingExpanded = true ) const; + using ExtensionCallback = llvm::function_ref; + + /// Iterate over each macro-expanded extension for this declaration, + /// invoking the given callback with each extension. + void forEachExpandedExtension(ExtensionCallback callback) const; + using MacroCallback = llvm::function_ref; /// Iterate over each attached macro with the given role, invoking the diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index dd55ebe4529a1..9ff0e80f99b27 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -407,20 +407,6 @@ void Decl::visitAuxiliaryDecls( } } - if (auto *nominal = dyn_cast(mutableThis)) { - auto conformanceBuffers = - evaluateOrDefault(ctx.evaluator, - ExpandConformanceMacros{nominal}, - {}); - for (auto bufferID : conformanceBuffers) { - auto startLoc = sourceMgr.getLocForBufferStart(bufferID); - auto *sourceFile = moduleDecl->getSourceFileContainingLocation(startLoc); - for (auto *extension : sourceFile->getTopLevelDecls()) { - callback(extension); - } - } - } - if (visitFreestandingExpanded) { if (auto *med = dyn_cast(mutableThis)) { if (auto bufferID = evaluateOrDefault( @@ -436,6 +422,29 @@ void Decl::visitAuxiliaryDecls( // FIXME: fold VarDecl::visitAuxiliaryDecls into this. } +void +Decl::forEachExpandedExtension(ExtensionCallback callback) const { + auto &ctx = getASTContext(); + auto *mutableThis = const_cast(this); + auto *nominal = dyn_cast(mutableThis); + if (!nominal) + return; + + SourceManager &sourceMgr = ctx.SourceMgr; + auto *moduleDecl = getModuleContext(); + auto conformanceBuffers = + evaluateOrDefault(ctx.evaluator, + ExpandConformanceMacros{nominal}, + {}); + for (auto bufferID : conformanceBuffers) { + auto startLoc = sourceMgr.getLocForBufferStart(bufferID); + auto *sourceFile = moduleDecl->getSourceFileContainingLocation(startLoc); + for (auto *extension : sourceFile->getTopLevelDecls()) { + callback(cast(extension)); + } + } +} + void Decl::forEachAttachedMacro(MacroRole role, MacroCallback macroCallback) const { for (auto customAttrConst : getSemanticAttrs().getAttributes()) { diff --git a/lib/AST/Module.cpp b/lib/AST/Module.cpp index 3071fb7ac6b20..b6c61f445b0a2 100644 --- a/lib/AST/Module.cpp +++ b/lib/AST/Module.cpp @@ -4104,8 +4104,7 @@ void FileUnit::getTopLevelDeclsWithAuxiliaryDecls( getTopLevelDecls(nonExpandedDecls); for (auto *decl : nonExpandedDecls) { decl->visitAuxiliaryDecls([&](Decl *auxDecl) { - if (!isa(auxDecl)) - results.push_back(auxDecl); + results.push_back(auxDecl); }); results.push_back(decl); } diff --git a/lib/IRGen/GenDecl.cpp b/lib/IRGen/GenDecl.cpp index ed9c67dd2596a..70265ea319bf4 100644 --- a/lib/IRGen/GenDecl.cpp +++ b/lib/IRGen/GenDecl.cpp @@ -5578,11 +5578,6 @@ void IRGenModule::emitNestedTypeDecls(DeclRange members) { continue; member->visitAuxiliaryDecls([&](Decl *decl) { - // FIXME: Conformance macros can generate extension decls. These - // are visited as top-level decls; skip them here. - if (isa(decl)) - return; - emitNestedTypeDecls({decl, nullptr}); }); switch (member->getKind()) { diff --git a/lib/Index/Index.cpp b/lib/Index/Index.cpp index 90c862a75a30e..b2fabfd438545 100644 --- a/lib/Index/Index.cpp +++ b/lib/Index/Index.cpp @@ -1148,6 +1148,9 @@ void IndexSwiftASTWalker::visitModule(ModuleDecl &Mod) { if (!handleSourceOrModuleFile(*SrcFile)) return; walk(*SrcFile); + if (auto *synthesizedSF = SrcFile->getSynthesizedFile()) { + walk(synthesizedSF); + } } else { IsModuleFile = true; isSystemModule = Mod.isNonUserModule(); diff --git a/lib/SILGen/SILGen.cpp b/lib/SILGen/SILGen.cpp index c70a797ceadc9..a6bae1ac854f4 100644 --- a/lib/SILGen/SILGen.cpp +++ b/lib/SILGen/SILGen.cpp @@ -2201,10 +2201,6 @@ class SILGenModuleRAII { for (auto *D : sf->getTopLevelDecls()) { // Emit auxiliary decls. D->visitAuxiliaryDecls([&](Decl *auxiliaryDecl) { - // Skip extensions decls; they are visited below. - if (isa(auxiliaryDecl)) - return; - FrontendStatsTracer StatsTracer(SGM.getASTContext().Stats, "SILgen-decl", auxiliaryDecl); SGM.visit(auxiliaryDecl); diff --git a/lib/SILGen/SILGenType.cpp b/lib/SILGen/SILGenType.cpp index 8fa81c8c6435f..b82ea7bbc9345 100644 --- a/lib/SILGen/SILGenType.cpp +++ b/lib/SILGen/SILGenType.cpp @@ -1101,11 +1101,6 @@ class SILGenType : public TypeMemberVisitor { SGM.emitLazyConformancesForType(theType); forEachMemberToLower(theType, [&](Decl *member) { - // FIXME: Conformance macros can generate extension decls. These - // are visited as top-level decls; skip them here. - if (isa(member)) - return; - visit(member); }); diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp index 5ae37f30556cc..937160ca48f80 100644 --- a/lib/Sema/TypeCheckDeclPrimary.cpp +++ b/lib/Sema/TypeCheckDeclPrimary.cpp @@ -1936,6 +1936,11 @@ class DeclChecker : public DeclVisitor { "`" + VD->getBaseName().userFacingName().str() + "`"); } } + + // Visit macro-generated extensions. + decl->forEachExpandedExtension([&](ExtensionDecl *extension) { + DeclVisitor::visit(extension); + }); } diff --git a/lib/Sema/TypeCheckMacros.cpp b/lib/Sema/TypeCheckMacros.cpp index 36a13fbd2fd87..ff81dbcc7b296 100644 --- a/lib/Sema/TypeCheckMacros.cpp +++ b/lib/Sema/TypeCheckMacros.cpp @@ -1442,7 +1442,13 @@ swift::expandConformances(CustomAttr *attr, MacroDecl *macro, extension->setExtendedNominal(nominal); nominal->addExtension(extension); - // Make it accessible to getTopLevelDecls() + // Most other macro-generated declarations are visited through calling + // 'visitAuxiliaryDecls' on the original declaration the macro is attached + // to. We don't do this for macro-generated extensions, because the + // extension is not a peer of the original declaration. Instead of + // requiring all callers of 'visitAuxiliaryDecls' to understand the + // hoisting behavior of macro-generated extensions, we make the + // extension accessible through 'getTopLevelDecls()'. if (auto file = dyn_cast( decl->getDeclContext()->getModuleScopeContext())) file->getOrCreateSynthesizedFile().addTopLevelDecl(extension); diff --git a/test/Index/index_macros.swift b/test/Index/index_macros.swift index f20480556e50f..f8a807c2d85e7 100644 --- a/test/Index/index_macros.swift +++ b/test/Index/index_macros.swift @@ -81,6 +81,12 @@ struct AddOne { // CHECK: [[@LINE-5]]:1 | function/Swift | freeLog() | [[FREE_LOG_USR]] | Ref,Call,Impl,RelCall,RelCont // CHECK-NEXT: RelCall,RelCont | instance-method/Swift | freeFunc() | [[FREE_FUNC_USR]] +func testExpr() { + #freestandingExpr + // CHECK: [[@LINE-1]]:3 | function/Swift | exprLog() | [[EXPR_LOG_USR]] | Ref,Call,Impl,RelCall,RelCont + // CHECK-NEXT: RelCall,RelCont | function/Swift | testExpr() +} + // CHECK: [[@LINE+4]]:40 | macro/Swift | Peer() | [[PEER_USR]] | Ref // CHECK: [[@LINE+3]]:23 | macro/Swift | MemberAttribute() | [[MEMBER_ATTRIBUTE_USR]] | Ref // CHECK: [[@LINE+2]]:15 | macro/Swift | Member() | [[MEMBER_USR]] | Ref @@ -112,11 +118,6 @@ struct TestAttached { // CHECK: [[@LINE-24]]:39 | function/Swift | peerLog() | [[PEER_LOG_USR]] | Ref,Call,Impl,RelCall,RelCont // CHECK-NEXT: RelCall,RelCont | instance-method/Swift | peerFunc() | [[PEER_FUNC_USR]] -// `Conformance` adds `TestProto` as a conformance on an extension of `TestAttached` -// CHECK: [[@LINE-28]]:1 | extension/ext-struct/Swift | TestAttached | {{.*}} | Def,Impl -// CHECK: [[@LINE-29]]:1 | protocol/Swift | TestProto | [[PROTO_USR]] | Ref,Impl,RelBase -// CHECK-NEXT: RelBase | extension/ext-struct/Swift | TestAttached - // CHECK: [[@LINE+1]]:8 | struct/Swift | Outer | [[OUTER_USR:.*]] | Def struct Outer { // CHECK: [[@LINE+1]]:4 | macro/Swift | PeerMember() | [[PEER_MEMBER_USR]] | Ref @@ -137,16 +138,19 @@ struct Outer { // CHECK: [[@LINE-6]]:16 | function/Swift | memberLog() | [[MEMBER_LOG_USR]] | Ref,Call,Impl,RelCall,RelCont // CHECK-NEXT: RelCall,RelCont | instance-method/Swift | memberFunc() | [[INNER_FUNC_USR]] + +// Expanded extensions are visited last + +// `Conformance` adds `TestProto` as a conformance on an extension of `TestAttached` +// CHECK: [[@LINE-51]]:1 | extension/ext-struct/Swift | TestAttached | {{.*}} | Def,Impl +// CHECK: [[@LINE-52]]:1 | protocol/Swift | TestProto | [[PROTO_USR]] | Ref,Impl,RelBase +// CHECK-NEXT: RelBase | extension/ext-struct/Swift | TestAttached + // `Conformance` adds `TestProto` as a conformance on an extension of `TestInner` -// CHECK: [[@LINE-10]]:3 | extension/ext-struct/Swift | TestInner | {{.*}} | Def,Impl -// CHECK: [[@LINE-11]]:3 | protocol/Swift | TestProto | [[PROTO_USR]] | Ref,Impl,RelBase +// CHECK: [[@LINE-18]]:3 | extension/ext-struct/Swift | TestInner | {{.*}} | Def,Impl +// CHECK: [[@LINE-19]]:3 | protocol/Swift | TestProto | [[PROTO_USR]] | Ref,Impl,RelBase // CHECK-NEXT: RelBase | extension/ext-struct/Swift | TestInner -func testExpr() { - #freestandingExpr - // CHECK: [[@LINE-1]]:3 | function/Swift | exprLog() | [[EXPR_LOG_USR]] | Ref,Call,Impl,RelCall,RelCont - // CHECK-NEXT: RelCall,RelCont | function/Swift | testExpr() -} //--- IndexMacros.swift import SwiftSyntax diff --git a/test/Macros/macro_expand_conformances.swift b/test/Macros/macro_expand_conformances.swift index f1eca1873b97b..cf85e22b94a5b 100644 --- a/test/Macros/macro_expand_conformances.swift +++ b/test/Macros/macro_expand_conformances.swift @@ -63,6 +63,12 @@ requireHashable(S2()) requireEquatable(E.Nested()) +extension E { + @Equatable struct NestedInExtension {} +} + +requireEquatable(E.NestedInExtension()) + #if TEST_DIAGNOSTICS requireEquatable(PublicEquatable()) From 3ecfb532a9a2860e549accdc3797edd804dbd50f Mon Sep 17 00:00:00 2001 From: Holly Borla Date: Mon, 26 Jun 2023 09:51:50 -0700 Subject: [PATCH 2/2] [Macros] Type-check macro-generated extensions via the top-level decls of the synthesized source file instead of a separate API to gather the extensions. This is how macro-generated extensions are visited everywhere else. --- include/swift/AST/Decl.h | 6 ------ lib/AST/Decl.cpp | 23 ----------------------- lib/Sema/TypeCheckDeclPrimary.cpp | 5 ----- lib/Sema/TypeChecker.cpp | 10 ++++++++++ 4 files changed, 10 insertions(+), 34 deletions(-) diff --git a/include/swift/AST/Decl.h b/include/swift/AST/Decl.h index a945d5cec7b92..556ee0f456948 100644 --- a/include/swift/AST/Decl.h +++ b/include/swift/AST/Decl.h @@ -924,12 +924,6 @@ class alignas(1 << DeclAlignInBits) Decl : public ASTAllocated { bool visitFreestandingExpanded = true ) const; - using ExtensionCallback = llvm::function_ref; - - /// Iterate over each macro-expanded extension for this declaration, - /// invoking the given callback with each extension. - void forEachExpandedExtension(ExtensionCallback callback) const; - using MacroCallback = llvm::function_ref; /// Iterate over each attached macro with the given role, invoking the diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index 9ff0e80f99b27..755b6a424bb13 100644 --- a/lib/AST/Decl.cpp +++ b/lib/AST/Decl.cpp @@ -422,29 +422,6 @@ void Decl::visitAuxiliaryDecls( // FIXME: fold VarDecl::visitAuxiliaryDecls into this. } -void -Decl::forEachExpandedExtension(ExtensionCallback callback) const { - auto &ctx = getASTContext(); - auto *mutableThis = const_cast(this); - auto *nominal = dyn_cast(mutableThis); - if (!nominal) - return; - - SourceManager &sourceMgr = ctx.SourceMgr; - auto *moduleDecl = getModuleContext(); - auto conformanceBuffers = - evaluateOrDefault(ctx.evaluator, - ExpandConformanceMacros{nominal}, - {}); - for (auto bufferID : conformanceBuffers) { - auto startLoc = sourceMgr.getLocForBufferStart(bufferID); - auto *sourceFile = moduleDecl->getSourceFileContainingLocation(startLoc); - for (auto *extension : sourceFile->getTopLevelDecls()) { - callback(cast(extension)); - } - } -} - void Decl::forEachAttachedMacro(MacroRole role, MacroCallback macroCallback) const { for (auto customAttrConst : getSemanticAttrs().getAttributes()) { diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp index 937160ca48f80..5ae37f30556cc 100644 --- a/lib/Sema/TypeCheckDeclPrimary.cpp +++ b/lib/Sema/TypeCheckDeclPrimary.cpp @@ -1936,11 +1936,6 @@ class DeclChecker : public DeclVisitor { "`" + VD->getBaseName().userFacingName().str() + "`"); } } - - // Visit macro-generated extensions. - decl->forEachExpandedExtension([&](ExtensionDecl *extension) { - DeclVisitor::visit(extension); - }); } diff --git a/lib/Sema/TypeChecker.cpp b/lib/Sema/TypeChecker.cpp index cfdd067e5ccab..febef12db3e2f 100644 --- a/lib/Sema/TypeChecker.cpp +++ b/lib/Sema/TypeChecker.cpp @@ -297,6 +297,16 @@ TypeCheckSourceFileRequest::evaluate(Evaluator &eval, SourceFile *SF) const { } } + // Type-check macro-generated extensions. + if (auto *synthesizedSF = SF->getSynthesizedFile()) { + for (auto *decl : synthesizedSF->getTopLevelDecls()) { + if (!decl->isImplicit()) { + assert(isa(decl)); + TypeChecker::typeCheckDecl(decl); + } + } + } + typeCheckDelayedFunctions(*SF); }