From 270f1d861902e8b510a46de5a8621175b0b37c0d Mon Sep 17 00:00:00 2001 From: "Henrik G. Olsson" Date: Wed, 3 Dec 2025 17:32:44 -0800 Subject: [PATCH 1/5] [Swiftify] don't add macro when required type is unavailable A Swift module only imports a type definition once. This means that some clang modules may have function signatures imported with `UnsafePointer` even if `qux` is only forward declared in that module (which would normally be imported as `OpaquePointer`), because some Swift file in the module imported the clang module with the definition, so ClangImporter sees the definition. The macro expansion only imports the imports of the clang module it belongs to however, so namelookup for `qux` fails. This patch detects when this will result in an error and avoids attaching the macro in those cases. rdar://165864358 --- lib/ClangImporter/SwiftifyDecl.cpp | 51 +++++- .../conflicting-opaqueness.swift | 148 ++++++++++++++++++ 2 files changed, 195 insertions(+), 4 deletions(-) create mode 100644 test/Interop/C/swiftify-import/conflicting-opaqueness.swift diff --git a/lib/ClangImporter/SwiftifyDecl.cpp b/lib/ClangImporter/SwiftifyDecl.cpp index 9a7770117092f..2b332fa5cffab 100644 --- a/lib/ClangImporter/SwiftifyDecl.cpp +++ b/lib/ClangImporter/SwiftifyDecl.cpp @@ -24,8 +24,10 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" +#include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/StmtVisitor.h" #include "clang/AST/Type.h" +#include "clang/Basic/Module.h" using namespace swift; using namespace importer; @@ -271,14 +273,15 @@ struct CountedByExpressionValidator }; -// Don't try to transform any Swift types that _SwiftifyImport doesn't know how -// to handle. -static bool SwiftifiableCountedByPointerType(Type swiftType) { +static bool IsConcretePointerType(Type swiftType) { Type nonnullType = swiftType->lookThroughSingleOptionalType(); PointerTypeKind PTK; return nonnullType->getAnyPointerElementType(PTK) && (PTK == PTK_UnsafePointer || PTK == PTK_UnsafeMutablePointer); } + +// Don't try to transform any Swift types that _SwiftifyImport doesn't know how +// to handle. static bool SwiftifiableSizedByPointerType(const clang::ASTContext &ctx, Type swiftType, const clang::CountAttributedType *CAT) { @@ -311,7 +314,7 @@ static bool SwiftifiableCAT(const clang::ASTContext &ctx, return CAT && CountedByExpressionValidator().Visit(CAT->getCountExpr()) && (CAT->isCountInBytes() ? SwiftifiableSizedByPointerType(ctx, swiftType, CAT) - : SwiftifiableCountedByPointerType(swiftType)); + : IsConcretePointerType(swiftType)); } // Searches for template instantiations that are not behind type aliases. @@ -331,6 +334,36 @@ struct UnaliasedInstantiationVisitor } }; +struct ForwardDeclaredConcreteTypeVisitor + : clang::RecursiveASTVisitor { + bool hasForwardDeclaredConcreteType = false; + clang::Module *Owner; + + ForwardDeclaredConcreteTypeVisitor(clang::Module *Owner) : Owner(Owner) {}; + + bool VisitRecordType(clang::RecordType *RT) { + const clang::RecordDecl *RD = RT->getDecl()->getDefinition(); + ASSERT(RD && "pointer to concrete type without type definition?"); + const clang::Module *M = RD->getOwningModule(); + if (!Owner->isModuleVisible(M)) { + hasForwardDeclaredConcreteType = true; + DLOG("Imported signature contains concrete type not available in clang module, skipping"); + LLVM_DEBUG(DUMP(RD)); + return false; + } + return true; + } + + bool IsIncompatibleImport(Type SwiftTy, clang::QualType ClangTy) { + DLOG_SCOPE("Checking compatibility of type: " << ClangTy << "\n"); + if (!IsConcretePointerType(SwiftTy)) { + return false; + } + TraverseType(ClangTy); + return hasForwardDeclaredConcreteType; + } +}; + // until CountAttributedType::getAttributeName lands in our LLVM branch static StringRef getAttributeName(const clang::CountAttributedType *CAT) { switch (CAT->getKind()) { @@ -375,6 +408,8 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self, clang::ASTContext &clangASTContext = Self.getClangASTContext(); + ForwardDeclaredConcreteTypeVisitor CheckForwardDecls(ClangDecl->getOwningModule()); + // We only attach the macro if it will produce an overload. Any __counted_by // will produce an overload, since UnsafeBufferPointer is still an improvement // over UnsafePointer, but std::span will only produce an overload if it also @@ -389,6 +424,10 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self, else ABORT("Unexpected AbstractFunctionDecl subclass."); clang::QualType clangReturnTy = ClangDecl->getReturnType(); + + if (CheckForwardDecls.IsIncompatibleImport(swiftReturnTy, clangReturnTy)) + return false; + bool returnIsStdSpan = printer.registerStdSpanTypeMapping( swiftReturnTy, clangReturnTy); auto *CAT = clangReturnTy->getAs(); @@ -440,6 +479,10 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self, } ASSERT(swiftParam); Type swiftParamTy = swiftParam->getInterfaceType(); + + if (CheckForwardDecls.IsIncompatibleImport(swiftParamTy, clangParamTy)) + return false; + bool paramHasBoundsInfo = false; auto *CAT = clangParamTy->getAs(); if (CAT && mappedIndex == SwiftifyInfoPrinter::SELF_PARAM_INDEX) { diff --git a/test/Interop/C/swiftify-import/conflicting-opaqueness.swift b/test/Interop/C/swiftify-import/conflicting-opaqueness.swift new file mode 100644 index 0000000000000..a000c541e74d1 --- /dev/null +++ b/test/Interop/C/swiftify-import/conflicting-opaqueness.swift @@ -0,0 +1,148 @@ +// REQUIRES: swift_feature_SafeInteropWrappers + +// RUN: %empty-directory(%t) +// RUN: split-file %s %t + +// RUN: %target-swift-frontend -emit-module -plugin-path %swift-plugin-dir -o %t/test.swiftmodule %t/test.swift -I %t -enable-experimental-feature SafeInteropWrappers -strict-memory-safety \ +// RUN: -verify -verify-additional-file %t%{fs-sep}foo.h -verify-additional-file %t%{fs-sep}bar.h -verify-additional-file %t%{fs-sep}baz.h -verify-additional-file %t%{fs-sep}qux.h -Rmacro-expansions + +// Macro expansions in foo.h do not have access to the definition of `struct qux`, +// so don't attach macro on functions that use contain that type in foo.h. +// bar.h imports the type, so attach the macro. +// baz.h imports bar.h which re-exports the type, so attach the macro there too. + +//--- foo.h +#pragma once + +#define __counted_by(x) __attribute__((__counted_by__(x))) + +struct qux; +// expected-note@+1{{'foo' declared here}} +void foo(struct qux *x, int * __counted_by(len) p, int len); +// expected-note@+1{{'fooReturn' declared here}} +struct qux * fooReturn(int * __counted_by(len) p, int len); + + +//--- bar.h +#pragma once +#include "qux.h" + +#define __counted_by(x) __attribute__((__counted_by__(x))) + +// expected-expansion@+10:59{{ +// expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}} +// expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func bar(_ x: UnsafeMutablePointer!, _ p: UnsafeMutableBufferPointer) {|}} +// expected-remark@3{{macro content: | let len = Int32(exactly: p.count)!|}} +// expected-remark@4{{macro content: | return unsafe bar(x, p.baseAddress!, len)|}} +// expected-remark@5{{macro content: |}|}} +// }} +// expected-expansion@+3:6{{ +// expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'bar' here}} +// }} +void bar(struct qux *x, int * __counted_by(len) p, int len); +// expected-expansion@+10:58{{ +// expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}} +// expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func barReturn(_ p: UnsafeMutableBufferPointer) -> UnsafeMutablePointer! {|}} +// expected-remark@3{{macro content: | let len = Int32(exactly: p.count)!|}} +// expected-remark@4{{macro content: | return unsafe barReturn(p.baseAddress!, len)|}} +// expected-remark@5{{macro content: |}|}} +// }} +// expected-expansion@+3:14{{ +// expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'barReturn' here}} +// }} +struct qux * barReturn(int * __counted_by(len) p, int len); + + +//--- baz.h +#pragma once + +#include "bar.h" + +struct qux; +// expected-expansion@+10:59{{ +// expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}} +// expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func baz(_ x: UnsafeMutablePointer!, _ p: UnsafeMutableBufferPointer) {|}} +// expected-remark@3{{macro content: | let len = Int32(exactly: p.count)!|}} +// expected-remark@4{{macro content: | return unsafe baz(x, p.baseAddress!, len)|}} +// expected-remark@5{{macro content: |}|}} +// }} +// expected-expansion@+3:6{{ +// expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'baz' here}} +// }} +void baz(struct qux *x, int * __counted_by(len) p, int len); +// expected-expansion@+10:58{{ +// expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}} +// expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func bazReturn(_ p: UnsafeMutableBufferPointer) -> UnsafeMutablePointer! {|}} +// expected-remark@3{{macro content: | let len = Int32(exactly: p.count)!|}} +// expected-remark@4{{macro content: | return unsafe bazReturn(p.baseAddress!, len)|}} +// expected-remark@5{{macro content: |}|}} +// }} +// expected-expansion@+3:14{{ +// expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'bazReturn' here}} +// }} +struct qux * bazReturn(int * __counted_by(len) p, int len); + + +//--- qux.h +#pragma once + +struct qux { int placeholder; }; + + +//--- test.swift +import Foo +import Bar +import Baz + +func callFoo(_ x: UnsafeMutablePointer, _ p: UnsafeMutablePointer, _ len: CInt) { + unsafe foo(x, p, len) + let _: UnsafeMutablePointer = unsafe fooReturn(p, len) +} + +func callFoo2(_ x: UnsafeMutablePointer, _ p: UnsafeMutableBufferPointer) { + // expected-error@+2{{missing argument for parameter #3 in call}} + // expected-error@+1{{cannot convert value of type 'UnsafeMutableBufferPointer' (aka 'UnsafeMutableBufferPointer') to expected argument type 'UnsafeMutablePointer'}} + unsafe foo(x, p) + // expected-error@+2{{missing argument for parameter #2 in call}} + // expected-error@+1{{cannot convert value of type 'UnsafeMutableBufferPointer' (aka 'UnsafeMutableBufferPointer') to expected argument type 'UnsafeMutablePointer'}} + let _: UnsafeMutablePointer = unsafe fooReturn(p) +} + + +func callBar(_ x: UnsafeMutablePointer, _ p: UnsafeMutablePointer, _ len: CInt) { + unsafe bar(x, p, len) + let _: UnsafeMutablePointer = unsafe barReturn(p, len) +} + +func callBar2(_ x: UnsafeMutablePointer, _ p: UnsafeMutableBufferPointer) { + unsafe bar(x, p) + let _: UnsafeMutablePointer = unsafe barReturn(p) +} + + +func callBaz(_ x: UnsafeMutablePointer, _ p: UnsafeMutablePointer, _ len: CInt) { + unsafe baz(x, p, len) + let _: UnsafeMutablePointer = unsafe bazReturn(p, len) +} + +func callBaz2(_ x: UnsafeMutablePointer, _ p: UnsafeMutableBufferPointer) { + unsafe baz(x, p) + let _: UnsafeMutablePointer = unsafe bazReturn(p) +} + + +//--- module.modulemap +module Foo { + header "foo.h" +} +module Bar { + header "bar.h" + export qux +} +module Baz { + header "baz.h" + export Bar +} +module qux { + header "qux.h" +} From 96d3deb03c196edf647d94d560c69710368a48cf Mon Sep 17 00:00:00 2001 From: "Henrik G. Olsson" Date: Tue, 16 Dec 2025 14:19:08 -0800 Subject: [PATCH 2/5] [Swiftify] fix missing owning module Implicit decls don't have an owning module, since multiple modules could instantiate them on demand. Since no implicit functions currently have any need for safe wrappers we can simply detect this and early exit. For templated functions, we need to fetch the owning module from the instantiation. --- lib/ClangImporter/SwiftifyDecl.cpp | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/lib/ClangImporter/SwiftifyDecl.cpp b/lib/ClangImporter/SwiftifyDecl.cpp index 2b332fa5cffab..54c59c177cad7 100644 --- a/lib/ClangImporter/SwiftifyDecl.cpp +++ b/lib/ClangImporter/SwiftifyDecl.cpp @@ -24,6 +24,7 @@ #include "clang/AST/ASTContext.h" #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" +#include "clang/AST/DeclObjC.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/StmtVisitor.h" #include "clang/AST/Type.h" @@ -337,9 +338,9 @@ struct UnaliasedInstantiationVisitor struct ForwardDeclaredConcreteTypeVisitor : clang::RecursiveASTVisitor { bool hasForwardDeclaredConcreteType = false; - clang::Module *Owner; + const clang::Module *Owner; - ForwardDeclaredConcreteTypeVisitor(clang::Module *Owner) : Owner(Owner) {}; + ForwardDeclaredConcreteTypeVisitor(const clang::Module *Owner) : Owner(Owner) { ASSERT(Owner); }; bool VisitRecordType(clang::RecordType *RT) { const clang::RecordDecl *RD = RT->getDecl()->getDefinition(); @@ -393,6 +394,13 @@ static size_t getNumParams(const clang::FunctionDecl* D) { } } // namespace +static const clang::Decl *getTemplateInstantiation(const clang::FunctionDecl *D) { + return D->getTemplateInstantiationPattern(); +} +static const clang::Decl *getTemplateInstantiation(const clang::ObjCMethodDecl *) { + return nullptr; +} + template static bool swiftifyImpl(ClangImporter::Implementation &Self, SwiftifyInfoFunctionPrinter &printer, @@ -406,9 +414,20 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self, ClangDecl->getAccess() == clang::AS_private) return false; + if (ClangDecl->isImplicit()) { + DLOG("implicit functions lack lifetime and bounds info"); + return false; + } + clang::ASTContext &clangASTContext = Self.getClangASTContext(); - ForwardDeclaredConcreteTypeVisitor CheckForwardDecls(ClangDecl->getOwningModule()); + const clang::Module *OwningModule = nullptr; + if (const auto *Instance = getTemplateInstantiation(ClangDecl)) { + OwningModule = Instance->getOwningModule(); + } else { + OwningModule = ClangDecl->getOwningModule(); + } + ForwardDeclaredConcreteTypeVisitor CheckForwardDecls(OwningModule); // We only attach the macro if it will produce an overload. Any __counted_by // will produce an overload, since UnsafeBufferPointer is still an improvement From cd5d8e13d9ffbb2c7191d1244a77358d4b53afb3 Mon Sep 17 00:00:00 2001 From: "Henrik G. Olsson" Date: Wed, 17 Dec 2025 16:22:42 -0800 Subject: [PATCH 3/5] handle bridging header decls in ForwardDeclaredConcreteTypeVisitor --- lib/ClangImporter/SwiftifyDecl.cpp | 34 ++++++++-- .../C/swiftify-import/bridging-header.swift | 67 +++++++++++++++++-- 2 files changed, 88 insertions(+), 13 deletions(-) diff --git a/lib/ClangImporter/SwiftifyDecl.cpp b/lib/ClangImporter/SwiftifyDecl.cpp index 54c59c177cad7..2b043e8801bce 100644 --- a/lib/ClangImporter/SwiftifyDecl.cpp +++ b/lib/ClangImporter/SwiftifyDecl.cpp @@ -228,7 +228,7 @@ struct CountedByExpressionValidator case clang::BuiltinType::ULong: case clang::BuiltinType::LongLong: case clang::BuiltinType::ULongLong: - DLOG("Ignoring count parameter with non-portable integer literal"); + DLOG("Ignoring count parameter with non-portable integer literal\n"); return false; default: return true; @@ -330,7 +330,7 @@ struct UnaliasedInstantiationVisitor bool VisitTemplateSpecializationType(const clang::TemplateSpecializationType *) { hasUnaliasedInstantiation = true; - DLOG("Signature contains raw template, skipping"); + DLOG("Signature contains raw template, skipping\n"); return false; } }; @@ -340,15 +340,33 @@ struct ForwardDeclaredConcreteTypeVisitor bool hasForwardDeclaredConcreteType = false; const clang::Module *Owner; - ForwardDeclaredConcreteTypeVisitor(const clang::Module *Owner) : Owner(Owner) { ASSERT(Owner); }; + ForwardDeclaredConcreteTypeVisitor(const clang::Module *Owner) : Owner(Owner) {}; bool VisitRecordType(clang::RecordType *RT) { const clang::RecordDecl *RD = RT->getDecl()->getDefinition(); ASSERT(RD && "pointer to concrete type without type definition?"); const clang::Module *M = RD->getOwningModule(); + + if (!Owner && !M) { + DLOG("Both decls are in bridging header"); + return true; + } + + if (!Owner) { + hasForwardDeclaredConcreteType = true; + DLOG("Imported signature contains concrete type not available in bridging header, skipping\n"); + LLVM_DEBUG(DUMP(RD)); + return false; + } + if (!M) { + ABORT([RD](auto &out) { + out << "Imported signature contains concrete type without an owning clang module:\n"; + RD->dump(out); + }); + } if (!Owner->isModuleVisible(M)) { hasForwardDeclaredConcreteType = true; - DLOG("Imported signature contains concrete type not available in clang module, skipping"); + DLOG("Imported signature contains concrete type not available in clang module, skipping\n"); LLVM_DEBUG(DUMP(RD)); return false; } @@ -415,7 +433,7 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self, return false; if (ClangDecl->isImplicit()) { - DLOG("implicit functions lack lifetime and bounds info"); + DLOG("implicit functions lack lifetime and bounds info\n"); return false; } @@ -427,6 +445,8 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self, } else { OwningModule = ClangDecl->getOwningModule(); } + bool IsInBridgingHeader = MappedDecl->getModuleContext()->getName().str() == CLANG_HEADER_MODULE_NAME; + ASSERT(OwningModule || IsInBridgingHeader); ForwardDeclaredConcreteTypeVisitor CheckForwardDecls(OwningModule); // We only attach the macro if it will produce an overload. Any __counted_by @@ -620,7 +640,7 @@ void ClangImporter::Implementation::swiftify(AbstractFunctionDecl *MappedDecl) { MacroDecl *SwiftifyImportDecl = dyn_cast_or_null(getKnownSingleDecl(SwiftContext, "_SwiftifyImport")); if (!SwiftifyImportDecl) { - DLOG("_SwiftifyImport macro not found"); + DLOG("_SwiftifyImport macro not found\n"); return; } @@ -672,7 +692,7 @@ void ClangImporter::Implementation::swiftifyProtocol( MacroDecl *SwiftifyImportDecl = dyn_cast_or_null( getKnownSingleDecl(SwiftContext, "_SwiftifyImportProtocol")); if (!SwiftifyImportDecl) { - DLOG("_SwiftifyImportProtocol macro not found"); + DLOG("_SwiftifyImportProtocol macro not found\n"); return; } diff --git a/test/Interop/C/swiftify-import/bridging-header.swift b/test/Interop/C/swiftify-import/bridging-header.swift index 4124fbb12b52c..bf7c9667f7f7a 100644 --- a/test/Interop/C/swiftify-import/bridging-header.swift +++ b/test/Interop/C/swiftify-import/bridging-header.swift @@ -16,6 +16,9 @@ imports for TMP_DIR/test.swift: _StringProcessing _SwiftConcurrencyShims _Concurrency + A + B + C imports for __ObjC.foo: imports for @__swiftmacro_So3foo15_SwiftifyImportfMp_.swift: __ObjC @@ -25,21 +28,73 @@ imports for @__swiftmacro_So3foo15_SwiftifyImportfMp_.swift: @__swiftmacro_So3foo15_SwiftifyImportfMp_.swift ------------------------------ /// This is an auto-generated wrapper for safer interop -@_alwaysEmitIntoClient @available(visionOS 1.0, tvOS 12.2, watchOS 5.2, iOS 12.2, macOS 10.14.4, *) @_disfavoredOverload public func foo(_ p: Span) { - let len = Int32(exactly: p.count)! +@_alwaysEmitIntoClient @available(visionOS 1.0, tvOS 12.2, watchOS 5.2, iOS 12.2, macOS 10.14.4, *) @_disfavoredOverload public func foo(_ p: Span, _ x: UnsafeMutablePointer!) { + let len = no_module_t(exactly: p.count)! return unsafe p.withUnsafeBufferPointer { _pPtr in - return unsafe foo(len, _pPtr.baseAddress!) + return unsafe foo(len, _pPtr.baseAddress!, x) } } ------------------------------ //--- test.swift -func test(s: Span) { - foo(s) +import A +import B +import C + +func test(s: Span, x: UnsafeMutablePointer) { + unsafe foo(s, x) +} + +func test2(p: UnsafeMutablePointer, len: CInt, y: UnsafeMutablePointer) { + unsafe bar(len, p, y) +} + +func test3(p: UnsafeMutablePointer, len: CInt, z: UnsafeMutablePointer) { + unsafe baz(len, p, z) } //--- bridging.h #include #include -void foo(int len, const int * __counted_by(len) p __noescape); +#include "no-module.h" +#include "a.h" +#include "c.h" + +struct no_module_record_t; +void foo(no_module_t len, const a_t * __counted_by(len) p __noescape, struct no_module_record_t *x); + +struct b_t; +void bar(int len, const int * __counted_by(len) p __noescape, struct b_t *y); + +void baz(int len, const int * __counted_by(len) p __noescape, struct c_t *z); + +//--- no-module.h +typedef int no_module_t; +struct no_module_record_t { + int placeholder; +}; + +//--- a.h +typedef int a_t; + +//--- b.h +struct b_t { + int placeholder; +}; + +//--- c.h +struct c_t { + int placeholder; +}; + +//--- module.modulemap +module A { + header "a.h" +} +module B { + header "b.h" +} +module C { + header "c.h" +} From db7c13c3dd65f860b93d926fcd83edf9a5c48d41 Mon Sep 17 00:00:00 2001 From: "Henrik G. Olsson" Date: Wed, 17 Dec 2025 22:09:59 -0800 Subject: [PATCH 4/5] traverse imported Swift type rather than clang type This fixes an assert when signatures contain `struct forward_declared_t **`, and makes sure that `struct foo { struct forward_declared_t *bar; }` does *not* block swiftification, since `@_SwiftifyImport` does not need to access the struct field. --- lib/ClangImporter/SwiftifyDecl.cpp | 121 +++++++++++------- .../conflicting-opaqueness.swift | 27 +++- 2 files changed, 97 insertions(+), 51 deletions(-) diff --git a/lib/ClangImporter/SwiftifyDecl.cpp b/lib/ClangImporter/SwiftifyDecl.cpp index 2b043e8801bce..89ad699604b17 100644 --- a/lib/ClangImporter/SwiftifyDecl.cpp +++ b/lib/ClangImporter/SwiftifyDecl.cpp @@ -20,11 +20,13 @@ #include "swift/AST/Decl.h" #include "swift/AST/DiagnosticsClangImporter.h" #include "swift/AST/ParameterList.h" +#include "swift/AST/TypeWalker.h" #include "clang/AST/ASTContext.h" #include "clang/AST/Attr.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclObjC.h" +#include "clang/AST/DeclTemplate.h" #include "clang/AST/RecursiveASTVisitor.h" #include "clang/AST/StmtVisitor.h" #include "clang/AST/Type.h" @@ -274,11 +276,13 @@ struct CountedByExpressionValidator }; -static bool IsConcretePointerType(Type swiftType) { +static Type ConcretePointeeType(Type swiftType) { Type nonnullType = swiftType->lookThroughSingleOptionalType(); PointerTypeKind PTK; - return nonnullType->getAnyPointerElementType(PTK) && - (PTK == PTK_UnsafePointer || PTK == PTK_UnsafeMutablePointer); + Type PointeeTy = nonnullType->getAnyPointerElementType(PTK); + if (PointeeTy && (PTK == PTK_UnsafePointer || PTK == PTK_UnsafeMutablePointer)) + return PointeeTy; + return Type(); } // Don't try to transform any Swift types that _SwiftifyImport doesn't know how @@ -315,7 +319,7 @@ static bool SwiftifiableCAT(const clang::ASTContext &ctx, return CAT && CountedByExpressionValidator().Visit(CAT->getCountExpr()) && (CAT->isCountInBytes() ? SwiftifiableSizedByPointerType(ctx, swiftType, CAT) - : IsConcretePointerType(swiftType)); + : !ConcretePointeeType(swiftType).isNull()); } // Searches for template instantiations that are not behind type aliases. @@ -335,50 +339,81 @@ struct UnaliasedInstantiationVisitor } }; -struct ForwardDeclaredConcreteTypeVisitor - : clang::RecursiveASTVisitor { +static const clang::Decl *getTemplateInstantiation(const clang::Decl *D) { + if (auto FuncD = dyn_cast(D)) { + return FuncD->getTemplateInstantiationPattern(); + } + if (auto RecordD = dyn_cast(D)) { + return RecordD->getTemplateInstantiationPattern(); + } + if (auto EnumD = dyn_cast(D)) { + return EnumD->getTemplateInstantiationPattern(); + } + if (auto VarD = dyn_cast(D)) { + return VarD->getTemplateInstantiationPattern(); + } + return nullptr; +} + +static clang::Module *getOwningModule(const clang::Decl *ClangDecl) { + if (const auto *Instance = getTemplateInstantiation(ClangDecl)) { + return Instance->getOwningModule(); + } + return ClangDecl->getOwningModule(); +} + +struct ForwardDeclaredConcreteTypeVisitor : public TypeWalker { bool hasForwardDeclaredConcreteType = false; const clang::Module *Owner; ForwardDeclaredConcreteTypeVisitor(const clang::Module *Owner) : Owner(Owner) {}; - bool VisitRecordType(clang::RecordType *RT) { - const clang::RecordDecl *RD = RT->getDecl()->getDefinition(); - ASSERT(RD && "pointer to concrete type without type definition?"); - const clang::Module *M = RD->getOwningModule(); + Action walkToTypePre(Type ty) override { + DLOG("Walking type:\n"); + LLVM_DEBUG(DUMP(ty)); + if (Type PointeeTy = ConcretePointeeType(ty)) { + auto *Nom = PointeeTy->getAnyNominal(); + const clang::Decl *ClangDecl = Nom->getClangDecl(); + if (!ClangDecl) { + return Action::Continue; + } - if (!Owner && !M) { - DLOG("Both decls are in bridging header"); - return true; - } + if (auto RD = dyn_cast(ClangDecl)) { + const clang::RecordDecl *Def = RD->getDefinition(); + ASSERT(Def && "pointer to concrete type without type definition?"); + const clang::Module *M = getOwningModule(ClangDecl); - if (!Owner) { - hasForwardDeclaredConcreteType = true; - DLOG("Imported signature contains concrete type not available in bridging header, skipping\n"); - LLVM_DEBUG(DUMP(RD)); - return false; - } - if (!M) { - ABORT([RD](auto &out) { - out << "Imported signature contains concrete type without an owning clang module:\n"; - RD->dump(out); - }); - } - if (!Owner->isModuleVisible(M)) { - hasForwardDeclaredConcreteType = true; - DLOG("Imported signature contains concrete type not available in clang module, skipping\n"); - LLVM_DEBUG(DUMP(RD)); - return false; + if (!Owner && !M) { + DLOG("Both decls are in bridging header\n"); + return Action::Continue; + } + + if (!Owner) { + hasForwardDeclaredConcreteType = true; + DLOG("Imported signature contains concrete type not available in bridging header, skipping\n"); + LLVM_DEBUG(DUMP(Def)); + return Action::Stop; + } + if (!M) { + ABORT([Def](auto &out) { + out << "Imported signature contains concrete type without an owning clang module:\n"; + Def->dump(out); + }); + } + if (!Owner->isModuleVisible(M)) { + hasForwardDeclaredConcreteType = true; + DLOG("Imported signature contains concrete type not available in clang module, skipping\n"); + LLVM_DEBUG(DUMP(Def)); + return Action::Stop; + } + } } - return true; + return Action::Continue; } bool IsIncompatibleImport(Type SwiftTy, clang::QualType ClangTy) { DLOG_SCOPE("Checking compatibility of type: " << ClangTy << "\n"); - if (!IsConcretePointerType(SwiftTy)) { - return false; - } - TraverseType(ClangTy); + SwiftTy.walk(*this); return hasForwardDeclaredConcreteType; } }; @@ -412,13 +447,6 @@ static size_t getNumParams(const clang::FunctionDecl* D) { } } // namespace -static const clang::Decl *getTemplateInstantiation(const clang::FunctionDecl *D) { - return D->getTemplateInstantiationPattern(); -} -static const clang::Decl *getTemplateInstantiation(const clang::ObjCMethodDecl *) { - return nullptr; -} - template static bool swiftifyImpl(ClangImporter::Implementation &Self, SwiftifyInfoFunctionPrinter &printer, @@ -439,12 +467,7 @@ static bool swiftifyImpl(ClangImporter::Implementation &Self, clang::ASTContext &clangASTContext = Self.getClangASTContext(); - const clang::Module *OwningModule = nullptr; - if (const auto *Instance = getTemplateInstantiation(ClangDecl)) { - OwningModule = Instance->getOwningModule(); - } else { - OwningModule = ClangDecl->getOwningModule(); - } + const clang::Module *OwningModule = getOwningModule(ClangDecl); bool IsInBridgingHeader = MappedDecl->getModuleContext()->getName().str() == CLANG_HEADER_MODULE_NAME; ASSERT(OwningModule || IsInBridgingHeader); ForwardDeclaredConcreteTypeVisitor CheckForwardDecls(OwningModule); diff --git a/test/Interop/C/swiftify-import/conflicting-opaqueness.swift b/test/Interop/C/swiftify-import/conflicting-opaqueness.swift index a000c541e74d1..6561bd1e6bf65 100644 --- a/test/Interop/C/swiftify-import/conflicting-opaqueness.swift +++ b/test/Interop/C/swiftify-import/conflicting-opaqueness.swift @@ -19,9 +19,26 @@ struct qux; // expected-note@+1{{'foo' declared here}} void foo(struct qux *x, int * __counted_by(len) p, int len); +// expected-note@+1{{'fooIndirect' declared here}} +void fooIndirect(struct qux * * x, int * __counted_by(len) p, int len); // expected-note@+1{{'fooReturn' declared here}} struct qux * fooReturn(int * __counted_by(len) p, int len); +struct container_t { + struct qux *item; +}; +// expected-expansion@+10:6{{ +// expected-note@1 5{{in expansion of macro '_SwiftifyImport' on global function 'fooWrapped' here}} +// }} +// expected-expansion@+7:75{{ +// expected-remark@2{{macro content: |@_alwaysEmitIntoClient @_disfavoredOverload public func fooWrapped(_ x: UnsafeMutablePointer!, _ p: UnsafeMutableBufferPointer) {|}} +// expected-remark@3{{macro content: | let len = Int32(exactly: p.count)!|}} +// expected-remark@4{{macro content: | return unsafe fooWrapped(x, p.baseAddress!, len)|}} +// expected-remark@5{{macro content: |}|}} +// expected-remark@1{{macro content: |/// This is an auto-generated wrapper for safer interop|}} +// }} +void fooWrapped(struct container_t * x, int * __counted_by(len) p, int len); + //--- bar.h #pragma once @@ -94,18 +111,24 @@ import Foo import Bar import Baz -func callFoo(_ x: UnsafeMutablePointer, _ p: UnsafeMutablePointer, _ len: CInt) { +func callFoo(_ x: UnsafeMutablePointer, _ y: UnsafeMutablePointer?>, _ z: UnsafeMutablePointer, _ p: UnsafeMutablePointer, _ len: CInt) { unsafe foo(x, p, len) + unsafe fooIndirect(y, p, len) let _: UnsafeMutablePointer = unsafe fooReturn(p, len) + unsafe fooWrapped(z, p, len) } -func callFoo2(_ x: UnsafeMutablePointer, _ p: UnsafeMutableBufferPointer) { +func callFoo2(_ x: UnsafeMutablePointer, _ y: UnsafeMutablePointer?>, _ z: UnsafeMutablePointer, _ p: UnsafeMutableBufferPointer) { // expected-error@+2{{missing argument for parameter #3 in call}} // expected-error@+1{{cannot convert value of type 'UnsafeMutableBufferPointer' (aka 'UnsafeMutableBufferPointer') to expected argument type 'UnsafeMutablePointer'}} unsafe foo(x, p) + // expected-error@+2{{missing argument for parameter #3 in call}} + // expected-error@+1{{cannot convert value of type 'UnsafeMutableBufferPointer' (aka 'UnsafeMutableBufferPointer') to expected argument type 'UnsafeMutablePointer'}} + unsafe fooIndirect(y, p) // expected-error@+2{{missing argument for parameter #2 in call}} // expected-error@+1{{cannot convert value of type 'UnsafeMutableBufferPointer' (aka 'UnsafeMutableBufferPointer') to expected argument type 'UnsafeMutablePointer'}} let _: UnsafeMutablePointer = unsafe fooReturn(p) + unsafe fooWrapped(z, p) } From ca3b77f6fca2bd56890ac0ffff67bc3feada59e6 Mon Sep 17 00:00:00 2001 From: "Henrik G. Olsson" Date: Thu, 18 Dec 2025 17:24:08 -0800 Subject: [PATCH 5/5] allow functions in modules to depend on decls with no owning module Since decls with no module are imported to __ObjC this is not an issue, because __ObjC is always implicitly imported. Added test case that would previously assert. --- lib/ClangImporter/SwiftifyDecl.cpp | 10 +-- .../bridging-header-from-module.swift | 71 +++++++++++++++++++ 2 files changed, 73 insertions(+), 8 deletions(-) create mode 100644 test/Interop/C/swiftify-import/bridging-header-from-module.swift diff --git a/lib/ClangImporter/SwiftifyDecl.cpp b/lib/ClangImporter/SwiftifyDecl.cpp index 89ad699604b17..5ceacfce7ee10 100644 --- a/lib/ClangImporter/SwiftifyDecl.cpp +++ b/lib/ClangImporter/SwiftifyDecl.cpp @@ -383,8 +383,8 @@ struct ForwardDeclaredConcreteTypeVisitor : public TypeWalker { ASSERT(Def && "pointer to concrete type without type definition?"); const clang::Module *M = getOwningModule(ClangDecl); - if (!Owner && !M) { - DLOG("Both decls are in bridging header\n"); + if (!M) { + DLOG("Concrete type is in bridging header, which is always imported\n"); return Action::Continue; } @@ -394,12 +394,6 @@ struct ForwardDeclaredConcreteTypeVisitor : public TypeWalker { LLVM_DEBUG(DUMP(Def)); return Action::Stop; } - if (!M) { - ABORT([Def](auto &out) { - out << "Imported signature contains concrete type without an owning clang module:\n"; - Def->dump(out); - }); - } if (!Owner->isModuleVisible(M)) { hasForwardDeclaredConcreteType = true; DLOG("Imported signature contains concrete type not available in clang module, skipping\n"); diff --git a/test/Interop/C/swiftify-import/bridging-header-from-module.swift b/test/Interop/C/swiftify-import/bridging-header-from-module.swift new file mode 100644 index 0000000000000..a6f0a7adf5c5f --- /dev/null +++ b/test/Interop/C/swiftify-import/bridging-header-from-module.swift @@ -0,0 +1,71 @@ +// REQUIRES: swift_feature_SafeInteropWrappers + +// RUN: %empty-directory(%t) +// RUN: split-file %s %t + +// RUN: %target-swift-frontend -typecheck -plugin-path %swift-plugin-dir -o %t/test.swiftmodule -I %t -import-objc-header %t/bridging.h -enable-experimental-feature SafeInteropWrappers -strict-memory-safety -warnings-as-errors -Xcc -Werror -Xcc -Wno-nullability-completeness -Xcc -Wno-div-by-zero -Xcc -Wno-pointer-to-int-cast %t/test.swift -verify +// RUN: %target-swift-frontend -typecheck -plugin-path %swift-plugin-dir -o %t/test.swiftmodule -I %t -import-objc-header %t/bridging.h -enable-experimental-feature SafeInteropWrappers -strict-memory-safety -warnings-as-errors -Xcc -Werror -Xcc -Wno-nullability-completeness -Xcc -Wno-div-by-zero -Xcc -Wno-pointer-to-int-cast %t/test.swift -dump-macro-expansions 2>&1 | %FileCheck --dry-run > %t/macro-expansions.out +// RUN: %diff %t/macro-expansions.out %t/macro-expansions.expected +// RUN: %target-swift-frontend -typecheck -plugin-path %swift-plugin-dir -o %t/test.swiftmodule -I %t -import-objc-header %t/bridging.h -enable-experimental-feature SafeInteropWrappers -strict-memory-safety -warnings-as-errors -Xcc -Werror -Xcc -Wno-nullability-completeness -Xcc -Wno-div-by-zero -Xcc -Wno-pointer-to-int-cast %t/test.swift -dump-source-file-imports 2>&1 | %FileCheck --dry-run > %t/imports.out +// RUN: %diff %t/imports.out %t/imports.expected + +//--- imports.expected +imports for TMP_DIR/test.swift: + Swift + __ObjC + _StringProcessing + _SwiftConcurrencyShims + _Concurrency + A +imports for A.foo: +imports for @__swiftmacro_So3foo15_SwiftifyImportfMp_.swift: + Swift + __ObjC + lifetimebound + ptrcheck + _StringProcessing + _SwiftConcurrencyShims + _Concurrency + +//--- macro-expansions.expected +@__swiftmacro_So3foo15_SwiftifyImportfMp_.swift +------------------------------ +/// This is an auto-generated wrapper for safer interop +@_alwaysEmitIntoClient @available(visionOS 1.0, tvOS 12.2, watchOS 5.2, iOS 12.2, macOS 10.14.4, *) @_disfavoredOverload public func foo(_ p: Span, _ x: UnsafeMutablePointer!) { + let len = Int32(exactly: p.count)! + return unsafe p.withUnsafeBufferPointer { _pPtr in + return unsafe foo(len, _pPtr.baseAddress!, x) + } +} +------------------------------ + +//--- test.swift +import A + +func test(s: Span, x: UnsafeMutablePointer) { + unsafe foo(s, x) +} + +//--- bridging.h +// claim this header as part of bridging header, making it not belong to a module +#include "no-module.h" + +//--- no-module.h +#pragma once +typedef int no_module_t; +struct no_module_record_t { + int placeholder; +}; + +//--- a.h +#include +#include +// this module header now depends on a decl with no module +#include "no-module.h" +typedef int a_t; +void foo(int len, const int * __counted_by(len) p __noescape, struct no_module_record_t *x); + +//--- module.modulemap +module A { + header "a.h" +}