From da0d42859d289f38566949a69e6f1a43c2e32337 Mon Sep 17 00:00:00 2001 From: Becca Royal-Gordon Date: Tue, 21 May 2024 17:33:21 -0700 Subject: [PATCH] [C++] Fix double pointers to foreign ref types When a C++ type `ForeignTy` is imported as a foreign reference type, Swift should strip a level of indirection from parameters of that type, so e.g. `ForeignTy *` and `ForeignTy &` should be imported to Swift as just `ForeignTy`. However, it should only strip *one* level of indirection. importType() was instead stripping *all* levels, so things like `ForeignTy **` and `ForeignTy *&` were *also* being incorrectly imported as `ForeignTy`. Narrow this behavior so it only applies to a single level of indirection and add a few test cases to pin down the specifics. Fixes rdar://123905345. --- lib/ClangImporter/ImportType.cpp | 30 ++++++++++---- .../swift-bridging-annotations.swift | 39 +++++++++++++++++++ 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/lib/ClangImporter/ImportType.cpp b/lib/ClangImporter/ImportType.cpp index 3387648045875..b0a9a742acc5c 100644 --- a/lib/ClangImporter/ImportType.cpp +++ b/lib/ClangImporter/ImportType.cpp @@ -201,6 +201,13 @@ namespace { : ImportHint::OtherPointer}; } + static bool + isDirectUseOfForeignReferenceType(clang::QualType clangPointeeType, + Type swiftPointeeType) { + return swiftPointeeType && swiftPointeeType->isForeignReferenceType() && + !clangPointeeType->isPointerType(); + } + class SwiftTypeConverter : public clang::TypeVisitor { @@ -494,9 +501,10 @@ namespace { pointeeQualType, ImportTypeKind::Value, addImportDiagnostic, AllowNSUIntegerAsInt, Bridgeability::None, ImportTypeAttrs()); - // If this is imported as a reference type, ignore the pointer. - if (pointeeType && pointeeType->isForeignReferenceType()) - return {pointeeType, ImportHint::OtherPointer}; + // If this is imported as a reference type, ignore the innermost pointer. + // (`T *` becomes `T`, but `T **` becomes `UnsafeMutablePointer`.) + if (isDirectUseOfForeignReferenceType(pointeeQualType, pointeeType)) + return {pointeeType, ImportHint::OtherPointer}; // If the pointed-to type is unrepresentable in Swift, or its C // alignment is greater than the maximum Swift alignment, import as @@ -592,7 +600,7 @@ namespace { if (!pointeeType) return Type(); - if (pointeeType->isForeignReferenceType()) + if (isDirectUseOfForeignReferenceType(pointeeQualType, pointeeType)) return {pointeeType, ImportHint::None}; if (pointeeQualType->isFunctionType()) { @@ -2531,6 +2539,15 @@ ClangImporter::Implementation::importParameterType( swiftParamTy = importedType.getType(); } + // `isInOut` is set above if we stripped off a mutable `&` before importing + // the type. Normally, we want to use an `inout` parameter in this situation. + // However, if the parameter belongs to a foreign reference type *and* the + // reference we stripped out was directly to that type (rather than to a + // pointer to that type), the foreign reference type should "eat" the + // indirection of the `&`, so we *don't* want to use an `inout` parameter. + if (isInOut && isDirectUseOfForeignReferenceType(paramTy, swiftParamTy)) + isInOut = false; + return ImportParameterTypeResult{swiftParamTy, isInOut, isParamTypeImplicitlyUnwrapped}; } @@ -2611,9 +2628,8 @@ static ParamDecl *getParameterInfo(ClangImporter::Implementation *impl, // Foreign references are already references so they don't need to be passed // as inout. - paramInfo->setSpecifier(isInOut && !swiftParamTy->isForeignReferenceType() - ? ParamSpecifier::InOut - : ParamSpecifier::Default); + paramInfo->setSpecifier(isInOut ? ParamSpecifier::InOut + : ParamSpecifier::Default); paramInfo->setInterfaceType(swiftParamTy); impl->recordImplicitUnwrapForDecl(paramInfo, isParamTypeImplicitlyUnwrapped); diff --git a/test/Interop/Cxx/ergonomics/swift-bridging-annotations.swift b/test/Interop/Cxx/ergonomics/swift-bridging-annotations.swift index 2b3514ab0bf20..76336400ec295 100644 --- a/test/Interop/Cxx/ergonomics/swift-bridging-annotations.swift +++ b/test/Interop/Cxx/ergonomics/swift-bridging-annotations.swift @@ -27,6 +27,19 @@ public protocol Proto { #if BRIDGING_HEADER_TEST func f() -> SharedObject { return SharedObject.create() } +func g() { + var logger: LoggerSingleton? + var loggerPtr: UnsafeMutablePointer? + var loggerPtrPtr: UnsafeMutablePointer?>? + + takeLoggersByPointer(logger, &logger, &loggerPtr) + takeLoggersByPointer(logger, loggerPtr, loggerPtrPtr) + takeLoggersByPointer(nil, nil, nil) + + takeLoggersByReference(logger!, &logger, &loggerPtr) + takeLoggersByReference(logger!, &loggerPtr!.pointee, &loggerPtrPtr!.pointee) +} + func releaseSharedObject(_: SharedObject) { } #endif @@ -71,6 +84,12 @@ public: static LoggerSingleton *getInstance(); }; +void takeLoggersByPointer(LoggerSingleton *ptr, LoggerSingleton **ptr_ptr, LoggerSingleton ***ptr_ptr_ptr); +void takeLoggersByReference(LoggerSingleton &ref, LoggerSingleton *&ref_ptr, LoggerSingleton **&ref_ptr_ptr); + +void takeLoggersByConstPointer(const LoggerSingleton **pointee0, LoggerSingleton const **pointee1, LoggerSingleton *const *pointer); +void takeLoggersByConstReference(const LoggerSingleton *&pointee0, LoggerSingleton const *&pointee1, LoggerSingleton *const &pointer); + class SWIFT_UNSAFE_REFERENCE UnsafeNonCopyable { public: UnsafeNonCopyable(UnsafeNonCopyable &) = delete; @@ -110,6 +129,26 @@ private: // CHECK: class func getInstance() -> LoggerSingleton! // CHECK: } +// CHECK-LABEL: func takeLoggersByPointer( +// CHECK-SAME: _ ptr: LoggerSingleton!, +// CHECK-SAME: _ ptr_ptr: UnsafeMutablePointer!, +// CHECK-SAME: _ ptr_ptr_ptr: UnsafeMutablePointer?>!) + +// CHECK-LABEL: func takeLoggersByReference( +// CHECK-SAME: _ ref: LoggerSingleton, +// CHECK-SAME: _ ref_ptr: inout LoggerSingleton!, +// CHECK-SAME: _ ref_ptr_ptr: inout UnsafeMutablePointer!) + +// CHECK-LABEL: func takeLoggersByConstPointer( +// CHECK-SAME: _ pointee0: UnsafeMutablePointer!, +// CHECK-SAME: _ pointee1: UnsafeMutablePointer!, +// CHECK-SAME: _ pointer: UnsafePointer!) + +// CHECK-LABEL: func takeLoggersByConstReference( +// CHECK-SAME: _ pointee0: inout LoggerSingleton!, +// CHECK-SAME: _ pointee1: inout LoggerSingleton!, +// CHECK-SAME: _ pointer: LoggerSingleton!) + // CHECK: class UnsafeNonCopyable { // CHECK: } // CHECK: func returnsPointerToUnsafeReference() -> UnsafeNonCopyable!