Introduce ns_error_domain attribute.

[MForster]

Summary

ns_error_domain can be used by, e.g. NS_ERROR_ENUM, in order to
identify a global declaration representing the domain constant.

Introduces the attribute, Sema handling, diagnostics, and test case.

This is cherry-picked from https://github.com/llvm/llvm-project-staging/commit/a14779f504b02ad0e4dbc39d6d10cadc7ed4cfd0
and adapted to updated Clang APIs.

[gribozavr2]

clang/include/clang/Basic/Attr.td

1860

Could we try to add a list of subjects here? It seems like it is a type-only attribute, and most likely enum-only.

let Subjects = SubjectList<[Enum]>;

clang/include/clang/Basic/AttrDocs.td

3317

Shouldn't the category be DocCatType since it is a type attribute?

3319

I think we should try to expand the docs. For example:

In Cocoa frameworks in Objective-C, one can group related error codes in enums, and categorize these enums with error domains.

The `ns_error_domain` attribute specifies the error domain that error codes belong to. This attribute is attached to enums that describe error codes.

This metadata is useful for documentation purposes, for static analysis, and for improving interoperability between Objective-C and Swift. It is not used for code generation in Objective-C.

For example:

(insert an example from tests)

clang/test/Analysis/ns_error_enum.m

2

This file is a .m -- any specific reason? I'd call it .c and run the test in C, Objective-C, and C++ modes (enums might work slightly differently, the name lookup functionality might work differently).

[MForster]

• Address review comments

[MForster]

clang/include/clang/Basic/Attr.td

1860

@milseman, could you comment on this?

In the meantime I've added the restriction. Obviously this makes the tests fail. I will also test this change against the Swift unit tests.

clang/test/Analysis/ns_error_enum.m

2

The test doesn't compile in C or C++ (non-defining declaration of enumeration with a fixed underlying type is only permitted as a standalone declaration; missing list of enumerators?). Not sure if it's worth adapting.

[gribozavr2]

Did your latest update unintentionally drop the test file clang/test/Analysis/ns_error_enum.m?

[aaron.ballman]

FWIW, this is *really* hard to review because it's not a diff against the trunk and so it's not immediately clear what the actual changes are.

The change is missing all of its test coverage.

clang/include/clang/Basic/Attr.td

1860

FWIW, this is not a attribute; it's a declaration attribute.

Is there a reason it's not inheritable?

I assume it's not getting a Clang spelling because Objective-C isn't tracking C2x yet? (Though that spelling still seems useful to Objective-C++ users in general for these NS attributes.)

clang/include/clang/Basic/AttrDocs.td

3317

This is not a type attribute. It should be set to DocCatDecl.

clang/lib/Sema/SemaDeclAttr.cpp

5329–5363

This shouldn't be necessary as the common attribute handler checks subjects. Also, it's the wrong subject (this would allow things like putting the attribute onto a struct).

5329–5363

Where is this error defined?

5329–5363

Please do not name the parameter with the same name as a type.

5329–5363

Doesn't match the usual naming conventions. Same issue elsewhere in the patch.

5329–5363

if (const Expr *E = Attr.getArgAsExpr(0))
  loc = E->getBeginLoc();

[MForster]

Fix diff

[MForster]

In D84005#2158019, @gribozavr2 wrote:

Did your latest update unintentionally drop the test file clang/test/Analysis/ns_error_enum.m?

I'm struggling with arcanist :-). Fixed.

[MForster]

In D84005#2158029, @aaron.ballman wrote:

FWIW, this is *really* hard to review because it's not a diff against the trunk and so it's not immediately clear what the actual changes are.

The change is missing all of its test coverage.

I'm sorry. This was me struggling with arcanist. It should be a diff against trunk now.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

9450

ns_error_domain -> 'ns_error_domain' (with the single quotes around it, because it's syntax).

9454

I don't think this requires a custom diagnostic -- we can use err_attribute_argument_n_type instead.

clang/lib/Sema/SemaDeclAttr.cpp

5329–5363

Where is this error defined?

Now I found it, it was dropped in the version of the review I was looking at. :-)

clang/test/Analysis/ns_error_enum.m

1

I think this test belongs in Sema, not in Analysis (nothing about the feature is specific to static analysis).

Also, missing Sema tests that the attribute appertains to the correct subject, accepts the correct number and types of arguments, etc.

I'd also appreciate seeing tests of the edge cases, like a locally-defined enumeration, a forward declaration of an enumeration, etc.

2

Enums with fixed underlying types exist in C++ and C, so I was expecting the attribute to work there. If the attribute isn't supported in these languages, should the attribute be tied to a language mode?

[gribozavr2]

clang/include/clang/Basic/Attr.td

1860

FWIW, this is not a attribute; it's a declaration attribute.

Sorry, yes, of course I meant to say "declaration attribute".

Is there a reason it's not inheritable?

Good observation, I think it should be.

I assume it's not getting a Clang spelling because Objective-C isn't tracking C2x yet?

Cocoa users are expected to use the NS_* macros instead of using the attribute directly, so even if a C2x spelling was an option (IDK if it is), there would be very limited use for it.

clang/test/Analysis/ns_error_enum.m

2

There are Apple SDK headers that parse in all language modes (C, Objective-C, C++, Objective-C++), so I think it is quite important to test this feature in all modes. I suspect the reason for the error is that different language modes require a slightly different macro definition.

[aaron.ballman]

clang/include/clang/Basic/Attr.td

1860

Cocoa users are expected to use the NS_* macros instead of using the attribute directly, so even if a C2x spelling was an option (IDK if it is), there would be very limited use for it.

Okay, that's reasonable, thanks!

clang/test/Analysis/ns_error_enum.m

2

There are Apple SDK headers that parse in all language modes (C, Objective-C, C++, Objective-C++), so I think it is quite important to test this feature in all modes.

In that case, I definitely agree. This should have multiple RUN lines to test the various language modes.

[MForster]

• Run test also in C and C++ mode
• Address more review comments

clang/include/clang/Basic/Attr.td

1860

Is there a reason it's not inheritable?

I made it inheritable. @milseman, any comment on whether that is appropriate?

clang/test/Analysis/ns_error_enum.m

2

There are Apple SDK headers that parse in all language modes (C, Objective-C, C++, Objective-C++), so I think it is quite important to test this feature in all modes.

In that case, I definitely agree. This should have multiple RUN lines to test the various language modes.

So, it seems that the error message is quite new. I suspect that the Apple SDK headers have not yet been updated. At least I'm getting the same error messages when running the test with the (rather elaborate) original definitions of CF_ENUM and NS_ERROR_ENUM.

For now I have just disabled the diagnostic for C and C++ (-Wno-elaborated-enum-base). For ObjC it is disabled anyway.

[MForster]

clang/include/clang/Basic/Attr.td

1860

Could we try to add a list of subjects here? It seems like it is a type-only attribute, and most likely enum-only.

let Subjects = SubjectList<[Enum]>;

@milseman, could you comment on this?

In the meantime I've added the restriction. Obviously this makes the tests fail. I will also test this change against the Swift unit tests.

FWIW, no failures in the Swift tests.

[aaron.ballman]

It's a bit odd that this attribute has an AST node created for it but nothing is using that AST node elsewhere in the project. Are there other patches expected for making use of this attribute?

clang/include/clang/Basic/AttrDocs.td

3330

I still feel like I'm lacking information about the domain. In the example, the domain is an uninitialized const char *, so how does this information get used by the attribute? Can I use any type of global I want? What about a literal?

clang/include/clang/Basic/DiagnosticSemaKinds.td

9449

This diagnostic doesn't match the attribute definition (the subject list only lists enumerations). You should be able to remove this diagnostic entirely and just rely on the common attribute handler to diagnose incorrect subjects, though you should add an ErrorDiag to the subject list in Attr.td.

clang/lib/Sema/SemaDeclAttr.cpp

5330

Please don't use Attr as a declaration identifier, it's the name of a type already and that confuses some editors.

5331

This code should be removed, the common attribute handler already diagnoses incorrect subjects (and this doesn't match the interface in Attr.td anyway).

5336

Variables should match the usual coding style conventions.

5355

We're doing this lookup in the context of where the attribute is being used, but then creating the attribute with only the identifier and not the result of that lookup. This makes me a bit worried that when something goes to resolve that identifier to a variable later, it may get a different result because the lookup context will be different. Do you need to store the VarDecl on the semantic attribute, rather than the identifier?

clang/test/Analysis/ns_error_enum.c

1 ↗

(On Diff #279228)

The test should still be moved out of Analysis and into Sema.

3 ↗

(On Diff #279228)

One more run line for objective-c++?

[MForster]

Address review comments

[MForster]

In D84005#2162433, @aaron.ballman wrote:

It's a bit odd that this attribute has an AST node created for it but nothing is using that AST node elsewhere in the project. Are there other patches expected for making use of this attribute?

It gets used from Swift.

clang/include/clang/Basic/AttrDocs.td

3330

I won't claim that I understand this well. Maybe @gribozavr2 or @milseman can explain this better. Literals are not allowed, however. The test explicitly forbids that.

clang/include/clang/Basic/DiagnosticSemaKinds.td

9454

I don't think this requires a custom diagnostic -- we can use err_attribute_argument_n_type instead.

Done. Can you please confirm that n is 1-based?

clang/lib/Sema/SemaDeclAttr.cpp

5355

We're doing this lookup in the context of where the attribute is being used, but then creating the attribute with only the identifier and not the result of that lookup. This makes me a bit worried that when something goes to resolve that identifier to a variable later, it may get a different result because the lookup context will be different. Do you need to store the VarDecl on the semantic attribute, rather than the identifier?

When this gets imported into Swift, only the name of the identifier gets used. I'm not quite sure why this was defined like this. This is another thing where I would hope that @gribozavr2 or @milseman know more...

[MForster]

@milseman, @doug.gregor, could you please help with the open questions on this review?

Specifically:

• Is ns_error_domain ever needed for something other than an enum?
• Why is this designed in the way it is (requiring an identifier for the domain, not allowing literals and then only using the name of the identifier from Swift)?
• Is it ok to make this attribute inheritable?

[riccibruno]

clang/lib/Sema/SemaDeclAttr.cpp

5349

Just a note that LookupResult::getAsSingle has tricky semantics (returns null if the result is not exactly LookupResultKind::Found) and has been (and still is) the source of many bugs in clang.

(Example: my favourite one is still the silly:

struct S {
  void not_overloaded();
  enum { not_overloaded }; // error; redefinition of 'not_overloaded'

  void overloaded();
  void overloaded(int);
  enum { overloaded }; // clang is fine with this!
};

)

I don't know if it is a problem here or not though.

[aaron.ballman]

In D84005#2171962, @MForster wrote:

In D84005#2162433, @aaron.ballman wrote:

It's a bit odd that this attribute has an AST node created for it but nothing is using that AST node elsewhere in the project. Are there other patches expected for making use of this attribute?

It gets used from Swift.

Are there plans to use this attribute in Clang itself? If not, is there a way Swift can use attribute plugins instead of modifying Clang?

clang/include/clang/Basic/AttrDocs.td

3330

Literals are not allowed, however. The test explicitly forbids that.

And I'm wondering why. For instance, attributes with enumeration arguments accept the enumerator as either an identifier or a string literal. I was sort of assuming that these domains are notionally like enumerators in that there is a list of domains, which is why I was asking about literals.

clang/include/clang/Basic/DiagnosticSemaKinds.td

9454

Yes, the argument positions are 1-based.

clang/lib/Sema/SemaDeclAttr.cpp

5336

It looks like loc was missed and still need to be updated to Loc.

[doug.gregor]

In D84005#2171982, @MForster wrote:

@milseman, @doug.gregor, could you please help with the open questions on this review?

Specifically:

• Is ns_error_domain ever needed for something other than an enum?

No, it only makes sense on enums.

• Why is this designed in the way it is (requiring an identifier for the domain, not allowing literals and then only using the name of the identifier from Swift)?

It's codifying the design of NSError, which has been this way since... longer than Clang has existed, and is independent of Swift. NSErrors have a domain (identified by an NSString constant symbol, not a literal, for pointer uniqueness and code size) and a code (an integer, conventionally defined by an enum). The two need to be used together, e.g., you create an NSError with a domain and a code from that domain. This attribute finally ties those things together at the source level.

This is leads to the answer to Aaron's question:

Are there plans to use this attribute in Clang itself?

It would absolutely make sense to add some warnings if you've mismatched your domain and code when constructing an NSError (e.g., uses of https://developer.apple.com/documentation/foundation/nserror/1522782-errorwithdomain?language=objc) or even if when testing an error in an "if" statement (if checking both domain and code, make sure the code enumerator is from the same domain). I bet you'd catch some fiddly error-handling bugs this way.

• Is it ok to make this attribute inheritable?

Sure.

[aaron.ballman]

In D84005#2172747, @doug.gregor wrote:

In D84005#2171982, @MForster wrote:

@milseman, @doug.gregor, could you please help with the open questions on this review?

Specifically:

• Is ns_error_domain ever needed for something other than an enum?

No, it only makes sense on enums.

• Why is this designed in the way it is (requiring an identifier for the domain, not allowing literals and then only using the name of the identifier from Swift)?

It's codifying the design of NSError, which has been this way since... longer than Clang has existed, and is independent of Swift. NSErrors have a domain (identified by an NSString constant symbol, not a literal, for pointer uniqueness and code size) and a code (an integer, conventionally defined by an enum). The two need to be used together, e.g., you create an NSError with a domain and a code from that domain. This attribute finally ties those things together at the source level.

Ah, thank you, that makes the design far more clear to me.

[MForster]

• Address review comments

[MForster]

I have updated the attribute documentation to include the additional information provided by Doug. I think adding additional diagnostics would rather be separate changes.

I think I have addressed all remaining review comments.

clang/lib/Sema/SemaDeclAttr.cpp

5349

Just a note that LookupResult::getAsSingle has tricky semantics [...]

Thanks for the advice, but I think this is not an issue here. The check should only succeed if there is a unique lookup result.

[gribozavr2]

clang/include/clang/Basic/AttrDocs.td

3340

const char * => NSString * const?

clang/test/Sema/ns_error_enum.c

25 ↗

(On Diff #280804)

const char * => NSString * const? You'd need to define a fake NSString type, but that should be rather easy:

@interface NSString
@end

30 ↗

(On Diff #280804)

"MyStructWithErrorDomain" would be a better name, I think.

42 ↗

(On Diff #280804)

Also a test for passing 0 or more than 1 argument to the attribute?

[MForster]

Rebase

[MForster]

Address review comments

[MForster]

clang/test/Sema/ns_error_enum.c

25 ↗

(On Diff #280804)

const char * => NSString * const?

Done.

Turns out that this makes the test incompatible with C and C++. Some more research makes me believe that this feature is only supposed to be used from ObjC(++). I removed the C/C++ RUN lines from this test. I will also send a follow-up change to turn this into an error in those languages. But I want to make it a separate change because of the potential for breakage in existing code.

[MForster]

Fix typo

[MForster]

Rename test back to ns_error_enum.m

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

5333

Comments should be grammatical including punctuation (elsewhere as well).

5345

lookupResult -> Result (or something else that matches the usual naming conventions).

5355

Based on the answer from @doug.gregor, I think this should be adding the result of the lookup to the semantic attribute and not the identifier (the identifier may not be unique, the VarDecl must be unique though).

5355

Shouldn't we also be validating that what we found is an NSString that has the correct properties? (That doesn't seem like it should be a change which risks breaking code based on what I understood from Doug's description.)

[MForster]

Two clarifying questions...

clang/lib/Sema/SemaDeclAttr.cpp

5355

Based on the answer from @doug.gregor, I think this should be adding the result of the lookup to the semantic attribute and not the identifier (the identifier may not be unique, the VarDecl must be unique though).

How are you suggesting to implement that? Change the argument to to be a DeclArgument or ExprArgument instead of an IdentifierArgument?

5355

Shouldn't we also be validating that what we found is an NSString that has the correct properties?

Is your suggestion to string-compare the name of the type?

[aaron.ballman]

clang/lib/Sema/SemaDeclAttr.cpp

5355

Shouldn't we also be validating that what we found is an NSString that has the correct properties?

Is your suggestion to string-compare the name of the type?

You should be able to compare the QualType of the resulting VarDecl against ASTContext::getObjCNSStringType() (accounting for qualifiers, pointers, etc).

5355

Based on the answer from @doug.gregor, I think this should be adding the result of the lookup to the semantic attribute and not the identifier (the identifier may not be unique, the VarDecl must be unique though).

How are you suggesting to implement that? Change the argument to to be a DeclArgument or ExprArgument instead of an IdentifierArgument?

I think DeclArgument is probably the correct approach and you should be able to model it somewhat off the cleanup attribute. Otherwise, you can gin up a "fake" attribute argument. Those aren't arguments used by the parser or pretty printer, but are used to form the semantic attribute constructor to provide additional information.

[MForster]

Store the VarDecl instead of the identifier

clang/lib/Sema/SemaDeclAttr.cpp

5355

Turns out that this didn't work well. ASTContext::getObjCNSStringType() is only initialized if ObjC string literals are instantiated without an NSString type being defined. In our case there is an NSString type, because we need to declare a global variable of that type.

I've resorted to a string comparison after all.

[MForster]

Rebase against master

[riccibruno]

clang/lib/Sema/SemaDeclAttr.cpp

5343

Just send the declaration into the diagnostic. See the recent D84656 for the rationale.

5359

[MForster]

Use declaration in diagnostics instead of name

[MForster]

Apply ClangTidy suggestions

[gribozavr2]

clang/lib/Sema/SemaDeclAttr.cpp

3650–3651

Please use spaces instead of tabs.

[riccibruno]

clang/lib/Sema/SemaDeclAttr.cpp

3650–3651

Sorry about that, fixed in 517fe058d42a1f937e14de4b61a5ac2ad326c850

[MForster]

Rebase

[aaron.ballman]

LGTM aside from an almost NFC simplification.

clang/lib/Sema/SemaDeclAttr.cpp

5355

Well that's really unfortunate to learn, but good news: isNSStringType() is in SemaDeclAttr.cpp and I hadn't noticed that before, so I think you can use that instead, assuming that a mutable string is acceptable for the API. If mutable strings are not acceptable, then I think we should add a new parameter to isNSStringType() to handle it.

[MForster]

Simplify code

[MForster]

clang/lib/Sema/SemaDeclAttr.cpp

5355

Nice find. Should have found this myself.

[MForster]

Fix patch

[MForster]

For context, this is the backported change, to be applied downstream before landing this review: https://github.com/apple/llvm-project/pull/1565

[MForster]

Pretty-print VarDecl arguments correctly

[aaron.ballman]

clang/utils/TableGen/ClangAttrEmitter.cpp

332 ↗

(On Diff #282168)

I think this change is good, but if you wouldn't mind adding an ast dumping test (in the AST test directory) that exercises the change, I'd appreciate it.

[MForster]

Add an -ast-print test

[gribozavr2]

clang/test/AST/ast-print-attr.c

20 ↗

(On Diff #284286)

Stray "2".

[MForster]

Fix mistake

[aaron.ballman]

Continues to LGTM with your changes, but I did spot one tiny nit (no further reviews needed from me).

clang/lib/Sema/SemaDeclAttr.cpp

5347

Minor nit, you can pass VD instead of DRE->getDecl() here.

[MForster]

Fix nit

[MForster]

Rebase before submission

[thakis]

Looks like this breaks check-clangd on Windows: http://45.33.8.238/win/21943/step_9.txt

Please take a look, and revert while you investigate if the fix takes a while.

[MForster]

In D84005#2215685, @thakis wrote:

Looks like this breaks check-clangd on Windows: http://45.33.8.238/win/21943/step_9.txt

Update from an offline conversation: This may actually be rather an issue with the test, which was introduced just today: https://reviews.llvm.org/D80525. We're looking into reverting that instead.

clang/include/clang/Basic/Attr.td

def ObjCBridgeRelated : InheritableAttr {
   let Subjects = SubjectList<[Record], ErrorDiag>;
   let Args = [IdentifierArgument<"RelatedClass">,
           IdentifierArgument<"ClassMethod">,
           IdentifierArgument<"InstanceMethod">];
   let HasCustomParsing = 1;
   let Documentation = [Undocumented];
 }
 
+def NSErrorDomain : Attr {
+  let Spellings = [GNU<"ns_error_domain">];
+  let Subjects = SubjectList<[Enum]>;
+  let Args = [IdentifierArgument<"ErrorDomain">];
+  let Documentation = [NSErrorDomainDocs];
+}
+
 def NSReturnsRetained : DeclOrTypeAttr {
   let Spellings = [Clang<"ns_returns_retained">];
 //  let Subjects = SubjectList<[ObjCMethod, ObjCProperty, Function]>;
   let Documentation = [RetainBehaviorDocs];
 }
 
 def NSReturnsNotRetained : InheritableAttr {
   let Spellings = [Clang<"ns_returns_not_retained">];

clang/include/clang/Basic/AttrDocs.td

def NoAliasDocs : Documentation {
   let Category = DocCatFunction;
   let Content = [{
 The ``noalias`` attribute indicates that the only memory accesses inside
 function are loads and stores from objects pointed to by its pointer-typed
 arguments, with arbitrary offsets.
   }];
 }
 
+def NSErrorDomainDocs : Documentation {
+  let Category = DocCatType;
+  let Content = [{
+In Cocoa frameworks in Objective-C, one can group related error codes in enums
+and categorize these enums with error domains.
+
+The ``ns_error_domain`` attribute indicates a global constant representing the
+error domain that error codes belong to. This attribute is attached to enums
+that describe error codes.
+
+This metadata is useful for documentation purposes, for static analysis, and for
+improving interoperability between Objective-C and Swift. It is not used for
+code generation in Objective-C.
+
+For example:
+
+  .. code-block:: objc
+
+    #define NS_ERROR_ENUM(_type, _name, _domain)  \
+      enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+    const char *MyErrorDomain;
+    typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+      MyErrFirst,
+      MyErrSecond,
+    };
+  }];
+}
+
 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_type_unsupported : Error<
   "%0 is not supported on this target">;
 def err_nsconsumed_attribute_mismatch : Error<
   "overriding method has mismatched ns_consumed attribute on its"
   " parameter">;
 def err_nsreturns_retained_attribute_mismatch : Error<
   "overriding method has mismatched ns_returns_%select{not_retained|retained}0"
   " attributes">;
+def err_nserrordomain_not_tagdecl : Error<
+  "ns_error_domain attribute only valid on "
+  "%select{enums, structs, and unions|enums, structs, unions, and classes}0">;
+def err_nserrordomain_invalid_decl : Error<
+  "domain argument %0 does not refer to global constant">;
+def err_nserrordomain_requires_identifier : Error<
+  "domain argument must be an identifier">;
+
 def warn_nsconsumed_attribute_mismatch : Warning<
   err_nsconsumed_attribute_mismatch.Text>, InGroup<NSConsumedMismatch>;
 def warn_nsreturns_retained_attribute_mismatch : Warning<
   err_nsreturns_retained_attribute_mismatch.Text>, InGroup<NSReturnsMismatch>;
 
 def note_getter_unavailable : Note<
   "or because setter is declared here, but no getter method %0 is found">;
 def err_invalid_protocol_qualifiers : Error<

clang/lib/Sema/SemaDeclAttr.cpp

for (; Field != FieldEnd; ++Field) {
     // sufficient (consider structs passed on the stack instead of in registers
     // as an example).
     if (S.Context.getTypeSize(FieldType) != FirstSize ||
         S.Context.getTypeAlign(FieldType) > FirstAlign) {
       // Warn if we drop the attribute.
       bool isSize = S.Context.getTypeSize(FieldType) != FirstSize;
       unsigned FieldBits = isSize? S.Context.getTypeSize(FieldType)
                                  : S.Context.getTypeAlign(FieldType);
       S.Diag(Field->getLocation(),
           diag::warn_transparent_union_attribute_field_size_align)
         << isSize << Field->getDeclName() << FieldBits;
       unsigned FirstBits = isSize? FirstSize : FirstAlign;
       S.Diag(FirstField->getLocation(),
              diag::note_transparent_union_first_field_size_align)
         << isSize << FirstBits;
       return;
     }
   }
static void handleObjCRequiresSuperAttr(Sema &S, Decl *D,
   if (Method->getMethodFamily() == OMF_dealloc) {
     S.Diag(D->getBeginLoc(), diag::warn_objc_requires_super_protocol) << Attrs
                                                                       << 1;
     return;
   }
 
   D->addAttr(::new (S.Context) ObjCRequiresSuperAttr(S.Context, Attrs));
 }
 
+static void handleNSErrorDomain(Sema &S, Decl *D, const ParsedAttr &Attr) {
+  if (!isa<TagDecl>(D)) {
+    S.Diag(D->getBeginLoc(), diag::err_nserrordomain_not_tagdecl)
+        << S.getLangOpts().CPlusPlus;
+    return;
+  }
+  IdentifierLoc *identLoc =
+      Attr.isArgIdent(0) ? Attr.getArgAsIdent(0) : nullptr;
+  if (!identLoc || !identLoc->Ident) {
+    // Try to locate the argument directly
+    SourceLocation loc = Attr.getLoc();
+    if (Attr.isArgExpr(0) && Attr.getArgAsExpr(0))
+      loc = Attr.getArgAsExpr(0)->getBeginLoc();
+
+    S.Diag(loc, diag::err_nserrordomain_requires_identifier);
+    return;
+  }
+
+  // Verify that the identifier is a valid decl in the C decl namespace
+  LookupResult lookupResult(S, DeclarationName(identLoc->Ident),
+                            SourceLocation(),
+                            Sema::LookupNameKind::LookupOrdinaryName);
+  if (!S.LookupName(lookupResult, S.TUScope) ||
+      !lookupResult.getAsSingle<VarDecl>()) {
+    S.Diag(identLoc->Loc, diag::err_nserrordomain_invalid_decl)
+        << identLoc->Ident;
+    return;
+  }
+
+  D->addAttr(::new (S.Context)
+                 NSErrorDomainAttr(S.Context, Attr, identLoc->Ident));
+}
+
 static void handleObjCBridgeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   IdentifierLoc *Parm = AL.isArgIdent(0) ? AL.getArgAsIdent(0) : nullptr;
 
   if (!Parm) {
     S.Diag(D->getBeginLoc(), diag::err_objc_attr_not_id) << AL << 0;
     return;
   }
 
   // Typedefs only allow objc_bridge(id) and have some additional checking.
case ParsedAttr::AT_ObjCDesignatedInitializer:
     handleObjCDesignatedInitializer(S, D, AL);
     break;
   case ParsedAttr::AT_ObjCRuntimeName:
     handleObjCRuntimeName(S, D, AL);
     break;
   case ParsedAttr::AT_ObjCBoxable:
     handleObjCBoxable(S, D, AL);
     break;
+  case ParsedAttr::AT_NSErrorDomain:
+    handleNSErrorDomain(S, D, AL);
+    break;
   case ParsedAttr::AT_CFAuditedTransfer:
     handleSimpleAttributeWithExclusions<CFAuditedTransferAttr,
                                         CFUnknownTransferAttr>(S, D, AL);
     break;
   case ParsedAttr::AT_CFUnknownTransfer:
     handleSimpleAttributeWithExclusions<CFUnknownTransferAttr,
                                         CFAuditedTransferAttr>(S, D, AL);
     break;

clang/test/Analysis/ns_error_enum.m

+// RUN: %clang_cc1 -verify %s
+
+#define CF_ENUM(_type, _name) enum _name : _type _name; enum _name : _type
+#define NS_ENUM(_type, _name) CF_ENUM(_type, _name)
+
+#define NS_ERROR_ENUM(_type, _name, _domain)  \
+  enum _name : _type _name; enum __attribute__((ns_error_domain(_domain))) _name : _type
+
+typedef NS_ENUM(unsigned, MyEnum) {
+  MyFirst,
+  MySecond,
+};
+
+typedef NS_ENUM(invalidType, MyInvalidEnum) {
+// expected-error@-1{{unknown type name 'invalidType'}}
+// expected-error@-2{{unknown type name 'invalidType'}}
+  MyFirstInvalid,
+  MySecondInvalid,
+};
+
+const char *MyErrorDomain;
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnum, MyErrorDomain) {
+  MyErrFirst,
+  MyErrSecond,
+};
+struct __attribute__((ns_error_domain(MyErrorDomain))) MyStructErrorDomain {};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, InvalidDomain) {
+  // expected-error@-1{{domain argument 'InvalidDomain' does not refer to global constant}}
+  MyErrFirstInvalid,
+  MyErrSecondInvalid,
+};
+
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalid, "domain-string");
+  // expected-error@-1{{domain argument must be an identifier}}
+
+int __attribute__((ns_error_domain(MyErrorDomain))) NotTagDecl;
+  // expected-error@-1{{ns_error_domain attribute only valid on enums, structs, and unions}}
+
+void foo() {}
+typedef NS_ERROR_ENUM(unsigned char, MyErrorEnumInvalidFunction, foo);
+  // expected-error@-1{{domain argument 'foo' does not refer to global constant}}