Page MenuHomePhabricator

[clang-tidy] Add a check for [super self] in initializers πŸ”
ClosedPublic

Authored by stephanemoore on Mar 25 2019, 6:19 PM.

Details

Summary

This check aims to address a relatively common benign error where
Objective-C subclass initializers call -self on their superclass instead
of invoking a superclass initializer, typically -init. The error is
typically benign because libobjc recognizes that improper initializer
chaining is commonΒΉ.

One theory for the frequency of this error might be that -init and -self
have the same return type which could potentially cause inappropriate
autocompletion to -self instead of -init. The equal selector lengths and
triviality of common initializer code probably contribute to errors like
this slipping through code review undetected.

This check aims to flag errors of this form in the interests of
correctness and reduce incidence of initialization failing to chain to
-[NSObject init].

[1] "In practice, it will be hard to rely on this function. Many classes do not properly chain -init calls."
From _objc_rootInit in https://opensource.apple.com/source/objc4/objc4-750.1/runtime/NSObject.mm.auto.html.

Test Notes:
Verified via make check-clang-tools.

Diff Detail

Repository
rL LLVM

Event Timeline

stephanemoore created this revision.Mar 25 2019, 6:19 PM
Herald added a project: Restricted Project. Β· View Herald TranscriptMar 25 2019, 6:19 PM
stephanemoore edited the summary of this revision. (Show Details)Mar 25 2019, 6:20 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp
61 β†—(On Diff #192235)

Please don't use auto where type is not obvious.

clang-tools-extra/docs/ReleaseNotes.rst
110 β†—(On Diff #192235)

Please enclose NSObject in ``. Probably same for -self if this is language construct.

clang-tools-extra/docs/clang-tidy/checks/objc-super-self.rst
6 β†—(On Diff #192235)

Please synchronize with Release Notes.

aaron.ballman added inline comments.Mar 26 2019, 11:52 AM
clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp
57 β†—(On Diff #192235)

This matcher seems like it would be generally useful -- would you mind adding it to the AST matcher interface rather than local to this check? It doesn't have to be done as part of this patch (we can leave the matcher here for the moment).

110 β†—(On Diff #192235)

Is there a %0 missing from the diagnostic for this method declaration you're passing in?

Removed usage of auto with a nonobvious type in isSubclassOf matcher.
Added backticks around NSObject in docs.
Synchronized check description in release notes and check documentation.

stephanemoore marked 5 inline comments as done.

Fix diagnostic format string to actually use the message's method declaration.

stephanemoore marked an inline comment as done.

Enclose -self in backticks in release notes and check documentation.

Enclose -self in backticks in check documentation (overlooked in previous attempt).

stephanemoore added inline comments.Mar 26 2019, 12:23 PM
clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp
57 β†—(On Diff #192235)

Definitely agreed. I will send out a followup for a new AST matcher.

110 β†—(On Diff #192235)

Good catch! That's egg on my face πŸ˜…πŸ₯š

I think I was internally conflicted over specifically mentioning -[NSObject self] versus using the method declaration of the message expression and somehow got stuck halfway in-between πŸ˜“ I think it's better to mention the method declaration of the message. Let me know if you think that I should instead mention -[NSObject self].

clang-tools-extra/docs/ReleaseNotes.rst
110 β†—(On Diff #192235)

Good catch. Done.

Eugene.Zelenko added inline comments.Mar 26 2019, 1:00 PM
clang-tools-extra/docs/ReleaseNotes.rst
113 β†—(On Diff #192317)

Please use double backticks for language constructs. Same in other places.

Use double backticks rather than single backticks for symbols in documentation.

stephanemoore marked 2 inline comments as done.Mar 26 2019, 3:19 PM
stephanemoore added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
113 β†—(On Diff #192317)

Sorry I misread your earlier comment. I have been editing too much Markdown recently πŸ˜… I have updated to use double backticks.

stephanemoore marked 2 inline comments as done.Mar 26 2019, 3:32 PM
stephanemoore added inline comments.
clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp
110 β†—(On Diff #192235)

Whoops; I forgot to update the message in the test.

Update tests to match updated diagnostic.

I think this basically LGTM, but I'd appreciate hearing from someone more well-versed in ObjC before landing this. My primary question is: are there situations where [super self] is sensible (if so, how should the user silence the diagnostic; will NOLINT suffice or be too annoying), and are there any other circumstances where the fix-it is dangerous that we should be concerned by?

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp
112 β†—(On Diff #192383)

This could be dangerous if the [super self] construct is in a macro, couldn't it? e.g.,

#define DERP self

[super DERP];

I don't think there's ever a reason to call [super self], and doing so through a macro could easily indicate a bug.

Diagnostic nitpick: the Objective-C term is "init method", not "initializer".

I don't think there's ever a reason to call [super self], and doing so through a macro could easily indicate a bug.

Agreed.

The only relatively common usage of -[NSObject self] that I am aware of is through KVC, e.g., to construct a set of distinct objects using array operators:

NSArray *uniqueObjects = [array valueForKeyPath:@"@distinctUnionOfObjects.self"]

I don't recall observing -[NSObject self] used in other situations.

Diagnostic nitpick: the Objective-C term is "init method", not "initializer".

Good catch. Glancing through the project, I have the impression that there is some inconsistency in the terminology used by different diagnostics and documentation in the project. For example, the ARC documentation on method families and various tests use the term "init methods". In contrast, -Wobjc-designated-initializers uses the term "initializer". The general pattern seems to be that documentation/diagnostics related to ARC use the term "init method" and diagnostics/documentation related to designated initializers use the term "initializer". I had assumed that "initializer" would be more intuitive to readers as this is the term typically used in Apple developer documentation, e.g., Concepts in Objective-C Programming > Object Initialization.

I believe that "initializer" will be more intuitive because of Apple documentation but I can change to "init method" if that is preferred. Please let me know what your preference is.

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp
112 β†—(On Diff #192383)

Good point. Let me add some test cases and make sure this is handled properly.

Ooh, I should have remembered "designated initializer". I guess it doesn't matter so much either way.

Add test cases with [super self] expanded from macros.

stephanemoore marked 3 inline comments as done.Mar 29 2019, 4:19 PM
stephanemoore added inline comments.
clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp
112 β†—(On Diff #192383)

Added some test cases where [super self] is expanded from macros.

I don't think there's ever a reason to call [super self], and doing so through a macro could easily indicate a bug.

Agreed.

The only relatively common usage of -[NSObject self] that I am aware of is through KVC, e.g., to construct a set of distinct objects using array operators:

It's used idiomatically in ARC and GC as a way of preventing an object from being deallocated before a certain point, since the compiler cannot eliminate the call to self. But that shouldn't affect this warning.

I don't think there's ever a reason to call [super self], and doing so through a macro could easily indicate a bug.

Thank you for the verification! And agreed about the macro thing -- I am only worried about trying to issue a fix-it in that case, not the diagnostic itself.

clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp
112 β†—(On Diff #192383)

You missed the test case I was worried about -- where the macro is mixed into the expression. I don't think we want to try to add a fix-it in that case.

clang-tools-extra/test/clang-tidy/objc-super-self.m
41 β†—(On Diff #192938)

Are you missing a CHECK-FIXES here?

Personally, I don't think we should try to generate a fixit for this case, so I would expect a CHECK-FIXES that ensures this doesn't get modified.

stephanemoore marked an inline comment as done.

Add a test case where a macro emits just self in the message expression.

stephanemoore added inline comments.Apr 2 2019, 12:24 PM
clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp
112 β†—(On Diff #192383)

Added a test case though at the moment it generates a fixit.

Before I investigate modifying the check to avoid generating a fixit in this case, I think it would be helpful for me to understand your concerns in that scenario better. Is your concern that a proper fix might involve fixing the macro itself rather than the message expression?

#if FLAG
#define INVOCATION self
#else
#define INVOCATION init
#endif

- (instancetype)init {
  return [super INVOCATION];
}
clang-tools-extra/test/clang-tidy/objc-super-self.m
41 β†—(On Diff #192938)

No fix is currently generated for this case which is why there is no CHECK-FIXES. I also agree that no fix should be generated for this case.

I must confess that I have yet to fully understand why the fix for this case is discarded (though I am grateful for the behavior). Let me dig around to try to better understand why no fixit is generated for this case and assess adding a condition for emitting the fixit.

aaron.ballman added inline comments.Apr 3 2019, 5:14 AM
clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp
112 β†—(On Diff #192383)

Before I investigate modifying the check to avoid generating a fixit in this case, I think it would be helpful for me to understand your concerns in that scenario better. Is your concern that a proper fix might involve fixing the macro itself rather than the message expression?

Essentially, yes. Macros can get arbitrarily complex and so our rule of thumb is to not apply fix-its when the code being fixed is within a macro. Another example that can be tricky is adding another layer of macros:

#define FOO super
#define BAR self
#define BAZ FOO BAR

- (instancetype)init {
  return [BAZ];
}
clang-tools-extra/test/clang-tidy/objc-super-self.m
41 β†—(On Diff #192938)

Our typical way to check that a fix is not applied is to use // CHECK-FIXES: <original code> to test that the fix was not applied.

stephanemoore planned changes to this revision.Apr 4 2019, 5:54 PM
stephanemoore marked an inline comment as done.
stephanemoore added inline comments.
clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp
112 β†—(On Diff #192383)

Thanks for the explanation! I'll get started making the changes πŸ‘

Update check to avoid emitting a fix if the expression is in a macro.

stephanemoore marked 3 inline comments as done.

Add CHECK-FIXES to verify code is preserved for scenarios that should not have fixes.

stephanemoore marked 4 inline comments as done.

Check if either the receiver or selector are in macro locations.

Fix some formatting issues.

stephanemoore added inline comments.Apr 15 2019, 5:17 PM
clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp
57 β†—(On Diff #192235)
clang-tools-extra/test/clang-tidy/objc-super-self.m
41 β†—(On Diff #192938)

Gotcha. Added CHECK-FIXES to verify original code is preserved.

aaron.ballman accepted this revision.Apr 16 2019, 1:33 PM

LGTM! You can either land this now and refactor after the AST matcher lands, or you can wait until the AST matcher lands and land this patch after -- your call.

This revision is now accepted and ready to land.Apr 16 2019, 1:33 PM

LGTM! You can either land this now and refactor after the AST matcher lands, or you can wait until the AST matcher lands and land this patch after -- your call.

I will plan on landing this and then follow up after the new AST matcher lands. I want to make sure the design for the new AST matcher is solid and evaluate the implications of the different approaches. I plan on updating both ForbiddenSubclassingCheck and SuperSelfCheck once the new AST matcher lands πŸ‘

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. Β· View Herald TranscriptApr 17 2019, 3:27 PM
Herald added a subscriber: llvm-commits. Β· View Herald Transcript