Merge pull request #30763 from jckarter/protocol-infer-objc-error-convention

Sema: Infer error convention from @objc protocol requirements.

Author: jckarter

include/swift/AST/DiagnosticsSema.def

@@ -4342,6 +4342,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}}
+}