Sema: Infer error convention from @objc protocol requirements.

When a Swift declaration witnesses an ObjC protocol requirement, its error convention needs to
match the requirement. Furthermore, if there are different protocol requirements that the
Swift method can witness, with different error conventions, we need to bail out because we
can't simultaneously match all of them. Fixes rdar://problem/59496036 | SR-12201.

Author: jckarter

include/swift/AST/DiagnosticsSema.def

@@ -4326,6 +4326,13 @@ NOTE(objc_ambiguous_inference_candidate,none,
      "%0 (in protocol %1) provides Objective-C name %2",
      (DeclName, DeclName, ObjCSelector))
 
+ERROR(objc_ambiguous_error_convention,none,
+      "%0 overrides or implements protocol requirements for Objective-C "
+      "declarations with incompatible error argument conventions",
+      (DeclName))
+NOTE(objc_ambiguous_error_convention_candidate,none,
+     "%0 provides an error argument here", (DeclName))
+
 ERROR(nonlocal_bridged_to_objc,none,
       "conformance of %0 to %1 can only be written in module %2",
       (Identifier, Identifier, Identifier))

include/swift/AST/ForeignErrorConvention.h

@@ -182,6 +182,16 @@ class ForeignErrorConvention {
            getKind() == NonZeroResult);
     return ResultType;
   }
+  
+  bool operator==(ForeignErrorConvention other) const {
+    return info.TheKind == other.info.TheKind
+      && info.ErrorIsOwned == other.info.ErrorIsOwned
+      && info.ErrorParameterIsReplaced == other.info.ErrorParameterIsReplaced
+      && info.ErrorParameterIndex == other.info.ErrorParameterIndex;
+  }
+  bool operator!=(ForeignErrorConvention other) const {
+    return !(*this == other);
+  }
 };
 
 }

lib/Sema/TypeCheckDeclObjC.cpp

@@ -1562,6 +1562,8 @@ void markAsObjC(ValueDecl *D, ObjCReason reason,
 
   if (auto method = dyn_cast<AbstractFunctionDecl>(D)) {
     // Determine the foreign error convention.
+    Optional<ForeignErrorConvention> inheritedConvention;
+    AbstractFunctionDecl *declProvidingInheritedConvention = nullptr;
     if (auto baseMethod = method->getOverriddenDecl()) {
       // If the overridden method has a foreign error convention,
       // adopt it.  Set the foreign error convention for a throwing
@@ -1570,14 +1572,44 @@ void markAsObjC(ValueDecl *D, ObjCReason reason,
       if (method->hasThrows()) {
         if (auto baseErrorConvention
               = baseMethod->getForeignErrorConvention()) {
-          errorConvention = baseErrorConvention;
+          inheritedConvention = baseErrorConvention;
+          declProvidingInheritedConvention = baseMethod;
         }
+      }
+    }
+    
+    for (auto req : findWitnessedObjCRequirements(method)) {
+      auto reqMethod = dyn_cast<AbstractFunctionDecl>(req);
+      if (!reqMethod) continue;
+      
+      // If the method witnesses an ObjC requirement that throws, adopt its
+      // error convention.
+      if (reqMethod->hasThrows()) {
+        if (auto reqErrorConvention = reqMethod->getForeignErrorConvention()) {
+          // Check for a conflict among protocol conformances or inherited
+          // methods.
+          if (declProvidingInheritedConvention
+              && inheritedConvention != reqErrorConvention) {
+            method->diagnose(diag::objc_ambiguous_error_convention,
+                             method->getFullName());
+            declProvidingInheritedConvention->diagnose(
+                             diag::objc_ambiguous_error_convention_candidate,
+                             declProvidingInheritedConvention->getFullName());
+            reqMethod->diagnose(diag::objc_ambiguous_error_convention_candidate,
+                                reqMethod->getFullName());
+            break;
+          }
 
-        assert(errorConvention && "Missing error convention");
-        method->setForeignErrorConvention(*errorConvention);
+          inheritedConvention = reqErrorConvention;
+          declProvidingInheritedConvention = reqMethod;
+        }
       }
+    }
+    
+    // Attach the foreign error convention.
+    if (inheritedConvention) {
+      method->setForeignErrorConvention(*inheritedConvention);
     } else if (method->hasThrows()) {
-      // Attach the foreign error convention.
       assert(errorConvention && "Missing error convention");
       method->setForeignErrorConvention(*errorConvention);
     }

test/SILGen/Inputs/objc_error_convention_from_protocol.h

@@ -0,0 +1,16 @@
+@import Foundation;
+
+@protocol Caller1
+@required
+- (BOOL)call:(void (^_Nonnull)(void))callback error:(NSError**)error;
+@end;
+
+@protocol CallerA
+@required
+- (BOOL)use:(NSInteger)x thenCall:(void (^_Nonnull)(void))callback error:(NSError**)error;
+@end;
+
+@protocol CallerB
+@required
+- (BOOL)use:(NSInteger)x error:(NSError**)error thenCall:(void (^_Nonnull)(void))callback;
+@end;

test/SILGen/objc_error_convention_from_protocol.swift

@@ -0,0 +1,26 @@
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -emit-silgen %s -import-objc-header %S/Inputs/objc_error_convention_from_protocol.h | %FileCheck %s
+// REQUIRES: objc_interop
+
+import Foundation
+
+class Caller1Impl: Caller1 {
+  // declared as - (BOOL)call:(void (^_Nonnull)(void))callback error:(NSError**)error;
+  // CHECK-LABEL: sil {{.*}} @$s{{.*}}11Caller1Impl{{.*}}call{{.*}}To :
+  // CHECK-SAME: $@convention(objc_method) (@convention(block) () -> (), Optional<AutoreleasingUnsafeMutablePointer<Optional<NSError>>>, Caller1Impl) -> ObjCBool
+  func call(_ callback: @escaping () -> ()) throws { }
+}
+
+class CallerAImpl: CallerA {
+  // declared as - (BOOL)use:(NSInteger)x thenCall:(void (^_Nonnull)(void))callback error:(NSError**)error;
+  // CHECK-LABEL: sil {{.*}} @$s{{.*}}11CallerAImpl{{.*}}use{{.*}}thenCall{{.*}}To :
+  // CHECK-SAME: $@convention(objc_method) (Int, @convention(block) () -> (), Optional<AutoreleasingUnsafeMutablePointer<Optional<NSError>>>, CallerAImpl) -> ObjCBool
+  func use(_ x: Int, thenCall callback: @escaping () -> ()) throws { }
+}
+
+class CallerBImpl: CallerB {
+  // declared as - (BOOL)use:(NSInteger)x error:(NSError**)error thenCall:(void (^_Nonnull)(void))callback;
+  // CHECK-LABEL: sil {{.*}} @$s{{.*}}11CallerBImpl{{.*}}use{{.*}}thenCall{{.*}}To :
+  // CHECK-SAME: $@convention(objc_method) (Int, Optional<AutoreleasingUnsafeMutablePointer<Optional<NSError>>>, @convention(block) () -> (), CallerBImpl) -> ObjCBool
+  func use(_ x: Int, thenCall callback: @escaping () -> ()) throws { }
+}
+

test/decl/protocol/Inputs/objc_error_convention_conflict.h

@@ -0,0 +1,15 @@
+@import Foundation;
+
+@protocol CallerX
+@required
+- (BOOL)use:(NSInteger)x error:(void (^_Nonnull)(void))callback error:(NSError*_Nullable*_Nullable)error;
+@end;
+@protocol CallerY
+@required
+- (BOOL)use:(NSInteger)x error:(NSError*_Nullable*_Nullable)error error:(void (^_Nonnull)(void))callback;
+@end;
+
+@interface CallerBaseY
+- (BOOL)use:(NSInteger)x error:(NSError*_Nullable*_Nullable)error error:(void (^_Nonnull)(void))callback;
+@end
+

test/decl/protocol/objc_error_convention_conflict.swift

@@ -0,0 +1,10 @@
+// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) -typecheck -verify %s -import-objc-header %S/Inputs/objc_error_convention_conflict.h
+// REQUIRES: objc_interop
+
+class CallerXYImpl: CallerX, CallerY {
+  func use(_ x: Int, error callback: @escaping () -> ()) throws { } // expected-error {{incompatible error argument conventions}}
+}
+
+class CallerXDerivedY: CallerBaseY, CallerX {
+  override func use(_ x: Int, error callback: @escaping () -> ()) throws { } // expected-error {{incompatible error argument conventions}}
+}