[ClangImporter] Handle NS_TYPED_ENUMs of NSUIntegers in system headers

jrose-apple · jrose-apple · commit 0ff871a798bf · 2019-04-26T11:30:14.000-07:00
The special rule for NSUInteger in system headers was colliding with
the behavior of NS_TYPED_ENUM (swift_newtype/swift_wrapper). Let
NS_TYPED_ENUM take precedence here.

(I also future-proofed this for BOOL and Boolean, but those don't
NS_TYPED_ENUM correctly right now for another reason: Bool is bridged
to Objective-C via NSNumber, but ObjCBool is not, even though it could
be.)

rdar://problem/50076612
diff --git a/lib/ClangImporter/ImportType.cpp b/lib/ClangImporter/ImportType.cpp
@@ -667,24 +667,41 @@ namespace {
       if (!decl) return Visit(type->desugar());
 
       Type mappedType = getAdjustedTypeDeclReferenceType(decl);
-      ImportHint hint = ImportHint::None;
 
       if (getSwiftNewtypeAttr(type->getDecl(), Impl.CurrentVersion)) {
         if (isCFTypeDecl(type->getDecl())) {
-          hint = ImportHint::SwiftNewtypeFromCFPointer;
-        } else {
+          return {mappedType, ImportHint::SwiftNewtypeFromCFPointer};
+        }
+
+        auto underlying = Visit(type->getDecl()->getUnderlyingType());
+        switch (underlying.Hint) {
+        case ImportHint::None:
+        case ImportHint::Void:
+        case ImportHint::Block:
+        case ImportHint::CFPointer:
+        case ImportHint::ObjCPointer:
+        case ImportHint::CFunctionPointer:
+        case ImportHint::OtherPointer:
+        case ImportHint::SwiftNewtypeFromCFPointer:
+          return {mappedType, underlying.Hint};
+
+        case ImportHint::BOOL:
+        case ImportHint::Boolean:
+        case ImportHint::NSUInteger:
+          // Toss out the special rules for these types; we still want to
+          // import as a wrapper.
+          return {mappedType, ImportHint::None};
+
+        case ImportHint::ObjCBridged:
           // If the underlying type was bridged, the wrapper type is
-          // only useful in bridged cases.
-          auto underlying = Visit(type->getDecl()->getUnderlyingType());
-          if (underlying.Hint == ImportHint::ObjCBridged) {
-            return { underlying.AbstractType,
-                     ImportHint(ImportHint::ObjCBridged, mappedType) };
-          }
-          hint = underlying.Hint;
+          // only useful in bridged cases. Exit early.
+          return { underlying.AbstractType,
+                   ImportHint(ImportHint::ObjCBridged, mappedType) };
         }
+      }
 
       // For certain special typedefs, we don't want to use the imported type.
-      } else if (auto specialKind = Impl.getSpecialTypedefKind(type->getDecl())) {
+      if (auto specialKind = Impl.getSpecialTypedefKind(type->getDecl())) {
         switch (specialKind.getValue()) {
         case MappedTypeNameKind::DoNothing:
         case MappedTypeNameKind::DefineAndUse:
@@ -696,6 +713,7 @@ namespace {
           break;
         }
 
+        ImportHint hint = ImportHint::None;
         if (type->getDecl()->getName() == "BOOL") {
           hint = ImportHint::BOOL;
         } else if (type->getDecl()->getName() == "Boolean") {
@@ -711,64 +729,62 @@ namespace {
           hint = ImportHint::OtherPointer;
         }
         // Any other interesting mapped types should be hinted here.
+        return { mappedType, hint };
+      }
 
       // Otherwise, recurse on the underlying type.  We need to recompute
       // the hint, and if the typedef uses different bridgeability than the
       // context then we may also need to bypass the typedef.
-      } else {
-        auto underlyingType = type->desugar();
+      auto underlyingType = type->desugar();
 
-        // Figure out the bridgeability we would normally use for this typedef.
-        auto typedefBridgeability =
+      // Figure out the bridgeability we would normally use for this typedef.
+      auto typedefBridgeability =
           getTypedefBridgeability(type->getDecl(), underlyingType);
 
-        // Figure out the typedef we should actually use.
-        auto underlyingBridgeability = Bridging;
-        SwiftTypeConverter innerConverter(Impl, AllowNSUIntegerAsInt,
-                                          underlyingBridgeability);
-        auto underlyingResult = innerConverter.Visit(underlyingType);
-
-        // If we used different bridgeability than this typedef normally
-        // 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;
-        }
+      // Figure out the typedef we should actually use.
+      auto underlyingBridgeability = Bridging;
+      SwiftTypeConverter innerConverter(Impl, AllowNSUIntegerAsInt,
+                                        underlyingBridgeability);
+      auto underlyingResult = innerConverter.Visit(underlyingType);
+
+      // If we used different bridgeability than this typedef normally
+      // 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;
+      }
 
 #ifndef NDEBUG
-        switch (underlyingResult.Hint) {
-        case ImportHint::Block:
-        case ImportHint::ObjCBridged:
-          // Bridging is fine for Objective-C and blocks.
+      switch (underlyingResult.Hint) {
+      case ImportHint::Block:
+      case ImportHint::ObjCBridged:
+        // Bridging is fine for Objective-C and blocks.
+        break;
+      case ImportHint::NSUInteger:
+        // NSUInteger might be imported as Int rather than UInt depending
+        // on where the import lives.
+        if (underlyingResult.AbstractType->getAnyNominal() ==
+            Impl.SwiftContext.getIntDecl())
           break;
-        case ImportHint::NSUInteger:
-          // NSUInteger might be imported as Int rather than UInt depending
-          // on where the import lives.
-          if (underlyingResult.AbstractType->getAnyNominal() ==
-              Impl.SwiftContext.getIntDecl())
-            break;
-          LLVM_FALLTHROUGH;
-        default:
-          if (!underlyingResult.AbstractType->isEqual(mappedType)) {
-            underlyingResult.AbstractType->dump();
-            mappedType->dump();
-          }
-          assert(underlyingResult.AbstractType->isEqual(mappedType) &&
-                 "typedef without special typedef kind was mapped "
-                 "differently from its underlying type?");
+        LLVM_FALLTHROUGH;
+      default:
+        if (!underlyingResult.AbstractType->isEqual(mappedType)) {
+          underlyingResult.AbstractType->dump();
+          mappedType->dump();
         }
+        assert(underlyingResult.AbstractType->isEqual(mappedType) &&
+               "typedef without special typedef kind was mapped "
+               "differently from its underlying type?");
+      }
 #endif
-        hint = underlyingResult.Hint;
 
-        // If the imported typealias is unavailable, return the
-        // underlying type.
-        if (decl->getAttrs().isUnavailable(Impl.SwiftContext))
-          mappedType = underlyingResult.AbstractType;
-      }
+      // If the imported typealias is unavailable, return the underlying type.
+      if (decl->getAttrs().isUnavailable(Impl.SwiftContext))
+        return underlyingResult;
 
-      return { mappedType, hint };
+      return { mappedType, underlyingResult.Hint };
     }
 
 #define SUGAR_TYPE(KIND)                                            \
diff --git a/test/ClangImporter/Inputs/custom-modules/Newtype.h b/test/ClangImporter/Inputs/custom-modules/Newtype.h
@@ -107,3 +107,6 @@ typedef NSError *ErrorNewType __attribute((swift_newtype(struct)));
 
 void testErrorDictionary(NSDictionary<NSError *, NSString *> * _Nonnull);
 void testErrorDictionaryNewtype(NSDictionary<ErrorNewType, NSString *> * _Nonnull);
+
+typedef NSUInteger NSUIntegerNewType __attribute((swift_newtype(struct)));
+extern const NSUIntegerNewType NSUIntegerNewTypeConstant;
diff --git a/test/ClangImporter/Inputs/custom-modules/NewtypeSystem.h b/test/ClangImporter/Inputs/custom-modules/NewtypeSystem.h
@@ -0,0 +1,5 @@
+@import Foundation;
+
+typedef NSUInteger NSUIntegerSystemNewType __attribute((swift_newtype(struct)));
+extern const NSUIntegerSystemNewType NSUIntegerSystemNewTypeConstant;
+
diff --git a/test/ClangImporter/Inputs/custom-modules/module.map b/test/ClangImporter/Inputs/custom-modules/module.map
@@ -87,6 +87,10 @@ module Newtype {
   header "Newtype.h"
 }
 
+module NewtypeSystem [system] {
+  header "NewtypeSystem.h"
+}
+
 module ImportAsMember {
   export *
 
@@ -224,4 +228,4 @@ module ConditionallyFoo {
 
 module ForwardDeclarationsHelper {
   header "ForwardDeclarationsHelper.h"
-}
+}
diff --git a/test/ClangImporter/objc_parse.swift b/test/ClangImporter/objc_parse.swift
@@ -6,6 +6,7 @@ import AppKit
 import AVFoundation
 
 import Newtype
+import NewtypeSystem
 import objc_ext
 import TestProtocols
 import TypeAndValue
@@ -683,3 +684,13 @@ func testErrorNewtype() {
   testErrorDictionary(3) // expected-error {{cannot convert value of type 'Int' to expected argument type '[AnyHashable : String]'}}
   testErrorDictionaryNewtype(3) // expected-error {{cannot convert value of type 'Int' to expected argument type '[AnyHashable : String]'}}
 }
+
+func testNSUIntegerNewtype() {
+  let _: NSUIntegerNewType = NSUIntegerNewType(4)
+  let _: UInt = NSUIntegerNewType(4).rawValue
+  let _: NSUIntegerNewType = NSUIntegerNewType.constant
+
+  let _: NSUIntegerSystemNewType = NSUIntegerSystemNewType(4)
+  let _: Int = NSUIntegerSystemNewType(4).rawValue
+  let _: NSUIntegerSystemNewType = NSUIntegerSystemNewType.constant
+}
