This is an archive of the discontinued LLVM Phabricator instance.

Introduce ns_error_domain attribute.
ClosedPublic

Authored by MForster on Jul 17 2020, 12:58 AM.

Details

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.

Diff Detail

Event Timeline

MForster created this revision.Jul 17 2020, 12:58 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript
MForster edited the summary of this revision. (Show Details)Jul 17 2020, 1:00 AM
gribozavr2 added inline comments.Jul 17 2020, 2:09 AM
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
1 ↗(On Diff #278672)

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 updated this revision to Diff 278724.Jul 17 2020, 4:38 AM
MForster marked 4 inline comments as done.
  • Address review comments
MForster marked an inline comment as done and an inline comment as not done.Jul 17 2020, 4:46 AM
MForster added inline comments.
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
1 ↗(On Diff #278672)

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.

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

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
5330

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

5331

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).

5332

Where is this error defined?

5336

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

5341
if (const Expr *E = Attr.getArgAsExpr(0))
  loc = E->getBeginLoc();
MForster updated this revision to Diff 278728.Jul 17 2020, 5:02 AM
MForster marked an inline comment as not done.

Fix diff

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

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

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 added inline comments.Jul 17 2020, 5:12 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
9450 ↗(On Diff #278728)

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

9454 ↗(On Diff #278728)

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

clang/lib/Sema/SemaDeclAttr.cpp
5332

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 ↗(On Diff #278728)

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.

1 ↗(On Diff #278672)

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 added inline comments.Jul 17 2020, 5:39 AM
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
1 ↗(On Diff #278672)

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 added inline comments.Jul 17 2020, 6:49 AM
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
1 ↗(On Diff #278672)

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 updated this revision to Diff 279228.Jul 20 2020, 6:43 AM
MForster marked 13 inline comments as done.
  • 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
1 ↗(On Diff #278672)

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 marked an inline comment as done.Jul 20 2020, 7:14 AM
MForster added inline comments.
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.

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
9456 ↗(On Diff #279228)

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 updated this revision to Diff 280415.Jul 24 2020, 5:17 AM
MForster marked 15 inline comments as done.

Address review comments

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 ↗(On Diff #278728)

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...

@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 added inline comments.
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.

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 ↗(On Diff #278728)

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.

@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.

@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 edited the summary of this revision. (Show Details)Jul 27 2020, 12:29 AM
MForster edited reviewers, added: doug.gregor; removed: milseman, jdoerfert.
MForster added subscribers: jdoerfert, milseman.
MForster updated this revision to Diff 280804.Jul 27 2020, 12:32 AM
MForster marked 8 inline comments as done.
MForster edited the summary of this revision. (Show Details)
  • Address review comments

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 accepted this revision.Jul 27 2020, 12:43 AM
gribozavr2 added inline comments.
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?

This revision is now accepted and ready to land.Jul 27 2020, 12:43 AM
MForster updated this revision to Diff 280828.Jul 27 2020, 2:32 AM
MForster marked 3 inline comments as done.

Address review comments

MForster marked an inline comment as done.Jul 27 2020, 2:32 AM
MForster added inline comments.
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 updated this revision to Diff 280829.Jul 27 2020, 2:34 AM
MForster marked an inline comment as done.

Fix typo

gribozavr2 accepted this revision.Jul 27 2020, 2:36 AM
MForster updated this revision to Diff 280831.Jul 27 2020, 2:46 AM

Rename test back to ns_error_enum.m

aaron.ballman added inline comments.Jul 27 2020, 5:25 AM
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 marked 3 inline comments as done.Jul 27 2020, 6:43 AM

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 added inline comments.Jul 27 2020, 6:56 AM
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 updated this revision to Diff 281525.Jul 29 2020, 5:00 AM
MForster marked 4 inline comments as done.

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 updated this revision to Diff 281531.Jul 29 2020, 5:28 AM

Rebase against master

riccibruno added inline comments.Jul 29 2020, 5:48 AM
clang/lib/Sema/SemaDeclAttr.cpp
5343

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

5359
MForster updated this revision to Diff 281546.Jul 29 2020, 6:04 AM

Use declaration in diagnostics instead of name

MForster marked 2 inline comments as done.Jul 29 2020, 6:04 AM
MForster updated this revision to Diff 281550.Jul 29 2020, 6:25 AM

Apply ClangTidy suggestions

gribozavr2 accepted this revision.Jul 29 2020, 6:35 AM
gribozavr2 added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
3650–3651

Please use spaces instead of tabs.

riccibruno added inline comments.Jul 29 2020, 6:57 AM
clang/lib/Sema/SemaDeclAttr.cpp
3650–3651

Sorry about that, fixed in 517fe058d42a1f937e14de4b61a5ac2ad326c850

aaron.ballman accepted this revision.Jul 30 2020, 6:31 AM

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 updated this revision to Diff 281927.Jul 30 2020, 7:36 AM

Simplify code

MForster marked an inline comment as done.Jul 30 2020, 7:37 AM
MForster added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
5355

Nice find. Should have found this myself.

MForster updated this revision to Diff 281928.Jul 30 2020, 7:41 AM
MForster marked 5 inline comments as done.

Fix patch

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

MForster updated this revision to Diff 282168.Jul 31 2020, 2:50 AM

Pretty-print VarDecl arguments correctly

aaron.ballman added inline comments.Jul 31 2020, 4:33 AM
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 updated this revision to Diff 284286.Aug 10 2020, 2:37 AM

Add an -ast-print test

MForster marked an inline comment as done.Aug 10 2020, 2:38 AM
gribozavr2 accepted this revision.Aug 10 2020, 2:48 AM
gribozavr2 added inline comments.
clang/test/AST/ast-print-attr.c
20 ↗(On Diff #284286)

Stray "2".

MForster updated this revision to Diff 284288.Aug 10 2020, 2:49 AM

Fix mistake

aaron.ballman accepted this revision.Aug 10 2020, 6:34 AM

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 updated this revision to Diff 284343.Aug 10 2020, 6:41 AM
MForster marked an inline comment as done.

Fix nit

MForster marked an inline comment as done.Aug 10 2020, 6:42 AM
MForster updated this revision to Diff 285329.Aug 13 2020, 4:19 AM

Rebase before submission

This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Aug 13 2020, 7:02 AM

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.

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.