[ClangImporter] Bridging can happen even without full bridgeability

Partially reverts f4f8349 (from July!) which caused us to start
importing global blocks with unbridged parameters, breaking source
compatibility. I'm still investigating whether there's an actual hole
in the logic; see next few commits.

rdar://problem/34913634

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

@@ -1313,7 +1313,7 @@ static Type adjustTypeForConcreteImport(ClangImporter::Implementation &impl,
       }
     }
 
-    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 +1356,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

@@ -907,22 +907,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/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/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()