Page MenuHomePhabricator

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
gribozavr2 added inline comments.Jul 27 2020, 12:43 AM
clang/test/Sema/ns_error_enum.c
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
5332

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

5344

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

5354

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

5354

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
5354

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?

5354

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
5354

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

5354

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
5354

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
5342

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

5358
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

Please use spaces instead of tabs.

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

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
5354

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
5354

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

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

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
5346

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.

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.