From ac448b865bec76a964637156a8aa0b4e23775e50 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Wed, 4 Dec 2024 11:23:48 -0800 Subject: [PATCH 1/2] [Clang importer] Don't cache swift_attr source files that have CustomAttrs with arguments Since we can't do a proper "deep" clone of expression nodes, cloning such a CustomAttr is necessarily shallow. In such cases, don't cache the swift_attr source files at all, so we get fresh attribute nodes for each such usage. --- lib/ClangImporter/ImportDecl.cpp | 73 ++++++++++++++----- lib/ClangImporter/ImporterImpl.h | 3 +- .../usr/include/completion_handler_globals.h | 5 ++ test/Macros/Inputs/macro_library.swift | 7 ++ test/Macros/expand_on_imported.swift | 4 + 5 files changed, 74 insertions(+), 18 deletions(-) diff --git a/lib/ClangImporter/ImportDecl.cpp b/lib/ClangImporter/ImportDecl.cpp index ecb9122b09d6c..cad6dbd45ed54 100644 --- a/lib/ClangImporter/ImportDecl.cpp +++ b/lib/ClangImporter/ImportDecl.cpp @@ -8244,14 +8244,17 @@ bool importer::hasSameUnderlyingType(const clang::Type *a, SourceFile &ClangImporter::Implementation::getClangSwiftAttrSourceFile( ModuleDecl &module, - StringRef attributeText + StringRef attributeText, + bool cached ) { - auto &sourceFiles = ClangSwiftAttrSourceFiles[attributeText]; + if (cached) { + auto &sourceFiles = ClangSwiftAttrSourceFiles[attributeText]; - // Check whether we've already created a source file. - for (auto sourceFile : sourceFiles) { - if (sourceFile->getParentModule() == &module) - return *sourceFile; + // Check whether we've already created a source file. + for (auto sourceFile : sourceFiles) { + if (sourceFile->getParentModule() == &module) + return *sourceFile; + } } // Create a new buffer with a copy of the attribute text, @@ -8273,7 +8276,11 @@ SourceFile &ClangImporter::Implementation::getClangSwiftAttrSourceFile( // Create the source file. auto sourceFile = new (SwiftContext) SourceFile(module, SourceFileKind::Library, bufferID); - sourceFiles.push_back(sourceFile); + + if (cached) { + auto &sourceFiles = ClangSwiftAttrSourceFiles[attributeText]; + sourceFiles.push_back(sourceFile); + } return *sourceFile; } @@ -8450,17 +8457,49 @@ ClangImporter::Implementation::importSwiftAttrAttributes(Decl *MappedDecl) { continue; } - // Dig out a source file we can use for parsing. - auto &sourceFile = getClangSwiftAttrSourceFile( - *MappedDecl->getDeclContext()->getParentModule(), - swiftAttr->getAttribute()); + bool cached = true; + while (true) { + // Dig out a source file we can use for parsing. + auto &sourceFile = getClangSwiftAttrSourceFile( + *MappedDecl->getDeclContext()->getParentModule(), + swiftAttr->getAttribute(), + cached); + + auto topLevelDecls = sourceFile.getTopLevelDecls(); + + // If we're using the cached version, check whether we can correctly + // clone the attribute. + if (cached) { + bool hasNonclonableAttribute = false; + for (auto decl : topLevelDecls) { + if (hasNonclonableAttribute) + break; + + for (auto attr : decl->getAttrs()) { + if (auto customAttr = dyn_cast(attr)) { + if (customAttr->getArgs() != nullptr) { + hasNonclonableAttribute = true; + break; + } + } + } + } - // Collect the attributes from the synthesized top-level declaration in - // the source file. - auto topLevelDecls = sourceFile.getTopLevelDecls(); - for (auto decl : topLevelDecls) { - for (auto attr : decl->getAttrs()) - MappedDecl->getAttrs().add(attr->clone(SwiftContext)); + if (hasNonclonableAttribute) { + cached = false; + continue; + } + } + + // Collect the attributes from the synthesized top-level declaration in + // the source file. + for (auto decl : topLevelDecls) { + for (auto attr : decl->getAttrs()) { + MappedDecl->getAttrs().add(attr->clone(SwiftContext)); + } + } + + break; } } diff --git a/lib/ClangImporter/ImporterImpl.h b/lib/ClangImporter/ImporterImpl.h index 71d5d09498e64..1812911b224dc 100644 --- a/lib/ClangImporter/ImporterImpl.h +++ b/lib/ClangImporter/ImporterImpl.h @@ -1057,7 +1057,8 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation /// Retrieve the placeholder source file for use in parsing Swift attributes /// in the given module. - SourceFile &getClangSwiftAttrSourceFile(ModuleDecl &module, StringRef attributeText); + SourceFile &getClangSwiftAttrSourceFile( + ModuleDecl &module, StringRef attributeText, bool cached); /// Utility function to import Clang attributes from a source Swift decl to /// synthesized Swift decl. diff --git a/test/Inputs/clang-importer-sdk/usr/include/completion_handler_globals.h b/test/Inputs/clang-importer-sdk/usr/include/completion_handler_globals.h index e7e11c0986125..8743d43bbf228 100644 --- a/test/Inputs/clang-importer-sdk/usr/include/completion_handler_globals.h +++ b/test/Inputs/clang-importer-sdk/usr/include/completion_handler_globals.h @@ -1,9 +1,11 @@ #if __SWIFT_ATTR_SUPPORTS_MACROS #define ADD_ASYNC __attribute__((swift_attr("@macro_library.AddAsync"))) #define ADD_ASYNC_FINAL __attribute__((swift_attr("@macro_library.AddAsyncFinal"))) +#define DO_SOMETHING_DOTTED __attribute__((swift_attr("@AcceptDotted(.something)"))) #else #define ADD_ASYNC #define ADD_ASYNC_FINAL +#define DO_SOMETHING_DOTTED #endif void async_divide(double x, double y, void (^ _Nonnull completionHandler)(double x)) ADD_ASYNC; @@ -15,6 +17,9 @@ void computer_divide(const SlowComputer *computer, double x, double y, void (^ _ ADD_ASYNC __attribute__((swift_name("SlowComputer.divide(self:_:_:completionHandler:)"))); +void f1(double x) DO_SOMETHING_DOTTED; +void f2(double x) DO_SOMETHING_DOTTED; +void f3(double x) DO_SOMETHING_DOTTED; #if __OBJC__ @import Foundation; diff --git a/test/Macros/Inputs/macro_library.swift b/test/Macros/Inputs/macro_library.swift index e448634a12338..ac1b4212ae97e 100644 --- a/test/Macros/Inputs/macro_library.swift +++ b/test/Macros/Inputs/macro_library.swift @@ -59,3 +59,10 @@ public macro AddAsync() = #externalMacro(module: "MacroDefinition", type: "AddAs @attached(peer, names: overloaded) public macro AddAsyncFinal() = #externalMacro(module: "MacroDefinition", type: "AddAsyncMacro") + +public enum Something { +case something +} + +@attached(peer, names: overloaded) +public macro AcceptedDotted(_: Something) = #externalMacro(module: "MacroDefinition", type: "EmptyPeerMacro") diff --git a/test/Macros/expand_on_imported.swift b/test/Macros/expand_on_imported.swift index 8dd3384fa937a..3ae05b8a872c2 100644 --- a/test/Macros/expand_on_imported.swift +++ b/test/Macros/expand_on_imported.swift @@ -20,6 +20,10 @@ import macro_library func testAll(x: Double, y: Double, computer: SlowComputer) async { let _: Double = await async_divide(1.0, 2.0) let _: Double = await computer.divide(x, y) + + f1(3.14159) + f2(3.14159) + f3(3.14159) } // CHECK: define{{.*}}@"$sSC12async_divideyS2d_SdtYaF" From 0c91a3409c9f03bf72121835f2a1280515df2f71 Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Wed, 4 Dec 2024 15:09:17 -0800 Subject: [PATCH 2/2] Formalize the notion of "can clone" so we don't try to clone an attribute that won't work --- include/swift/AST/Attr.h | 10 +++++++++- lib/AST/Attr.cpp | 11 +++++++++++ lib/ClangImporter/ImportDecl.cpp | 19 +++++++++++-------- 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/include/swift/AST/Attr.h b/include/swift/AST/Attr.h index c5b9ab365f183..3f01ff67326f9 100644 --- a/include/swift/AST/Attr.h +++ b/include/swift/AST/Attr.h @@ -508,13 +508,17 @@ class DeclAttribute : public AttributeBase { /// Create a copy of this attribute. DeclAttribute *clone(ASTContext &ctx) const; + + /// Determine whether we can clone this attribute. + bool canClone() const; }; #define UNIMPLEMENTED_CLONE(AttrType) \ AttrType *clone(ASTContext &ctx) const { \ llvm_unreachable("unimplemented"); \ return nullptr; \ - } + } \ +bool canClone() const { return false; } /// Describes a "simple" declaration attribute that carries no data. template @@ -1916,9 +1920,13 @@ class CustomAttr final : public DeclAttribute { /// Create a copy of this attribute. CustomAttr *clone(ASTContext &ctx) const { + assert(argList == nullptr && + "Cannot clone custom attribute with an argument list"); return create(ctx, AtLoc, getTypeExpr(), initContext, argList, isImplicit()); } + bool canClone() const { return argList == nullptr; } + private: friend class CustomAttrNominalRequest; void resetTypeInformation(TypeExpr *repr); diff --git a/lib/AST/Attr.cpp b/lib/AST/Attr.cpp index 58cb749ca8074..a27bcd164ab54 100644 --- a/lib/AST/Attr.cpp +++ b/lib/AST/Attr.cpp @@ -370,6 +370,17 @@ DeclAttribute *DeclAttribute::clone(ASTContext &ctx) const { } } +bool DeclAttribute::canClone() const { + switch (getKind()) { +#define DECL_ATTR(_,CLASS, ...) \ + case DeclAttrKind::CLASS: \ + if (&CLASS##Attr::canClone == &DeclAttribute::canClone) \ + return true; \ + return static_cast(this)->canClone(); +#include "swift/AST/DeclAttr.def" + } +} + const BackDeployedAttr * DeclAttributes::getBackDeployed(const ASTContext &ctx, bool forTargetVariant) const { diff --git a/lib/ClangImporter/ImportDecl.cpp b/lib/ClangImporter/ImportDecl.cpp index cad6dbd45ed54..826fd25abef58 100644 --- a/lib/ClangImporter/ImportDecl.cpp +++ b/lib/ClangImporter/ImportDecl.cpp @@ -8476,15 +8476,15 @@ ClangImporter::Implementation::importSwiftAttrAttributes(Decl *MappedDecl) { break; for (auto attr : decl->getAttrs()) { - if (auto customAttr = dyn_cast(attr)) { - if (customAttr->getArgs() != nullptr) { - hasNonclonableAttribute = true; - break; - } + if (!attr->canClone()) { + hasNonclonableAttribute = true; + break; } } } + // We cannot clone one of the attributes. Go back and build a new + // source file without caching it. if (hasNonclonableAttribute) { cached = false; continue; @@ -8492,10 +8492,13 @@ ClangImporter::Implementation::importSwiftAttrAttributes(Decl *MappedDecl) { } // Collect the attributes from the synthesized top-level declaration in - // the source file. + // the source file. If we're using a cached copy, clone the attribute. for (auto decl : topLevelDecls) { - for (auto attr : decl->getAttrs()) { - MappedDecl->getAttrs().add(attr->clone(SwiftContext)); + SmallVector attrs(decl->getAttrs().begin(), + decl->getAttrs().end()); + for (auto attr : attrs) { + MappedDecl->getAttrs().add(cached ? attr->clone(SwiftContext) + : attr); } }