Sema: add support for `__attribute__((__swift_error__))`

[compnerd]

Summary

Introduce a new attribute that is used to indicate the error handling
convention used by a function. This is used to translate the error
semantics from the decorated interface to a compatible Swift interface.

The supported error convention is one of:

• none: no error handling
• nonnull_error: a non-null error parameter indicates an error signifier
• null_result: a return value of NULL is an error signifier
• zero_result: a return value of 0 is an error signifier
• nonzero_result: a non-zero return value is an error signifier

Since this is the first of the attributes needed to support the semantic
annotation for Swift, this change also includes the necessary supporting
infrastructure for a new category of attributes (Swift).

This is based on the work of the original changes in
https://github.com/llvm/llvm-project-staging/commit/8afaf3aad2af43cfedca7a24cd817848c4e95c0c

[rjmccall]

LGTM. :)

[rjmccall]

Oh, there are a ton of typos in this documentation.

clang/include/clang/Basic/AttrDocs.td

3396

Please split this sentence: "...API. When calling the API, Swift will always pass a valid
address initialized to a null pointer."

3404

Typo: "default"

3409

Typo: "integral"

3415

Typo: "integral"

[compnerd]

• correct typos
• apply clang-format changes
• const correct a few declarations.

[rjmccall]

LGTM, thanks!

[aaron.ballman]

clang/include/clang/Basic/Attr.td

2122

Is this attribute also supported by GCC? It looked like it wasn't from my quick check, so I'd recommend switching the spelling to be Clang instead. Note, this will give you [[clang::swift_error(...)]] in both C and C++ as well. If you only want the GNU spelling, use GNU.

2128

Should this apply to function-like things such as blocks or function pointers? If so, you should use HasFunctionProto.

clang/include/clang/Basic/AttrDocs.td

3390

the dynamic -> which dynamic

3394

Canonical type or are typedefs to these types also fine? May want to mention that the type has to be cv-unqualified, but I do wonder whether something like NSError * const * is fine.

clang/lib/Sema/SemaDeclAttr.cpp

5536

const auto *?

5551–5552

No need to repeat this condition three times (and in some cases repeat the check). You can hoist this out of all of the lambdas and call it once.

5560

You should just be able to pass in AL directly (the diagnostics engine handles formatting the name properly), or is there a reason you want to call getNormalizedFullName() explicitly?

5576

Same here as above, but also, you can just pass in Ident.

5590

Same here as above.

5596

convention -> Convention per usual naming rules.

5600

Same here as above.

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

11

You should also have some tests where the attribute accepts no args, too many args, and unknown enum value.

[compnerd]

clang/include/clang/Basic/Attr.td

2122

Ah, oops, this should be GNU.

2128

I believe not.

clang/include/clang/Basic/AttrDocs.td

3394

I am definitely not the authority on this, but I believe that the common thing is to take NSError ** or CFErrorRef ** by canonical name. The cv-qualified versions, except on the outermost pointer, is something that can be treated as valid, though it is certainly extremely unusual. I've added test cases for them as well.

[compnerd]

Address feedback from @aaron.ballman

[rjmccall]

clang/include/clang/Basic/Attr.td

2128

Correct, it's just declared functions for now.

clang/include/clang/Basic/AttrDocs.td

3394

NSError * const * actually does not really work; the whole point is that this is an out-parameter.

[compnerd]

clang/include/clang/Basic/AttrDocs.td

3394

Oh right, its the const NSError ** const that can work, because the outer pointer can be non-mutable as it is a pointer by-reference scenario. Should we diagnose the NSError * const * case then? Any const qualified value is really unidiomatic to say the least.

[rjmccall]

const qualification isn't the most meaningful thing for Objective-C objects anyway.

[gribozavr2]

clang/include/clang/Basic/AttrDocs.td

3377–3382

3399

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

56

98

[compnerd]

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

56

Removed the test case in light of John's comment.

98

Similar

[compnerd]

Address @gribozavr2's comments

[aaron.ballman]

clang/include/clang/Basic/Attr.td

2128

SGTM, thank you for verifying.

clang/include/clang/Basic/AttrDocs.td

3394

I think we should diagnose (as an error) any case that can't work.

I think it may make sense to diagnose (as a warning) any case where we want to ignore the qualifiers, in case we want to give semantics to those qualifiers in this situation later. So this means we'd error on NSError * const * but warn and ignore the qualifiers on volatile NSError **. However, I don't insist on warning in this case unless there's a situation that the user might actually appreciate the warning because it matters (it's not clear to me if such a situation exists). Also, it's not clear to me whether we should or should not warn on a restrict-qualified pointer.

[compnerd]

clang/include/clang/Basic/AttrDocs.td

3394

I think that the newer diagnostics would make sense as a follow up improvement in the case that @rjmccall believe that they would be useful to users. As John mentioned, const is rarely used with ObjectiveC, so the use of that is pretty unidiomatic. In fact, trying to return the error would cause a warning in the first place (due to the assignment of a value to a const parameter).

[compnerd]

Discussed this with @aaron.ballman offline; he is okay with this since this is not going to permit incorrect code through without a diagnostic, even if it is not the most clear and the attribute wont be accidentally ignored. We can add more diagnostics as a follow up if it is useful.

clang/include/clang/Basic/Attr.td

 def Regparm : TypeAttr {
   let Spellings = [GCC<"regparm">];
   let Args = [UnsignedArgument<"NumParams">];
   let Documentation = [RegparmDocs];
   // Represented as part of the enclosing function type.
   let ASTNode = 0;
 }
 
+def SwiftError : InheritableAttr {
+  let Spellings = [GCC<"swift_error">];
+  let Args = [
+      EnumArgument<"Convention", "ConventionKind",
+                   ["none", "nonnull_error", "null_result", "zero_result", "nonzero_result"],
+                   ["None", "NonNullError", "NullResult", "ZeroResult", "NonZeroResult"]>
+  ];
+  let Subjects = SubjectList<[Function, ObjCMethod], ErrorDiag>;
+  let Documentation = [SwiftErrorDocs];
+}
+
 def NoDeref : TypeAttr {
   let Spellings = [Clang<"noderef">];
   let Documentation = [NoDerefDocs];
 }
 
 def ReqdWorkGroupSize : InheritableAttr {
   // Does not have a [[]] spelling because it is an OpenCL-related attribute.
   let Spellings = [GNU<"reqd_work_group_size">];

clang/include/clang/Basic/AttrDocs.td

.. code-block:: objc
     extern NSString *const MyErrorDomain;
     typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
       MyErrFirst,
       MyErrSecond,
     };
   }];
 }
 
+def SwiftDocs : DocumentationCategory<"Controlling Swift Import"> {
+  let Content = [{
+Clang supports additional attributes for controlling how APIs are imported into
+Swift.
+  }];
+}
+
+def SwiftErrorDocs : Documentation {
+  let Category = SwiftDocs;
+  let Heading = "swift_error";
+  let Content = [{
+The ``swift_error`` attribute controls whether a particular function (or
+Objective-C method) is imported into Swift as a throwing function, and if so,
+the dynamic convention it uses.
+
+All of these conventions except ``none`` require the function to have an error
+parameter. Currently, the error parameter is always the last parameter of type
+``NSError**`` or ``CFErrorRef*``.  Swift will remove the error parameter from
+the imported API, and dynamically will always pass a valid address initialized
+to a null pointer.
+
+* ``swift_error(none)`` means that the function should not be imported as
+throwing. The error parameter and result type will be left alone.
+
+* ``swift_error(null_result)`` means that calls to the function should be
+considered to have thrown if they return a null value. The return type must be
+a pointer type, and it will be imported into Swift with a non-optional type.
+This is the dedfault error convention for Objective-C methods that return
+pointers.
+
+* ``swift_error(zero_result)`` means that calls to the function should be
+considered to have thrown if they return a zero result. The return type must be
+an integeral type. If the return type would have been imported as ``Bool``, it
+is instead imported as ``Void``. This is the default error convention for
+Objective-C methods that return a type that would be imported as ``Bool``.
+
+* ``swift_error(nonzero_result)`` means that calls to the function should be
+considered to have thrown if they return a non-zero result. The return type must
+be an integeral type. If the return type would have been imported as ``Bool``,
+it is instead imported as ``Void``.
+
+* ``swift_error(nonnull_error)`` means that calls to the function should be
+considered to have thrown if they leave a non-null error in the error parameter.
+The return type is left unmodified.
+  }];
+}
+
 def OMPDeclareSimdDocs : Documentation {
   let Category = DocCatFunction;
   let Heading = "#pragma omp declare simd";
   let Content = [{
 The ``declare simd`` construct can be applied to a function to enable the creation
 of one or more versions that can process multiple arguments using SIMD
 instructions from a single invocation in a SIMD loop. The ``declare simd``
 directive is a declarative directive. There may be multiple ``declare simd``

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_objc_bridged_related_invalid_class_name : Error<
   "%0 must be name of an Objective-C class to be able to convert %1 to %2">;
 def err_objc_bridged_related_known_method : Error<
  "%0 must be explicitly converted to %1; use %select{%objcclass2|%objcinstance2}3 "
  "method for this conversion">;
 
 def err_objc_attr_protocol_requires_definition : Error<
   "attribute %0 can only be applied to @protocol definitions, not forward declarations">;
 
+def err_attr_swift_error_no_error_parameter : Error<
+  "'%0' attribute can only be applied to a %select{function|method}1 with an "
+  "error parameter">;
+def err_attr_swift_error_return_type : Error<
+  "'%0' attribute with '%1' convention can only be applied to a "
+  "%select{function|method}2 returning %select{an integeral type|a pointer}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 Qualifiers::OCL_Autoreleasing:
     S.Diag(AL.getLoc(), diag::warn_objc_precise_lifetime_meaningless)
         << (Lifetime == Qualifiers::OCL_Autoreleasing);
     break;
   }
 
   D->addAttr(::new (S.Context) ObjCPreciseLifetimeAttr(S.Context, AL));
 }
 
+static bool isErrorParameter(Sema &S, QualType QT) {
+  auto PT = QT->getAs<PointerType>();
+  if (!PT)
+    return false;
+
+  QualType Pointee = PT->getPointeeType();
+
+  // Check for NSError**.
+  if (auto OPT = Pointee->getAs<ObjCObjectPointerType>())
+    if (auto *ID = OPT->getInterfaceDecl())
+      if (ID->getIdentifier() == S.getNSErrorIdent())
+        return true;
+
+  // Check for CFError**.
+  if (auto PT = Pointee->getAs<PointerType>())
+    if (auto RT = PT->getPointeeType()->getAs<RecordType>())
+      if (S.isCFError(RT->getDecl()))
+        return true;
+
+  return false;
+}
+
+static void handleSwiftError(Sema &S, Decl *D, const ParsedAttr &AL) {
+  auto hasErrorParameter = [](Sema &S, Decl *D, const ParsedAttr &AL) -> bool {
+    if (D->isInvalidDecl())
+      return true;
+
+    for (unsigned I = 0, E = getFunctionOrMethodNumParams(D); I != E; ++I) {
+      if (isErrorParameter(S, getFunctionOrMethodParamType(D, I)))
+        return true;
+    }
+
+    S.Diag(AL.getLoc(), diag::err_attr_swift_error_no_error_parameter)
+      << AL.getNormalizedFullName() << isa<ObjCMethodDecl>(D);
+    return false;
+  };
+
+  auto hasPointerResult = [](Sema &S, Decl *D, const ParsedAttr &AL) -> bool {
+    if (D->isInvalidDecl())
+      return true;
+
+    // - C, ObjC, and block pointers are definitely okay.
+    // - References are definitely not okay.
+    // - nullptr_t is weird, but acceptable.
+    QualType RT = getFunctionOrMethodResultType(D);
+    if (RT->hasPointerRepresentation() && !RT->isReferenceType())
+      return true;
+
+    S.Diag(AL.getLoc(), diag::err_attr_swift_error_return_type)
+      << AL.getNormalizedFullName() << AL.getArgAsIdent(0)->Ident->getName()
+      << isa<ObjCMethodDecl>(D) << /*pointer*/ 1;
+    return false;
+  };
+
+  auto hasIntegerResult = [](Sema &S, Decl *D, const ParsedAttr &AL) -> bool {
+    if (D->isInvalidDecl())
+      return true;
+
+    QualType RT = getFunctionOrMethodResultType(D);
+    if (RT->isIntegralType(S.Context))
+      return true;
+
+    S.Diag(AL.getLoc(), diag::err_attr_swift_error_return_type)
+      << AL.getNormalizedFullName() << AL.getArgAsIdent(0)->Ident->getName()
+      << isa<ObjCMethodDecl>(D) << /*integeral*/ 0;
+    return false;
+  };
+
+  IdentifierLoc *Loc = AL.getArgAsIdent(0);
+  SwiftErrorAttr::ConventionKind convention;
+  if (!SwiftErrorAttr::ConvertStrToConventionKind(Loc->Ident->getName(),
+                                                  convention)) {
+    S.Diag(AL.getLoc(), diag::warn_attribute_type_not_supported)
+      << AL.getNormalizedFullName() << Loc->Ident;
+    return;
+  }
+
+  switch (convention) {
+  case SwiftErrorAttr::None:
+    // No additional validation required.
+    break;
+
+  case SwiftErrorAttr::NonNullError:
+    if (!hasErrorParameter(S, D, AL))
+      return;
+    break;
+
+  case SwiftErrorAttr::NullResult:
+    if (!hasErrorParameter(S, D, AL) || !hasPointerResult(S, D, AL))
+      return;
+    break;
+
+  case SwiftErrorAttr::NonZeroResult:
+  case SwiftErrorAttr::ZeroResult:
+    if (!hasErrorParameter(S, D, AL) || !hasIntegerResult(S, D, AL))
+      return;
+    break;
+  }
+
+  D->addAttr(::new (S.Context) SwiftErrorAttr(S.Context, AL, convention));
+}
+
 //===----------------------------------------------------------------------===//
 // Microsoft specific attribute handlers.
 //===----------------------------------------------------------------------===//
 
 UuidAttr *Sema::mergeUuidAttr(Decl *D, const AttributeCommonInfo &CI,
                               StringRef UuidAsWritten, MSGuidDecl *GuidDecl) {
   if (const auto *UA = D->getAttr<UuidAttr>()) {
     if (declaresSameEntity(UA->getGuidDecl(), GuidDecl))
static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
   // Type safety attributes.
   case ParsedAttr::AT_ArgumentWithTypeTag:
     handleArgumentWithTypeTagAttr(S, D, AL);
     break;
   case ParsedAttr::AT_TypeTagForDatatype:
     handleTypeTagForDatatypeAttr(S, D, AL);
     break;
 
+  // Swift attributes.
+  case ParsedAttr::AT_SwiftError:
+    handleSwiftError(S, D, AL);
+    break;
+
   // XRay attributes.
   case ParsedAttr::AT_XRayLogArgs:
     handleXRayLogArgsAttr(S, D, AL);
     break;
 
   case ParsedAttr::AT_PatchableFunctionEntry:
     handlePatchableFunctionEntryAttr(S, D, AL);
     break;

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: 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: TLSModel (SubjectMatchRule_variable_is_thread_local)
 // CHECK-NEXT: Target (SubjectMatchRule_function)
 // CHECK-NEXT: TestTypestate (SubjectMatchRule_function_is_member)
 // CHECK-NEXT: TrivialABI (SubjectMatchRule_record)
 // CHECK-NEXT: Uninitialized (SubjectMatchRule_variable_is_local)
 // CHECK-NEXT: UseHandle (SubjectMatchRule_variable_is_parameter)

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

+// RUN: %clang_cc1 -verify -fsyntax-only -fobjc-arc -fblocks %s
+
+@class NSError;
+
+#if __SIZEOF_POINTER__ == 4
+typedef unsigned char BOOL;
+#else
+typedef _Bool BOOL;
+#endif
+
+typedef struct __attribute__((__objc_bridge__(NSError))) __CFError *CFErrorRef;
+
+@interface Erroneous
+- (BOOL)m0:(NSError**)error __attribute__((__swift_error__(none)));
+- (BOOL)m1:(NSError**)error __attribute__((__swift_error__(nonnull_error)));
+- (BOOL)m2:(NSError**)error __attribute__((__swift_error__(null_result)));
+// expected-error@-1 {{'swift_error' attribute with 'null_result' convention can only be applied to a method returning a pointer}}
+- (BOOL)m3:(NSError**)error __attribute__((__swift_error__(nonzero_result)));
+- (BOOL)m4:(NSError**)error __attribute__((__swift_error__(zero_result)));
+
+- (Undeclared)n0:(NSError**)error __attribute__((__swift_error__(none)));
+// expected-error@-1 {{expected a type}}
+- (Undeclared)n1:(NSError**)error __attribute__((__swift_error__(nonnull_error)));
+// expected-error@-1 {{expected a type}}
+- (Undeclared)n2:(NSError**)error __attribute__((__swift_error__(null_result)));
+// expected-error@-1 {{expected a type}}
+- (Undeclared)n3:(NSError**)error __attribute__((__swift_error__(nonzero_result)));
+// expected-error@-1 {{expected a type}}
+// FIXME: the follow-on warning should really be suppressed, but apparently
+// having an ill-formed return type doesn't mark anything as invalid.
+// expected-error@-4 {{can only be applied}}
+- (Undeclared)n4:(NSError**)error __attribute__((__swift_error__(zero_result)));
+// expected-error@-1 {{expected a type}}
+// FIXME: the follow-on warning should really be suppressed, but apparently
+// having an ill-formed return type doesn't mark anything as invalid.
+// expected-error@-4 {{can only be applied}}
+
+- (instancetype)o0 __attribute__((__swift_error__(none)));
+- (instancetype)o1 __attribute__((__swift_error__(nonnull_error)));
+// expected-error@-1 {{'swift_error' attribute can only be applied to a method with an error parameter}}
+- (instancetype)o2 __attribute__((__swift_error__(null_result)));
+// expected-error@-1 {{'swift_error' attribute can only be applied to a method with an error parameter}}
+- (instancetype)o3 __attribute__((__swift_error__(nonzero_result)));
+// expected-error@-1 {{'swift_error' attribute can only be applied to a method with an error parameter}}
+- (instancetype)o4 __attribute__((__swift_error__(zero_result)));
+// expected-error@-1 {{'swift_error' attribute can only be applied to a method with an error parameter}}
+@end
+
+extern BOOL m0(CFErrorRef *) __attribute__((__swift_error__(none)));
+extern BOOL m1(CFErrorRef *) __attribute__((__swift_error__(nonnull_error)));
+extern BOOL m2(CFErrorRef *) __attribute__((__swift_error__(null_result)));
+// expected-error@-1 {{'swift_error' attribute with 'null_result' convention can only be applied to a function returning a pointer}}
+extern BOOL m3(CFErrorRef *) __attribute__((__swift_error__(nonzero_result)));
+extern BOOL m4(CFErrorRef *) __attribute__((__swift_error__(zero_result)));
+
+extern Undeclared n0(CFErrorRef *) __attribute__((__swift_error__(none)));
+// expected-error@-1 {{unknown type name 'Undeclared'}}
+extern Undeclared n1(CFErrorRef *) __attribute__((__swift_error__(nonnull_error)));
+// expected-error@-1 {{unknown type name 'Undeclared'}}
+extern Undeclared n2(CFErrorRef *) __attribute__((__swift_error__(null_result)));
+// expected-error@-1 {{unknown type name 'Undeclared'}}
+extern Undeclared n3(CFErrorRef *) __attribute__((__swift_error__(nonzero_result)));
+// expected-error@-1 {{unknown type name 'Undeclared'}}
+extern Undeclared n4(CFErrorRef *) __attribute__((__swift_error__(zero_result)));
+// expected-error@-1 {{unknown type name 'Undeclared'}}
+
+extern void *o0(CFErrorRef *) __attribute__((__swift_error__(none)));
+extern void *o1(CFErrorRef *) __attribute__((__swift_error__(nonnull_error)));
+extern void *o2(CFErrorRef *) __attribute__((__swift_error__(null_result)));
+extern void *o3(CFErrorRef *) __attribute__((__swift_error__(nonzero_result)));
+// expected-error@-1 {{'swift_error' attribute with 'nonzero_result' convention can only be applied to a function returning an integeral type}}
+extern void *o4(CFErrorRef *) __attribute__((__swift_error__(zero_result)));
+// expected-error@-1 {{'swift_error' attribute with 'zero_result' convention can only be applied to a function returning an integeral type}}
+
+extern void *p0(void) __attribute__((__swift_error__(none)));
+extern void *p1(void) __attribute__((__swift_error__(nonnull_error)));
+// expected-error@-1 {{'swift_error' attribute can only be applied to a function with an error parameter}}
+extern void *p2(void) __attribute__((__swift_error__(null_result)));
+// expected-error@-1 {{'swift_error' attribute can only be applied to a function with an error parameter}}
+extern void *p3(void) __attribute__((__swift_error__(nonzero_result)));
+// expected-error@-1 {{'swift_error' attribute can only be applied to a function with an error parameter}}
+extern void *p4(void) __attribute__((__swift_error__(zero_result)));
+// expected-error@-1 {{'swift_error' attribute can only be applied to a function with an error parameter}}
+
+extern BOOL b __attribute__((__swift_error__(none)));
+// expected-error@-1 {{attribute only applies to functions and Objective-C methods}}