[clang] Add support for attribute 'swift_async_error'

[erik.pilkington]

Summary

This attribute specifies how an error is represented for a swift async method. rdar://71941280

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

5867

You can elide this return.

5977

Should there be a diagnostic to combine swift_error and swift_async_error on the same function? Similarly, should there be a diagnostic when adding swift_async_error to a function that isn't (eventually) marked swift_async?

[erik.pilkington]

Get rid of a useless return.

clang/lib/Sema/SemaDeclAttr.cpp

5977

My understanding is that swift_error and swift_async_error don't conflict with each other. swift_error describes how the non-async swift function handles errors, whereas swift_async_error describes how the swift async function handles errors. Since both swift functions are generated, I think it can make sense to have both attributes on one ObjC function.

I think it can make sense to have swift_async_error without swift_async, since the swift importer infers some functions are async without the swift_async attribute using heuristics based on parameter names. I guess we could mimic those heuristics here to diagnose when swift_async_error doesn't make sense alone, but ISTM that it makes more sense to do that check in the swift importer.

[aaron.ballman]

LGTM modulo some testing requests. Thanks!

clang/lib/Sema/SemaDeclAttr.cpp

5977

Thanks for the information!

I think it can make sense to have both attributes on one ObjC function.

Fine by me then! Can you add a test case with a comment showing that this is explicitly expected to be okay?

I think it can make sense to have swift_async_error without swift_async

Also fine by me then! I don't think we need to go to great effort here to diagnose that (the swift importer can gain extra logic if that's useful). I think this is also worth a test case with a comment.

(In both cases, I'm thinking mostly about code archeology projects in the future to answer "did they think of" kind of questions.)

clang/include/clang/Basic/Attr.td

def SwiftAsync : InheritableAttr {
   let Subjects = SubjectList<[Function, ObjCMethod]>;
   let Args = [EnumArgument<"Kind", "Kind",
                 ["none", "swift_private", "not_swift_private"],
                 ["None", "SwiftPrivate", "NotSwiftPrivate"]>,
               ParamIdxArgument<"CompletionHandlerIndex", /*opt=*/1>];
   let Documentation = [SwiftAsyncDocs];
 }
 
+def SwiftAsyncError : InheritableAttr {
+  let Spellings = [Clang<"swift_async_error">];
+  let Subjects = SubjectList<[Function, ObjCMethod]>;
+  let Args = [EnumArgument<"Convention", "ConventionKind",
+              ["none", "nonnull_error", "zero_argument", "nonzero_argument"],
+              ["None", "NonNullError", "ZeroArgument", "NonZeroArgument"]>,
+              UnsignedArgument<"HandlerParamIdx", /*opt=*/1>];
+  let Documentation = [SwiftAsyncErrorDocs];
+}
+
 def Suppress : StmtAttr {
   let Spellings = [CXX11<"gsl", "suppress">];
   let Args = [VariadicStringArgument<"DiagnosticIdentifiers">];
   let Documentation = [SuppressDocs];
 }
 
 def SysVABI : DeclOrTypeAttr {
   let Spellings = [GCC<"sysv_abi">];

clang/include/clang/Basic/AttrDocs.td

 
 When using ``swift_async`` to enable importing, the first argument to the
 attribute is either ``swift_private`` or ``not_swift_private`` to indicate
 whether the function/method is private to the current framework, and the second
 argument is the index of the completion handler parameter.
   }];
 }
 
+def SwiftAsyncErrorDocs : Documentation {
+  let Category = SwiftDocs;
+  let Heading = "swift_async_error";
+  let Content = [{
+The ``swift_async_error`` attribute specifies how an error state will be
+represented in a swift async method. It's a bit analogous to the ``swift_error``
+attribute for the generated async method. The ``swift_async_error`` attribute
+can indicate a variety of different ways of representing an error.
+
+- ``__attribute__((swift_async_error(zero_argument, N)))``, specifies that the
+  async method is considered to have failed if the Nth argument to the
+  completion handler is zero.
+
+- ``__attribute__((swift_async_error(nonzero_argument, N)))``, specifies that
+  the async method is considered to have failed if the Nth argument to the
+  completion handler is non-zero.
+
+- ``__attribute__((swift_async_error(nonnull_error)))``, specifies that the
+  async method is considered to have failed if the ``NSError *`` argument to the
+  completion handler is non-null.
+
+- ``__attribute__((swift_async_error(none)))``, specifies that the async method
+  cannot fail.
+
+
+For instance:
+
+.. code-block:: objc
+
+  @interface MyClass : NSObject
+  -(void)asyncMethod:(void (^)(char, int, float))handler
+      __attribute__((swift_async(swift_private, 1)))
+      __attribute__((swift_async_error(zero_argument, 2)));
+  @end
+
+Here, the ``swift_async`` attribute specifies that ``handler`` is the completion
+handler for this method, and the ``swift_async_error`` attribute specifies that
+the ``int`` parameter is the one that represents the error.
+}];
+}
+
 def SuppressDocs : Documentation {
   let Category = DocCatStmt;
   let Content = [{
 The ``[[gsl::suppress]]`` attribute suppresses specific
 clang-tidy diagnostics for rules of the `C++ Core Guidelines`_ in a portable
 way. The attribute can be attached to declarations, statements, and at
 namespace scope.
 

clang/include/clang/Basic/DiagnosticSemaKinds.td

 
 def err_swift_async_no_access : Error<
   "first argument to 'swift_async' must be either 'none', 'swift_private', or "
   "'not_swift_private'">;
 def err_swift_async_bad_block_type : Error<
   "'swift_async' completion handler parameter must have block type returning"
   " 'void', type here is %0">;
 
+def err_swift_async_error_without_swift_async : Error<
+  "%0 attribute must be applied to a %select{function|method}1 annotated "
+  "with non-'none' attribute 'swift_async'">;
+def err_swift_async_error_no_error_parameter : Error<
+  "%0 attribute with 'nonnull_error' convention can only be applied to a "
+  "%select{function|method}1 with a completion handler with an error "
+  "parameter">;
+def err_swift_async_error_non_integral : Error<
+  "%0 attribute with '%1' convention must have an integral-typed parameter "
+  "in completion handler at index %2, type here is %3">;
+
 def warn_ignored_objc_externally_retained : Warning<
   "'objc_externally_retained' can only be applied to local variables "
   "%select{of retainable type|with strong ownership}0">,
   InGroup<IgnoredAttributes>;
 
 // Function Parameter Semantic Analysis.
 def err_param_with_void_type : Error<"argument may not have 'void' type">;
 def err_void_only_param : Error<

clang/lib/Sema/SemaDeclAttr.cpp

case SwiftErrorAttr::ZeroResult:
     if (!hasErrorParameter(S, D, AL) || !hasIntegerResult(S, D, AL))
       return;
     break;
   }
 
   D->addAttr(::new (S.Context) SwiftErrorAttr(S.Context, AL, Convention));
 }
 
+static void checkSwiftAsyncErrorBlock(Sema &S, Decl *D,
+                                      const SwiftAsyncErrorAttr *ErrorAttr,
+                                      const SwiftAsyncAttr *AsyncAttr) {
+  if (AsyncAttr->getKind() == SwiftAsyncAttr::None) {
+    if (ErrorAttr->getConvention() != SwiftAsyncErrorAttr::None) {
+      S.Diag(AsyncAttr->getLocation(),
+             diag::err_swift_async_error_without_swift_async)
+          << AsyncAttr << isa<ObjCMethodDecl>(D);
+    }
+    return;
+  }
+
+  const ParmVarDecl *HandlerParam = getFunctionOrMethodParam(
+      D, AsyncAttr->getCompletionHandlerIndex().getASTIndex());
+  // handleSwiftAsyncAttr already verified the type is correct, so no need to
+  // double-check it here.
+  const auto *FuncTy = HandlerParam->getType()
+                           ->getAs<BlockPointerType>()
+                           ->getPointeeType()
+                           ->getAs<FunctionProtoType>();
+  ArrayRef<QualType> BlockParams;
+  if (FuncTy)
+    BlockParams = FuncTy->getParamTypes();
+
+  switch (ErrorAttr->getConvention()) {
+  case SwiftAsyncErrorAttr::ZeroArgument:
+  case SwiftAsyncErrorAttr::NonZeroArgument: {
+    uint32_t ParamIdx = ErrorAttr->getHandlerParamIdx();
+    if (ParamIdx == 0 || ParamIdx > BlockParams.size()) {
+      S.Diag(ErrorAttr->getLocation(),
+             diag::err_attribute_argument_out_of_bounds) << ErrorAttr << 2;
+      return;
+    }
+    QualType ErrorParam = BlockParams[ParamIdx - 1];
+    if (!ErrorParam->isIntegralType(S.Context)) {
+      StringRef ConvStr =
+          ErrorAttr->getConvention() == SwiftAsyncErrorAttr::ZeroArgument
+              ? "zero_argument"
+              : "nonzero_argument";
+      S.Diag(ErrorAttr->getLocation(), diag::err_swift_async_error_non_integral)
+          << ErrorAttr << ConvStr << ParamIdx << ErrorParam;
+      return;
+    }
+    break;
+  }
+  case SwiftAsyncErrorAttr::NonNullError: {
+    bool AnyErrorParams = false;
+    for (QualType Param : BlockParams) {
+      // Check for NSError *.
+      if (const auto *ObjCPtrTy = Param->getAs<ObjCObjectPointerType>()) {
+        if (const auto *ID = ObjCPtrTy->getInterfaceDecl()) {
+          if (ID->getIdentifier() == S.getNSErrorIdent()) {
+            AnyErrorParams = true;
+            break;
+          }
+        }
+      }
+      // Check for CFError *.
+      if (const auto *PtrTy = Param->getAs<PointerType>()) {
+        if (const auto *RT = PtrTy->getPointeeType()->getAs<RecordType>()) {
+          if (S.isCFError(RT->getDecl())) {
+            AnyErrorParams = true;
+            break;
+          }
+        }
+      }
+    }
+
+    if (!AnyErrorParams) {
+      S.Diag(ErrorAttr->getLocation(),
+             diag::err_swift_async_error_no_error_parameter)
+          << ErrorAttr << isa<ObjCMethodDecl>(D);
+      return;
+    }
+    break;
+  }
+  case SwiftAsyncErrorAttr::None:
+    break;
+  }
+}
+
+static void handleSwiftAsyncError(Sema &S, Decl *D, const ParsedAttr &AL) {
+  IdentifierLoc *IDLoc = AL.getArgAsIdent(0);
+  SwiftAsyncErrorAttr::ConventionKind ConvKind;
+  if (!SwiftAsyncErrorAttr::ConvertStrToConventionKind(IDLoc->Ident->getName(),
+                                                       ConvKind)) {
+    S.Diag(AL.getLoc(), diag::warn_attribute_type_not_supported)
+        << AL << IDLoc->Ident;
+    return;
+  }
+
+  uint32_t ParamIdx = 0;
+  switch (ConvKind) {
+  case SwiftAsyncErrorAttr::ZeroArgument:
+  case SwiftAsyncErrorAttr::NonZeroArgument: {
+    if (!checkAttributeNumArgs(S, AL, 2))
+      return;
+
+    Expr *IdxExpr = AL.getArgAsExpr(1);
+    if (!checkUInt32Argument(S, AL, IdxExpr, ParamIdx))
+      return;
+    break;
+  }
+  case SwiftAsyncErrorAttr::NonNullError:
+  case SwiftAsyncErrorAttr::None: {
+    if (!checkAttributeNumArgs(S, AL, 1))
+      return;
+    break;
+  }
+  }
+
+  auto *ErrorAttr =
+      ::new (S.Context) SwiftAsyncErrorAttr(S.Context, AL, ConvKind, ParamIdx);
+  D->addAttr(ErrorAttr);
+
+  if (auto *AsyncAttr = D->getAttr<SwiftAsyncAttr>())
+    checkSwiftAsyncErrorBlock(S, D, ErrorAttr, AsyncAttr);
+}
+
 // For a function, this will validate a compound Swift name, e.g.
 // <code>init(foo:bar:baz:)</code> or <code>controllerForName(_:)</code>, and
 // the function will output the number of parameter names, and whether this is a
 // single-arg initializer.
 //
 // For a type, enum constant, property, or variable declaration, this will
 // validate either a simple identifier, or a qualified
 // <code>context.identifier</code> name.
if (Kind == SwiftAsyncAttr::None) {
     if (!BlockTy->getAs<FunctionType>()->getReturnType()->isVoidType()) {
       S.Diag(CompletionBlock->getLocation(),
              diag::err_swift_async_bad_block_type)
           << CompletionBlock->getType();
       return;
     }
   }
 
-  D->addAttr(::new (S.Context) SwiftAsyncAttr(S.Context, AL, Kind, Idx));
+  auto *AsyncAttr =
+      ::new (S.Context) SwiftAsyncAttr(S.Context, AL, Kind, Idx);
+  D->addAttr(AsyncAttr);
+
+  if (auto *ErrorAttr = D->getAttr<SwiftAsyncErrorAttr>())
+    checkSwiftAsyncErrorBlock(S, D, ErrorAttr, AsyncAttr);
 }
 
 //===----------------------------------------------------------------------===//
 // Microsoft specific attribute handlers.
 //===----------------------------------------------------------------------===//
 
 UuidAttr *Sema::mergeUuidAttr(Decl *D, const AttributeCommonInfo &CI,
                               StringRef UuidAsWritten, MSGuidDecl *GuidDecl) {
case ParsedAttr::AT_SwiftObjCMembers:
     handleSimpleAttribute<SwiftObjCMembersAttr>(S, D, AL);
     break;
   case ParsedAttr::AT_SwiftPrivate:
     handleSimpleAttribute<SwiftPrivateAttr>(S, D, AL);
     break;
   case ParsedAttr::AT_SwiftAsync:
     handleSwiftAsyncAttr(S, D, AL);
     break;
+  case ParsedAttr::AT_SwiftAsyncError:
+    handleSwiftAsyncError(S, D, AL);
+    break;
 
   // XRay attributes.
   case ParsedAttr::AT_XRayLogArgs:
     handleXRayLogArgsAttr(S, D, AL);
     break;
 
   case ParsedAttr::AT_PatchableFunctionEntry:
     handlePatchableFunctionEntryAttr(S, D, AL);

clang/test/Misc/pragma-attribute-supported-attributes-list.test

 // CHECK-NEXT: ReturnTypestate (SubjectMatchRule_function, SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: ReturnsNonNull (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: ReturnsTwice (SubjectMatchRule_function)
 // CHECK-NEXT: ScopedLockable (SubjectMatchRule_record)
 // CHECK-NEXT: Section (SubjectMatchRule_function, SubjectMatchRule_variable_is_global, SubjectMatchRule_objc_method, SubjectMatchRule_objc_property)
 // CHECK-NEXT: SetTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: SpeculativeLoadHardening (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: SwiftAsync (SubjectMatchRule_function, SubjectMatchRule_objc_method)
+// CHECK-NEXT: SwiftAsyncError (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: SwiftAsyncName (SubjectMatchRule_objc_method, SubjectMatchRule_function)
 // CHECK-NEXT: SwiftBridgedTypedef (SubjectMatchRule_type_alias)
 // CHECK-NEXT: SwiftContext (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: SwiftError (SubjectMatchRule_function, SubjectMatchRule_objc_method)
 // CHECK-NEXT: SwiftErrorResult (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: SwiftIndirectResult (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: SwiftNewType (SubjectMatchRule_type_alias)
 // CHECK-NEXT: SwiftObjCMembers (SubjectMatchRule_objc_interface)

clang/test/SemaObjC/attr-swift-async-error.m

+// RUN: %clang_cc1 %s -fblocks -fsyntax-only -verify
+
+#define ASYNC(...) __attribute__((swift_async(__VA_ARGS__)))
+#define ASYNC_ERROR(...) __attribute__((swift_async_error(__VA_ARGS__)))
+
+ASYNC(swift_private, 1)
+ASYNC_ERROR(zero_argument, 1)
+void test_good(void (^handler)(int));
+
+ASYNC(swift_private, 2)
+ASYNC_ERROR(nonzero_argument, 2)
+void test_good2(double, void (^handler)(double, int, double));
+
+enum SomeEnum { SE_a, SE_b };
+
+ASYNC(swift_private, 1)
+ASYNC_ERROR(nonzero_argument, 1)
+void test_good3(void (^handler)(enum SomeEnum, double));
+
+ASYNC_ERROR(zero_argument, 1)
+ASYNC(swift_private, 1)
+void test_rev_order(void (^handler)(int));
+
+@class NSError;
+
+ASYNC(swift_private, 1)
+ASYNC_ERROR(nonnull_error)
+void test_nserror(void (^handler)(NSError *));
+
+typedef struct __attribute__((objc_bridge(NSError))) __CFError * CFErrorRef;
+
+ASYNC(swift_private, 1)
+ASYNC_ERROR(nonnull_error)
+void test_cferror(void (^handler)(CFErrorRef));
+
+ASYNC(swift_private, 1)
+ASYNC_ERROR(nonnull_error) // expected-error {{'swift_async_error' attribute with 'nonnull_error' convention can only be applied to a function with a completion handler with an error parameter}}
+void test_interror(void (^handler)(int));
+
+ASYNC(swift_private, 1)
+ASYNC_ERROR(zero_argument, 1) // expected-error {{'swift_async_error' attribute with 'zero_argument' convention must have an integral-typed parameter in completion handler at index 1, type here is 'double'}}
+void test_not_integral(void (^handler)(double));
+
+ASYNC(swift_private, 1)
+ASYNC_ERROR(none)
+void test_none(void (^)());
+
+ASYNC(none)
+ASYNC_ERROR(none)
+void test_double_none(void (^)());
+
+ASYNC(none)
+ASYNC_ERROR(none, 1) // expected-error {{'swift_async_error' attribute takes one argument}}
+void test_double_none_args();
+
+ASYNC(swift_private, 1)
+ASYNC_ERROR(nonnull_error, 1) // expected-error{{'swift_async_error' attribute takes one argument}}
+void test_args(void (^)(void));
+
+ASYNC(swift_private, 1)
+ASYNC_ERROR(zero_argument, 1, 1) // expected-error{{'swift_async_error' attribute takes no more than 2 arguments}}
+void test_args2(void (^)(int));
+
+ASYNC_ERROR(none) int x; // expected-warning{{'swift_async_error' attribute only applies to functions and Objective-C methods}}
+
+@interface ObjC
+-(void)m1:(void (^)(int))handler
+  ASYNC(swift_private, 1)
+  ASYNC_ERROR(zero_argument, 1);
+
+-(void)m2:(int)first withSecond:(void (^)(int))handler
+  ASYNC(swift_private, 2)
+  ASYNC_ERROR(nonzero_argument, 1);
+
+-(void)m3:(void (^)(void))block
+  ASYNC_ERROR(zero_argument, 1) // expected-error {{'swift_async_error' attribute parameter 2 is out of bounds}}
+  ASYNC(swift_private, 1);
+
+-(void)m4:(void (^)(double, int, float))handler
+  ASYNC(swift_private, 1)
+  ASYNC_ERROR(nonzero_argument, 1); // expected-error{{swift_async_error' attribute with 'nonzero_argument' convention must have an integral-typed parameter in completion handler at index 1, type here is 'double'}}
+
+-(void)m5:(void (^)(NSError *))handler
+  ASYNC(swift_private, 1)
+  ASYNC_ERROR(nonnull_error);
+
+-(void)m6:(void (^)(void *))handler
+  ASYNC(swift_private, 1)
+  ASYNC_ERROR(nonnull_error); // expected-error{{'swift_async_error' attribute with 'nonnull_error' convention can only be applied to a method with a completion handler with an error parameter}}
+@end
+
+// 'swift_error' and 'swift_async_error' are OK on one function.
+ASYNC(swift_private, 1)
+ASYNC_ERROR(nonnull_error)
+__attribute__((swift_error(nonnull_error)))
+void swift_error_and_swift_async_error(void (^handler)(NSError *), NSError **);
+
+@interface TestNoSwiftAsync
+// swift_async_error can make sense without swift_async.
+-(void)doAThingWithCompletion:(void (^)(NSError *))completion
+  ASYNC_ERROR(nonnull_error);
+@end