Merge pull request #12729 from jrose-apple/bridging-typedefs-is-hard

[ClangImporter] Bridging can happen even without full bridgeability

Author: jrose-apple

lib/ClangImporter/ImportDecl.cpp

@@ -3476,6 +3476,8 @@ namespace {
                                                 Impl.CurrentVersion))
           declType = Impl.getClangASTContext().getTypedefType(newtypeDecl);
 
+      // Note that we deliberately don't bridge most globals because we want to
+      // preserve pointer identity.
       Type type = Impl.importType(declType,
                                   (isAudited ? ImportTypeKind::AuditedVariable
                                    : ImportTypeKind::Variable),

lib/ClangImporter/ImportType.cpp

@@ -684,13 +684,18 @@ namespace {
         auto typedefBridgeability = getTypedefBridgeability(underlyingType);
 
         // Figure out the typedef we should actually use.
-        SwiftTypeConverter innerConverter(Impl, AllowNSUIntegerAsInt, Bridging);
+        auto underlyingBridgeability =
+          (Bridging == Bridgeability::Full
+             ? typedefBridgeability : Bridgeability::None);
+        SwiftTypeConverter innerConverter(Impl, AllowNSUIntegerAsInt,
+                                          underlyingBridgeability);
         auto underlyingResult = innerConverter.Visit(underlyingType);
 
         // If we used different bridgeability than this typedef normally
-        // would, and therefore the underlying type is different from the
-        // mapping of the typedef, use the underlying type.
-        if (Bridging != typedefBridgeability &&
+        // would because we're in a non-bridgeable context, and therefore
+        // the underlying type is different from the mapping of the typedef,
+        // use the underlying type.
+        if (underlyingBridgeability != typedefBridgeability &&
             !underlyingResult.AbstractType->isEqual(mappedType)) {
           return underlyingResult;
         }
@@ -1291,29 +1296,9 @@ static Type adjustTypeForConcreteImport(ClangImporter::Implementation &impl,
 
   // SwiftTypeConverter turns block pointers into @convention(block) types.
   // In a bridgeable context, or in the direct structure of a typedef,
-  // we would prefer to instead use the default Swift convention.  But this
-  // does means that, when we're using a typedef of a block pointer type in
-  // an unbridgable context, we need to go back and do a fully-unbridged
-  // import of the underlying type.
+  // we would prefer to instead use the default Swift convention.
   if (hint == ImportHint::Block) {
-    if (bridging == Bridgeability::None) {
-      if (auto typedefType = clangType->getAs<clang::TypedefType>()) {
-        // In non-bridged contexts, drop the typealias sugar for blocks.
-        // FIXME: This will do the wrong thing if there's any adjustment to do
-        // besides optionality.
-        Type underlyingTy = impl.importType(typedefType->desugar(),
-                                            importKind,
-                                            allowNSUIntegerAsInt,
-                                            bridging,
-                                            OTK_None);
-        if (Type unwrappedTy = underlyingTy->getAnyOptionalObjectType())
-          underlyingTy = unwrappedTy;
-        if (!underlyingTy->isEqual(importedType))
-          importedType = underlyingTy;
-      }
-    }
-
-    if (bridging == Bridgeability::Full) {
+    if (canBridgeTypes(importKind) || importKind == ImportTypeKind::Typedef) {
       auto fTy = importedType->castTo<FunctionType>();
       FunctionType::ExtInfo einfo = fTy->getExtInfo();
       if (einfo.getRepresentation() != FunctionTypeRepresentation::Swift) {
@@ -1356,7 +1341,6 @@ static Type adjustTypeForConcreteImport(ClangImporter::Implementation &impl,
   // If we have a bridged Objective-C type and we are allowed to
   // bridge, do so.
   if (hint == ImportHint::ObjCBridged &&
-      bridging == Bridgeability::Full &&
       canBridgeTypes(importKind) &&
       importKind != ImportTypeKind::PropertyWithReferenceSemantics) {
     // id and Any can be bridged without Foundation. There would be

lib/ClangImporter/ImporterImpl.h

@@ -191,10 +191,13 @@ enum class Bridgeability {
   Full
 };
 
+/// Controls whether a typedef for \p type should name the fully-bridged Swift
+/// type or the original Clang type.
+///
+/// In either case we end up losing sugar at some uses sites, so this is more
+/// about what the right default is.
 static inline Bridgeability getTypedefBridgeability(clang::QualType type) {
-  return (type->isBlockPointerType() || type->isFunctionType())
-            ? Bridgeability::Full
-            : Bridgeability::None;
+  return type->isBlockPointerType() ? Bridgeability::Full : Bridgeability::None;
 }
 
 /// \brief Describes the kind of the C type that can be mapped to a stdlib
@@ -907,22 +910,45 @@ class LLVM_LIBRARY_VISIBILITY ClangImporter::Implementation
   ///
   /// \param type The Clang type to import.
   ///
-  /// \param kind The kind of type import we're performing.
+  /// \param kind A classification of the immediate context in which this type
+  ///   will be used. Different contexts result in the type being imported
+  ///   differently; for example, CF types are normally considered Unmanaged,
+  ///   but in parameter position they are known to always be passed at +0.
+  ///   See also the \p topLevelBridgeability parameter.
   ///
   /// \param allowNSUIntegerAsInt If true, NSUInteger will be imported as Int
-  ///        in certain contexts. If false, it will always be imported as UInt.
-  ///
-  /// \param bridgeability Whether we can bridge types in this context.
-  ///        This may restrict the ability to bridge types even in contexts
-  ///        that otherwise allow bridging, such as function results and
-  ///        parameters.
+  ///   in certain contexts. If false, it will always be imported as UInt.
+  ///
+  /// \param topLevelBridgeability A classification of the top-level context in
+  ///   which this type will be used. This and \p kind are used together to
+  ///   determine whether a type can be imported in a more Swifty way than
+  ///   a naive translation of its C type. Full bridgeability requires that SIL
+  ///   can get back to the original Clang type if it needs to, which implies
+  ///   that this type is part of a top-level declaration where we do bridging.
+  ///   Without full bridgeability, we can still do some Swifty importing (e.g.
+  ///   mapping NSString to String) if we're in an immediate context \p kind
+  ///   that allows bridging, but only in cases where Swift's default mapping
+  ///   "back" to C is the correct one. If the original type has something
+  ///   funny going on, we either have to use a less lossy version of the type
+  ///   (ObjCBool rather than Bool) or refuse to import it at all (a block with
+  ///   the \c ns_returns_retained attribute).
+  ///
+  /// \param optional If the imported type was a pointer-like type in C, this
+  ///   optionality is applied to the resulting Swift type.
+  ///
+  /// \param resugarNSErrorPointer If true, Objective-C's `NSError **` is
+  ///   imported as Foundation.NSErrorPointer rather than
+  ///   AutoreleasingUnsafeMutablePointer<...>. This is usually desirable
+  ///   behavior, but isn't necessary when we use Swift's \c throws anyway.
+  ///   Strictly speaking, though, this is a hack used to break cyclic
+  ///   dependencies.
   ///
   /// \returns The imported type, or null if this type could
-  /// not be represented in Swift.
+  ///   not be represented in Swift.
   Type importType(clang::QualType type,
                   ImportTypeKind kind,
                   bool allowNSUIntegerAsInt,
-                  Bridgeability bridgeability,
+                  Bridgeability topLevelBridgeability,
                   OptionalTypeKind optional = OTK_ImplicitlyUnwrappedOptional,
                   bool resugarNSErrorPointer = true);
 

test/ClangImporter/Inputs/custom-modules/ObjCParseExtras.h

@@ -209,3 +209,6 @@ typedef NSArray<NSString *> *NSStringArray;
 @interface BridgedTypedefs : NSObject
 @property (readonly,nonnull) NSArray<NSStringArray> *arrayOfArrayOfStrings;
 @end
+
+typedef NSString * _Nonnull (*FPTypedef)(NSString * _Nonnull);
+extern FPTypedef _Nonnull getFP(void);

test/ClangImporter/objc_id_as_any.swift

@@ -25,7 +25,7 @@ idLover.takesArray(ofId: &y) // expected-error{{argument type 'Any' does not con
 idLover.takesId(x)
 idLover.takesId(y)
 
-install_global_event_handler(idLover) // expected-error {{cannot convert value of type 'NSIdLover' to expected argument type 'event_handler?' (aka 'Optional<@convention(c) (AnyObject) -> ()>')}}
+install_global_event_handler(idLover) // expected-error {{cannot convert value of type 'NSIdLover' to expected argument type 'event_handler?' (aka 'Optional<@convention(c) (Any) -> ()>')}}
 
 // FIXME: this should not type-check!
 // Function conversions are not legal when converting to a thin function type.

test/ClangImporter/objc_parse.swift

@@ -643,3 +643,9 @@ func testTypeAndValue() {
 func testBridgedTypedef(bt: BridgedTypedefs) {
   let _: Int = bt.arrayOfArrayOfStrings // expected-error{{'[[String]]'}}
 }
+
+func testBridgeFunctionPointerTypedefs(fptrTypedef: FPTypedef) {
+  // See also print_clang_bool_bridging.swift.
+  let _: Int = fptrTypedef // expected-error{{'@convention(c) (String) -> String'}}
+  let _: Int = getFP() // expected-error{{'@convention(c) (String) -> String'}}
+}

test/IDE/print_clang_bool_bridging.swift

@@ -42,13 +42,13 @@ func testCBoolFnToBlockTypedef(_: @escaping CBoolFn) -> CBoolBlock
 func testObjCBoolFnToBlockTypedef(_: @escaping ObjCBoolFn) -> ObjCBoolBlock
 func testDarwinBooleanFnToBlockTypedef(_: @escaping DarwinBooleanFn) -> DarwinBooleanBlock
 
-typealias CBoolFnToBlockType = (CBoolFn) -> CBoolBlock
-typealias ObjCBoolFnToBlockType = (ObjCBoolFn) -> ObjCBoolBlock
-typealias DarwinBooleanFnToBlockType = (DarwinBooleanFn) -> DarwinBooleanBlock
+typealias CBoolFnToBlockType = (CBoolFn) -> (Bool) -> Bool
+typealias ObjCBoolFnToBlockType = (ObjCBoolFn) -> (ObjCBool) -> ObjCBool
+typealias DarwinBooleanFnToBlockType = (DarwinBooleanFn) -> (DarwinBoolean) -> DarwinBoolean
 
-var globalObjCBoolFnToBlockFP: @convention(c) (ObjCBoolFn) -> @convention(block) (ObjCBool) -> ObjCBool
-var globalObjCBoolFnToBlockFPP: UnsafeMutablePointer<@convention(c) (ObjCBoolFn) -> @convention(block) (ObjCBool) -> ObjCBool>?
-var globalObjCBoolFnToBlockBP: @convention(block) (ObjCBoolFn) -> @convention(block) (ObjCBool) -> ObjCBool
+var globalObjCBoolFnToBlockFP: @convention(c) (ObjCBoolFn) -> (ObjCBool) -> ObjCBool
+var globalObjCBoolFnToBlockFPP: UnsafeMutablePointer<@convention(c) (ObjCBoolFn) -> (ObjCBool) -> ObjCBool>?
+var globalObjCBoolFnToBlockBP: @convention(block) (ObjCBoolFn) -> (ObjCBool) -> ObjCBool
 
 var globalCBoolFn: CBoolFn
 var globalObjCBoolFn: ObjCBoolFn

test/IDE/print_omit_needless_words.swift

@@ -143,7 +143,7 @@
 // CHECK-FOUNDATION: func doSomethingElse(with: NSCopying & NSObjectProtocol)
 
 // Note: Function type -> "Function".
-// CHECK-FOUNDATION: func sort(_: @escaping @convention(c) (AnyObject, AnyObject) -> Int)
+// CHECK-FOUNDATION: func sort(_: @escaping @convention(c) (Any, Any) -> Int)
 
 // Note: Plural: NSArray without type arguments -> "Objects".
 // CHECK-FOUNDATION: func remove(_: [Any])

test/Interpreter/SDK/Inputs/BlockGlobals/BlockGlobals.h

@@ -0,0 +1,7 @@
+#import <Foundation/Foundation.h>
+
+extern NSString * _Nonnull (^ _Nonnull mutableBlockGlobal)(NSString * _Nonnull);
+extern NSString * _Nonnull (^ _Nonnull const constBlockGlobal)(NSString * _Nonnull);
+
+extern NSString * _Nonnull (* _Nonnull mutableFPGlobal)(NSString * _Nonnull);
+extern NSString * _Nonnull (* _Nonnull const constFPGlobal)(NSString * _Nonnull);

test/Interpreter/SDK/Inputs/BlockGlobals/BlockGlobals.m

@@ -0,0 +1,20 @@
+#import "BlockGlobals.h"
+
+NSString *(^mutableBlockGlobal)(NSString *) = ^ NSString *(NSString *arg) {
+  return [@"default mutable block: " stringByAppendingString: arg];
+};
+NSString *(^ const constBlockGlobal)(NSString *) = ^ NSString *(NSString *arg) {
+  return [@"default const block: " stringByAppendingString: arg];
+};
+
+static NSString *appendToDefault(NSString *arg) {
+  return [@"default mutable FP: " stringByAppendingString: arg];
+}
+
+NSString *(*mutableFPGlobal)(NSString *) = &appendToDefault;
+
+
+static NSString *appendToDefaultConst(NSString *arg) {
+  return [@"default const FP: " stringByAppendingString: arg];
+}
+NSString *(* const constFPGlobal)(NSString *) = &appendToDefaultConst;

test/Interpreter/SDK/block_globals.swift

@@ -0,0 +1,29 @@
+// RUN: %empty-directory(%t)
+
+// RUN: %target-clang -fobjc-arc %S/Inputs/BlockGlobals/BlockGlobals.m -c -o %t/BlockGlobals.o
+// RUN: %target-build-swift %s -import-objc-header %S/Inputs/BlockGlobals/BlockGlobals.h %t/BlockGlobals.o -o %t/main
+// RUN: %target-run %t/main
+
+// REQUIRES: executable_test
+// REQUIRES: objc_interop
+
+import Foundation
+import StdlibUnittest
+
+var BlockGlobalsTestSuite = TestSuite("BlockGlobals")
+
+BlockGlobalsTestSuite.test("const block") {
+  expectEqual((@convention(block) (String) -> String).self as Any.Type, type(of: constBlockGlobal))
+  expectEqual("default const block: abc", constBlockGlobal("abc"))
+}
+
+BlockGlobalsTestSuite.test("const function pointer") {
+  expectEqual((@convention(c) (String) -> String).self as Any.Type, type(of: constFPGlobal))
+  expectEqual("default const FP: abc", constFPGlobal("abc"))
+}
+
+// FIXME: Add tests for mutable globals, including mutating them, once the
+// compiler supports it, as well as loading from the const globals without
+// immediately calling them.
+
+runAllTests()