This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy ObjC] [3/3] New check objc-forbidden-subclassing
ClosedPublic

Authored by benhamilton on Oct 20 2017, 1:34 PM.

Details

Summary

This is part 3 of 3 of a series of changes to improve Objective-C
linting in clang-tidy.

This adds a new clang-tidy check objc-forbidden-subclassing which
ensures clients do not create subclasses of Objective-C classes which
are not designed to be subclassed.

(Note that for code under your control, you should use
attribute((objc_subclassing_restricted)) instead -- this
is intended for third-party APIs which cannot be modified.)

By default, the following classes (which are publicly documented
as not supporting subclassing) are forbidden from subclassing:

ABNewPersonViewController
ABPeoplePickerNavigationController
ABPersonViewController
ABUnknownPersonViewController
NSHashTable
NSMapTable
NSPointerArray
NSPointerFunctions
NSTimer
UIActionSheet
UIAlertView
UIImagePickerController
UITextInputMode
UIWebView

Clients can set a CheckOption
objc-forbidden-subclassing.ClassNames to a semicolon-separated
list of class names, which overrides this list.

Test Plan: ninja check-clang-tools

Diff Detail

Repository
rL LLVM

Event Timeline

benhamilton created this revision.Oct 20 2017, 1:34 PM
benhamilton added inline comments.Oct 20 2017, 1:38 PM
docs/clang-tidy/checks/list.rst
1–10 ↗(On Diff #119703)

FYI, I didn't actually change the order here. These were automatically alphabetized when I ran add_new_check.py.

alexfh edited edge metadata.Oct 20 2017, 2:29 PM
  1. Should we make a separate module for ObjectiveC checks?
  2. Is this check actually Google-style-specific?
hokein added inline comments.Oct 23 2017, 1:36 AM
clang-tidy/google/ObjcForbiddenSubclassingCheck.cpp
24 ↗(On Diff #119703)

static is redundant as it is in anonymous namespace. How about using constexpr char DefaultForbiddenObjcClassNames[] = ...?

Since this is a long list, using string literal (per class a line) would improve the code readability, I think.

34 ↗(On Diff #119703)

We can use the utility function utils::options::parseStringList to parse the option list, which would simplify the code.

47 ↗(On Diff #119703)

nit:: trailing // namespace

clang-tidy/google/ObjcForbiddenSubclassingCheck.h
11 ↗(On Diff #119703)

The APPLE is not intended?

28 ↗(On Diff #119703)

Maybe put it into the ObjcForbiddenSubclassingCheck class?

39 ↗(On Diff #119703)

We also need to overwrite the storeOptions method if the check has its own options.

docs/clang-tidy/checks/google-objc-forbidden-subclassing.rst
9 ↗(On Diff #119703)

I think the word subclassing should be classes?

10 ↗(On Diff #119703)

Are they documented in Apple SDK doc? It is worth providing a link, for the users who are interested to know.

14 ↗(On Diff #119703)

Maybe add more background or context words for this note paragraph.

It took me a while to understand how it is related to the check. If I understand correctly, this means instead of using this check, for the code under your control, you should use __attribute__(...) to prevent your object-c classes which cannot be subclassed?

hokein edited edge metadata.Oct 23 2017, 1:41 AM
  1. Should we make a separate module for ObjectiveC checks?
  2. Is this check actually Google-style-specific?

Copy the email response from @benhamilton (not sure why the response is not in phabricator):

Happy to make a new module, didn't know if that entailed anything special.
This is not google-specific, anyone who writes ObjC should probably use it.

Creating an object-c module seems fine, but I wonder what if we create a google-specific object-c check in the future, which module should we put the check to?

benhamilton retitled this revision from [clang-tidy] New check google-objc-forbidden-subclassing to [clang-tidy ObjC] [3/3] New check objc-forbidden-subclassing.Oct 23 2017, 9:43 AM
benhamilton edited the summary of this revision. (Show Details)
benhamilton marked 8 inline comments as done.

@hokein: Fixed all those, thanks!

Creating an object-c module seems fine, but I wonder what if we create a google-specific object-c check in the future, which module should we put the check to?

It looks like those would be fine under the google module.

clang-tidy/google/ObjcForbiddenSubclassingCheck.h
11 ↗(On Diff #119703)

Oops, as you can see I renamed this check a few times.. :)

39 ↗(On Diff #119703)

Good catch, thank you!

I was surprised that my tests still passed without overriding storeOptions, but now I see that method just improves the UI to explain the options which are valid for the check.

docs/clang-tidy/checks/google-objc-forbidden-subclassing.rst
10 ↗(On Diff #119703)

The links are in several places. Some are in SDK docs, others are in headers.

I did make a list of URLs to the references in the SDK docs, but it is very long, and Apple tends to break SDK doc URLs every year when they release a new SDK, so I decided it wasn't worth including.

hokein added inline comments.Oct 24 2017, 1:41 AM
clang-tidy/google/ObjcForbiddenSubclassingCheck.h
39 ↗(On Diff #119703)

Yeah, the lint test doesn't cover storeOptions. This method is used when dumping the configuration. You can try clang-tidy -dump-config.

clang-tidy/objc/ForbiddenSubclassingCheck.cpp
73 ↗(On Diff #119878)

I think we can also do the filtering stuff in the AST matcher. We need to implement a isSubclassOf AST matcher for ObjcInterfaceDecl, the code is roughly like:

AST_MATCHER_P(ObjCInterfaceDecl, isSubClassof,
              ast_matchers::internal::Matcher<ObjCInterfaceDecl>, InnerMatcher) {
  const CXXRecordDecl *Parent = Node.getParent();
 for (const auto*SuperClass = Node->getSuperClass(); SuperClass; SuperClass = SuperClass->getSuperClass) {
   if (InnerMatcher.matches(*SuperClass, Finder, Builder))
      return true;
  }
  return false;
}

And the final matcher would be Finder->addMatcher(objcInterfaceDecl(isSubclassOf(objcInterfaceDecl(hasAnyName(std::vector<StringRef>(ForbiddenSuperclassNames.begin(), ForbiddenSuperclassNames.end())))));.

clang-tidy/objc/ForbiddenSubclassingCheck.h
40 ↗(On Diff #119878)

Sorry for missing this in the previous review.

Using a std::vector for the forbidden classes is sufficient -- most of clang-tidy checks are using std::vector for options, and with that, you can get rid of RawStringForbiddenSuperclassNames (just use utils::options::serializeStringList to store the option).

docs/clang-tidy/checks/google-objc-forbidden-subclassing.rst
10 ↗(On Diff #119703)

ok...:(

@hokein:

  • Introduce isSubclassOf() AST matcher
  • Use std::vector<std::string> to hold forbidden subclass names
benhamilton added inline comments.Oct 24 2017, 9:12 AM
clang-tidy/objc/ForbiddenSubclassingCheck.cpp
73 ↗(On Diff #119878)

OK, done! Easier now with ForbiddenSuperclassNames being a vector.

clang-tidy/objc/ForbiddenSubclassingCheck.h
40 ↗(On Diff #119878)

OK. I figured a set made logically more sense since options cannot be duplicated, but the infrastructure doesn't make that trivial, so changed to vector.

benhamilton edited the summary of this revision. (Show Details)Oct 24 2017, 9:27 AM
hokein accepted this revision.Oct 25 2017, 1:11 AM

LGTM with a few nits.

clang-tidy/objc/ForbiddenSubclassingCheck.cpp
1 ↗(On Diff #120081)

nit: This line should align with the last line of this block

95 ↗(On Diff #120081)

I'd add an assert(SubClass), the same to the SuperClass.

docs/clang-tidy/checks/objc-forbidden-subclassing.rst
4 ↗(On Diff #120081)

nit: remove the extra =.

22 ↗(On Diff #120081)

maybe name it ForbiddenSubClassNames? ClassNames is a bit ambiguous.

This revision is now accepted and ready to land.Oct 25 2017, 1:11 AM
benhamilton marked 4 inline comments as done.
clang-tidy/objc/ForbiddenSubclassingCheck.cpp
1 ↗(On Diff #120081)

Fixed here and in the header.

docs/clang-tidy/checks/objc-forbidden-subclassing.rst
22 ↗(On Diff #120081)

I made it ForbiddenSuperClassNames (since it is the superclass which we are listing here).

@alexfh: We'd love your thoughts on this diff!

This revision was automatically updated to reflect the committed changes.

@alexfh is on vacation from today. I think it is fine to submit the patch, committed for you.