[ClangImporter] Don't crash when a bad override affects NSErrors. (swiftlang#7907)

Most of the time the name importer does a good job deciding whether to
import a particular method as throwing or not. However, when a method
is an override, it skips all that work and assumes the decisions made
for the superclass method apply here as well---which makes sense,
since you're going to get the subclass implementation if you call the
superclass's entry point. This can really throw things off if the
types /don't/ match up, though. Handle the one case where this is
legal according to the rules of Objective-C, and make sure we don't
import methods in the other cases.

rdar://problem/30705461

Author: jrose-apple

lib/ClangImporter/ImportType.cpp

@@ -1771,15 +1771,28 @@ adjustResultTypeForThrowingFunction(ForeignErrorConvention::Info errorInfo,
   switch (errorInfo.TheKind) {
   case ForeignErrorConvention::ZeroResult:
   case ForeignErrorConvention::NonZeroResult:
+    // Check for a bad override.
+    if (resultTy->isVoid())
+      return Type();
     return TupleType::getEmpty(resultTy->getASTContext());
 
   case ForeignErrorConvention::NilResult:
-    resultTy = resultTy->getAnyOptionalObjectType();
-    assert(resultTy &&
-           "result type of NilResult convention was not imported as optional");
+    if (Type unwrappedTy = resultTy->getAnyOptionalObjectType())
+      return unwrappedTy;
+    // Check for a bad override.
+    if (resultTy->isVoid())
+      return Type();
+    // It's possible an Objective-C method overrides the base method to never
+    // fail, and marks the method _Nonnull to indicate that. Swift can't
+    // represent that, but it shouldn't fall over either.
     return resultTy;
 
   case ForeignErrorConvention::ZeroPreservedResult:
+    // Check for a bad override.
+    if (resultTy->isVoid())
+      return Type();
+    return resultTy;
+
   case ForeignErrorConvention::NonNilError:
     return resultTy;
   }

test/ClangImporter/foreign_errors.swift

@@ -1,13 +1,16 @@
 // RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -emit-silgen -parse-as-library -verify %s
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -emit-sil -O -parse-as-library -DEMIT_SIL %s
 
 // REQUIRES: objc_interop
 
 import Foundation
 import errors
 
+#if !EMIT_SIL
 func test0() {
   try ErrorProne.fail() // expected-error {{errors thrown from here are not handled}}
 }
+#endif
 
 // Test "AndReturnError" stripping.
 // rdar://20722195
@@ -71,10 +74,12 @@ func testBlockFinal() throws {
   try ErrorProne.runSwiftly(5000, callback: {})
 }
 
+#if !EMIT_SIL
 func testNonBlockFinal() throws {
   ErrorProne.runWithError(count: 0) // expected-error {{missing argument for parameter #1 in call}}
   ErrorProne.run(count: 0) // expected-error {{incorrect argument label in call (have 'count:', expected 'callback:')}}
 }
+#endif
 
 class VeryErrorProne : ErrorProne {
   override class func fail() throws {}
@@ -108,3 +113,24 @@ func testNSErrorExhaustive() {
     }
   }
 }
+
+func testBadOverrides(obj: FoolishErrorSub) throws {
+  try obj.performRiskyOperation()
+  let _: FoolishErrorSub = try obj.produceRiskyOutput()
+  let _: String = try obj.produceRiskyString()
+
+  let _: NSObject = try obj.badNullResult()
+  let _: CInt = try obj.badNullResult2() // This is unfortunate but consistent.
+  let _: CInt = try obj.badZeroResult()
+  try obj.badNonzeroResult() as Void
+
+  let base = obj as SensibleErrorBase
+  try base.performRiskyOperation()
+  let _: NSObject = try base.produceRiskyOutput()
+  let _: String = try base.produceRiskyString()
+
+  let _: NSObject = try base.badNullResult()
+  let _: NSObject = try base.badNullResult2()
+  let _: CInt = try base.badZeroResult()
+  try base.badNonzeroResult() as Void
+}

test/Inputs/clang-importer-sdk/usr/include/errors.h

@@ -59,3 +59,28 @@
 - (BOOL) obliterate: (NSError**) error;
 - (BOOL) invigorate: (NSError**) error callback: (void(^)(void)) block;
 @end
+
+
+@interface SensibleErrorBase: NSObject
+- (BOOL)performRiskyOperationAndReturnError:(NSError **)error;
+- (nullable NSObject *)produceRiskyOutputAndReturnError:(NSError **)error;
+- (nullable NSString *)produceRiskyStringAndReturnError:(NSError **)error;
+
+- (nullable NSObject *)badNullResult:(NSError **)err __attribute__((swift_error(null_result)));
+- (nullable NSObject *)badNullResult2:(NSError **)err __attribute__((swift_error(null_result)));
+- (int)badZeroResult:(NSError **)err __attribute__((swift_error(zero_result)));
+- (int)badNonzeroResult:(NSError **)err __attribute__((swift_error(nonzero_result)));
+@end
+
+@interface FoolishErrorSub : SensibleErrorBase
+// This is invalid, but Swift shouldn't crash when it sees it.
+- (void)performRiskyOperationAndReturnError:(NSError **)error;
+// This should technically be valid in Objective-C as a covariant return.
+- (nonnull FoolishErrorSub *)produceRiskyOutputAndReturnError:(NSError **)error;
+- (nonnull NSString *)produceRiskyStringAndReturnError:(NSError **)error;
+
+- (void)badNullResult:(NSError **)err;
+- (int)badNullResult2:(NSError **)err;
+- (void)badZeroResult:(NSError **)err;
+- (void)badNonzeroResult:(NSError **)err;
+@end