Match @objcImpl members’ foreign error conventions

ClangImporter can import some methods as throwing that `@objc` cannot generate. For instance, an imported Objective-C method with an error out parameter in an unconventional position can still be imported as throwing no matter its selector, but `@objc` can only generate an error out parameter in an unconventional position if the matching selector part consists of the word `error` or (for the first part) ends with `Error`. Detect and diagnose these situations.

Note that the tests do not cover all of the new diagnostics because some of these conditions (like the `Void` parameter) cause selector mismatches and others (like the owned error parameter) are representable in the compiler but cannot currently be imported. I have chosen to add these diagnostics anyway in case there is a corner case that I haven’t discovered.

Fixes rdar://110100071.

Author: beccadax

include/swift/AST/DiagnosticsSema.def

@@ -1657,6 +1657,49 @@ ERROR(objc_implementation_required_attr_mismatch,none,
       "the header",
       (DescriptiveDeclKind, ValueDecl *, bool))
 
+ERROR(objc_implementation_candidate_has_error_convention,none,
+      "%0 %1 does not match the declaration in the header because it throws an "
+      "error",
+      (DescriptiveDeclKind, ValueDecl *))
+
+ERROR(objc_implementation_candidate_lacks_error_convention,none,
+      "%0 %1 does not match the declaration in the header because it does not "
+      "throw an error",
+      (DescriptiveDeclKind, ValueDecl *))
+
+#define SELECT_FOREIGN_ERROR_CONVENTION_KIND \
+      "select{returning zero|returning non-zero|returning zero or a result|" \
+      "returning nil|setting the error parameter|%error}"
+
+ERROR(objc_implementation_mismatched_error_convention_kind,none,
+      "%0 %1 does not match the declaration in the header because it indicates "
+      "an error by %" SELECT_FOREIGN_ERROR_CONVENTION_KIND "2, rather than by "
+      "%" SELECT_FOREIGN_ERROR_CONVENTION_KIND "3",
+      (DescriptiveDeclKind, ValueDecl *, unsigned, unsigned))
+
+ERROR(objc_implementation_mismatched_error_convention_index,none,
+      "%0 %1 does not match the declaration in the header because it uses "
+      "parameter #%2 for the error, not parameter #%3; a selector part called "
+      "'error:' can control which parameter to use",
+      (DescriptiveDeclKind, ValueDecl *, unsigned, unsigned))
+
+ERROR(objc_implementation_mismatched_error_convention_void_param,none,
+      "%0 %1 does not match the declaration in the header because it "
+      "%select{removes the error parameter|makes the error parameter 'Void'}2 "
+      "rather than %select{making it 'Void'|removing it}2",
+      (DescriptiveDeclKind, ValueDecl *, bool))
+
+ERROR(objc_implementation_mismatched_error_convention_ownership,none,
+      "%0 %1 does not match the declaration in the header because it "
+      "%select{does not own|owns}2 the error parameter",
+      (DescriptiveDeclKind, ValueDecl *, bool))
+
+ERROR(objc_implementation_mismatched_error_convention_other,none,
+      "%0 %1 does not match the declaration in the header because it uses a "
+      "different Objective-C error convention; please file a bug report with a "
+      "sample project, as the compiler should be able to describe the mismatch",
+      (DescriptiveDeclKind, ValueDecl *))
+
 ERROR(objc_implementation_wrong_objc_name,none,
       "selector %0 for %1 %2 not found in header; did you mean %3?",
       (ObjCSelector, DescriptiveDeclKind, ValueDecl *, ObjCSelector))

lib/Sema/TypeCheckDeclObjC.cpp

@@ -3043,6 +3043,7 @@ class ObjCImplementationChecker {
     WrongType,
     WrongWritability,
     WrongRequiredAttr,
+    WrongForeignErrorConvention,
 
     Match,
     MatchWithExplicitObjCName,
@@ -3344,6 +3345,11 @@ class ObjCImplementationChecker {
       if (reqCtor->isRequired() != cast<ConstructorDecl>(cand)->isRequired())
         return MatchOutcome::WrongRequiredAttr;
 
+    if (auto reqAFD = dyn_cast<AbstractFunctionDecl>(req))
+      if (reqAFD->getForeignErrorConvention() !=
+              cast<AbstractFunctionDecl>(cand)->getForeignErrorConvention())
+        return MatchOutcome::WrongForeignErrorConvention;
+
     // If we got here, everything matched. But at what quality?
     if (explicitObjCName)
       return MatchOutcome::MatchWithExplicitObjCName;
@@ -3447,6 +3453,49 @@ class ObjCImplementationChecker {
                              ->getLocation());
       return;
     }
+
+    case MatchOutcome::WrongForeignErrorConvention: {
+      auto reqConv = cast<AbstractFunctionDecl>(req)->getForeignErrorConvention();
+      auto candConv = cast<AbstractFunctionDecl>(cand)->getForeignErrorConvention();
+
+      if (reqConv && !candConv)
+        diagnose(cand,
+                 diag::objc_implementation_candidate_has_error_convention,
+                 cand->getDescriptiveKind(), cand);
+      else if (!reqConv && candConv)
+        diagnose(cand,
+                 diag::objc_implementation_candidate_lacks_error_convention,
+                 cand->getDescriptiveKind(), cand);
+      else if (reqConv->getKind() != candConv->getKind())
+        diagnose(cand,
+                 diag::objc_implementation_mismatched_error_convention_kind,
+                 cand->getDescriptiveKind(), cand, candConv->getKind(),
+                 reqConv->getKind());
+      else if (reqConv->getErrorParameterIndex()
+                  != candConv->getErrorParameterIndex())
+        diagnose(cand,
+                 diag::objc_implementation_mismatched_error_convention_index,
+                 cand->getDescriptiveKind(), cand,
+                 candConv->getErrorParameterIndex() + 1,
+                 reqConv->getErrorParameterIndex() + 1);
+      else if (reqConv->isErrorParameterReplacedWithVoid()
+                  != candConv->isErrorParameterReplacedWithVoid())
+        diagnose(cand,
+                 diag::objc_implementation_mismatched_error_convention_void_param,
+                 cand->getDescriptiveKind(), cand,
+                 candConv->isErrorParameterReplacedWithVoid());
+      else if (reqConv->isErrorOwned() != candConv->isErrorOwned())
+        diagnose(cand,
+                 diag::objc_implementation_mismatched_error_convention_ownership,
+                 cand->getDescriptiveKind(), cand, candConv->isErrorOwned());
+      else
+        // Catch-all; probably shouldn't happen.
+        diagnose(cand,
+                 diag::objc_implementation_mismatched_error_convention_other,
+                 cand->getDescriptiveKind(), cand);
+
+      return;
+    }
     }
 
     llvm_unreachable("Unknown MatchOutcome");

test/decl/ext/Inputs/objc_implementation.h

@@ -106,6 +106,12 @@
 
 - (void)doSomethingOverloadedWithCompletionHandler:(void (^ _Nonnull)())completionHandler;
 - (void)doSomethingOverloaded __attribute__((__swift_attr__("@_unavailableFromAsync(message: \"Use async doSomethingOverloaded instead.\")")));
+
+- (BOOL)doSomethingThatCanFailWithHandler:(void (^ _Nonnull)())handler error:(NSError **)error;
+- (BOOL)doSomethingElseThatCanFail:(NSError **)error handler:(void (^ _Nonnull)())handler;
+- (BOOL)doSomethingThatCanFailWithWeirdParameterWithHandler:(void (^ _Nonnull)())handler :(NSError **)error;
+- (int)doSomethingThatCanFailWithWeirdReturnCodeWithError:(NSError **)error __attribute__((swift_error(nonzero_result)));
+
 @end
 
 @protocol PartiallyOptionalProtocol

test/decl/ext/objc_implementation.swift

@@ -335,6 +335,26 @@ protocol EmptySwiftProto {}
   @available(*, noasync)
   @objc(doSomethingOverloaded)
   public func doSomethingOverloaded() {}
+
+  @objc(doSomethingThatCanFailWithHandler:error:)
+  public func doSomethingThatCanFail(handler: @escaping () -> Void) throws {
+    // OK
+  }
+
+  @objc(doSomethingElseThatCanFail:handler:)
+  public func doSomethingElseThatCanFail(handler: @escaping () -> Void) throws {
+    // OK
+  }
+
+  @objc(doSomethingThatCanFailWithWeirdParameterWithHandler::)
+  public func doSomethingThatCanFailWithWeirdParameter(handler: @escaping () -> Void) throws {
+    // expected-warning@-1 {{instance method 'doSomethingThatCanFailWithWeirdParameter(handler:)' does not match the declaration in the header because it uses parameter #1 for the error, not parameter #2; a selector part called 'error:' can control which parameter to use}}
+  }
+
+  @objc(doSomethingThatCanFailWithWeirdReturnCodeWithError:)
+  public func doSomethingThatCanFailWithWeirdReturnCode() throws {
+    // expected-warning@-1 {{instance method 'doSomethingThatCanFailWithWeirdReturnCode()' does not match the declaration in the header because it indicates an error by returning zero, rather than by returning non-zero}}
+  }
 }
 
 @_objcImplementation(Conformance) extension ObjCClass {