This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy/checks] Update objc-property-declaration check to allow arbitrary acronyms and initialisms 🔧
ClosedPublic

Authored by stephanemoore on Sep 8 2018, 12:04 PM.

Details

Summary

§1 Description

This changes the objc-property-declaration check to allow arbitrary acronyms and initialisms instead of using whitelisted acronyms. In Objective-C it is relatively common to use project prefixes in property names for the purposes of disambiguation. For example, the CIColor¹ and CGColor² properties on UIColor both represent symbol prefixes being used in proeprty names outside of Apple's accepted acronyms³. The union of Apple's accepted acronyms and all symbol prefixes that might be used for disambiguation in property declarations effectively allows for any arbitrary sequence of capital alphanumeric characters to be acceptable in property declarations. This change updates the check accordingly.

The test variants with custom configurations are deleted as part of this change because their configurations no longer impact behavior. The acronym configurations are currently preserved for backwards compatibility of check configuration.

[1] https://developer.apple.com/documentation/uikit/uicolor/1621951-cicolor?language=objc
[2] https://developer.apple.com/documentation/uikit/uicolor/1621954-cgcolor?language=objc
[3] https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/APIAbbreviations.html#//apple_ref/doc/uid/20001285-BCIHCGAE

§2 Test Notes

Changes verified by:
• Running clang-tidy unit tests.
• Used check_clang_tidy.py to verify expected output of processing objc-property-declaration.m

Diff Detail

Repository
rL LLVM

Event Timeline

stephanemoore created this revision.Sep 8 2018, 12:04 PM

Fix a couple issues:
• Forward declare NSArray in objc-property-declaration.m.
• Remove unnecessary lightweight generics declaration from IDs property in objc-property-declaration.m to fix compilation error.

Sorted forward declared classes in objc-property-declaration.m.

benhamilton accepted this revision.Sep 10 2018, 8:16 AM

This is fine, but please update the comments (and docs?) to make it clear that we no longer enforce camelCase but allow aRBiTraRYcAsE now.

clang-tidy/objc/PropertyDeclarationCheck.cpp
28–31 ↗(On Diff #164583)

These comments are no longer accurate.

73–74 ↗(On Diff #164583)

Just to be clear, this also allows things like aRbITRaRyCapS, right? We should comment that this is explicitly by design.

113–115 ↗(On Diff #164583)

This comment is no longer accurate.

This revision is now accepted and ready to land.Sep 10 2018, 8:16 AM
stephanemoore marked an inline comment as done.

Updated comments, release notes, and documentation to be consistent with the implemented changes.

I believe that it should be safe to remove the Acronyms and IncludeDefaultAcronyms options but if it's alright with you, I would prefer to separate that into a followup change.

clang-tidy/objc/PropertyDeclarationCheck.cpp
28–31 ↗(On Diff #164583)

I think the comments are still more or less accurate? I think we have just changed the level of enforcement that we are applying.

We allow aRbITRaRyCapS now but that's based on the concession that it's fundamentally hard to identify appropriate lowerCamelCase with high precision (i.e., it is hard to reject "aRbITRaRyCapS" as a representation of "a rb IT ra ry cap S" without human judgment). I added a note that acronyms and initialisms are capitalized regardless of style though.

Sidenote:
A spark of curiosity led to me realizing that the previous approach would also have allowed aRbITRaRyCapS if all letters of the alphabet were to be whitelisted as acronyms. The previous check already included several letters in the default special acronyms and provided special treatment for "A" and "I" in the matching regex. I can't say for certain that every letter of the alphabet would have ended up being whitelisted but I audited the letters of the alphabet and I think I managed to construct a list of terms that could conceivably have been used in code and might have led to the whitelisting of each individual letter of the alphabet (attached below¹).

[1] List of Letters in English Alphabet and Terms Containing Them in Isolation

Chatted with Ben offline and he's okay with proceeding.

Rebased changes and resolved merge conflicts.
Moved release notes update next to other release notes regarding updates to existing checks.

Wizard accepted this revision.Dec 4 2018, 7:38 PM
This revision was automatically updated to reflect the committed changes.
clang-tools-extra/trunk/docs/clang-tidy/checks/objc-property-declaration.rst