Page MenuHomePhabricator

[clang-tidy] Add check to find calls to NSInvocation methods under ARC that don't have proper object argument lifetimes.
ClosedPublic

Authored by mwyman on Apr 6 2020, 10:35 AM.

Details

Summary

This check is similar to an ARC Migration check that warned about this incorrect usage under ARC, but most projects are no longer undergoing migration from pre-ARC code. The documentation for NSInvocation is not explicit about these requirements and incorrect usage has been found in many of our projects.

Diff Detail

Event Timeline

mwyman created this revision.Apr 6 2020, 10:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 10:35 AM
mwyman added a project: Restricted Project.
mwyman edited the summary of this revision. (Show Details)Apr 6 2020, 10:39 AM
mwyman updated this revision to Diff 255423.Apr 6 2020, 11:27 AM

Check for Objective-C block pointers as well, which also require __unsafe_unretained when retrieved as arguments from NSInvocation under ARC.

benhamilton accepted this revision.Apr 6 2020, 12:21 PM
benhamilton added inline comments.
clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.cpp
71–73 ↗(On Diff #255423)

Is there a bug filed for this? Seems like an oversight.

116 ↗(On Diff #255423)

-[NSInvocation %0] should

This revision is now accepted and ready to land.Apr 6 2020, 12:21 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.cpp
35 ↗(On Diff #255423)

Please use static instead of anonymous namespace for variable.

50 ↗(On Diff #255423)

Please use static instead of anonymous namespace for functions.

83 ↗(On Diff #255423)

Please don't use auto unless type is spelled in same statement or iterator. Please elide braces. Same in other places.

clang-tools-extra/docs/ReleaseNotes.rst
140

Please enclose NSInvocation in double back-ticks.

clang-tools-extra/docs/clang-tidy/checks/objc-nsinvocation-argument-lifetime.rst
7

Please enclose NSInvocation in double back-ticks. Same below.

Eugene.Zelenko retitled this revision from Add ClangTidy check to find calls to NSInvocation methods under ARC that don't have proper object argument lifetimes. to [clang-tidy] Add check to find calls to NSInvocation methods under ARC that don't have proper object argument lifetimes..
mwyman updated this revision to Diff 255461.Apr 6 2020, 1:14 PM
mwyman marked 6 inline comments as done.

Responding to review feedback.

mwyman marked an inline comment as done.Apr 6 2020, 1:16 PM
mwyman added inline comments.
clang-tools-extra/clang-tidy/objc/NsinvocationArgumentLifetimeCheck.cpp
71–73 ↗(On Diff #255423)

Maybe I'm incorrectly reading this, but per the comment on QualifiedTypeLoc it seems like this is intentional at this time: https://github.com/llvm/llvm-project/blob/549e87f3d04bbae91bc7bc38609ce7073e2c8a6d/clang/include/clang/AST/TypeLoc.h#L275

116 ↗(On Diff #255423)

Tweaked the format string so it now outputs "NSInvocation '-…'", since the diagnostic formatter inserts single-quotes, which would otherwise make it read "-[NSInvocation 'getArgument:atIndex:']", which seems weirder.

mwyman marked 2 inline comments as done.Apr 6 2020, 1:16 PM
mwyman updated this revision to Diff 255513.Apr 6 2020, 3:21 PM

Trying to fix Harbormaster build.

mwyman updated this revision to Diff 255519.Apr 6 2020, 3:36 PM

Missed CHECK-FIXES for block argument.

Harbormaster completed remote builds in B52063: Diff 255519.
aaron.ballman accepted this revision.Apr 8 2020, 7:13 AM

LGTM aside from the file names concern.

clang-tools-extra/clang-tidy/objc/CMakeLists.txt
11

I think the files should be renamed to NSInvocationArgumentLifetimeCheck based on typical naming conventions for NS identifiers.

mwyman updated this revision to Diff 256070.Apr 8 2020, 10:49 AM

Renamed NsinvocationArgumentLifetimeCheck -> NSInvocationArgumentLifetimeCheck to match other ObjC checker names.

mwyman marked an inline comment as done.Apr 8 2020, 10:51 AM
stephanemoore accepted this revision.Apr 8 2020, 5:33 PM
stephanemoore marked an inline comment as done.
stephanemoore added inline comments.
clang-tools-extra/clang-tidy/objc/NSInvocationArgumentLifetimeCheck.cpp
50

I'm not sure but I guess it's okay to assume that the enumeration ordering will remain stable?

clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m
65–70

How about a test case where we take the address of something on an ivar?

For example, I think the code below should not produce a diagnostic finding, right?

struct Example {
  __unsafe_unretained id Field;
};

@interface TestClass : NSObject {
  struct Example ExampleIvar;
}

@end

@implementation TestClass

- (void)processInvocation:(NSInvocation *)Invocation {
  [Invocation getArgument:&(self->ExampleIvar.Field) atIndex:2];
}

@end
65–70

Maybe worth adding an ivar case that doesn't produce a diagnostic?

mwyman updated this revision to Diff 256209.Apr 9 2020, 1:11 AM
mwyman marked an inline comment as done.

Updating per Stephane's comments. To deal with struct fields needed to match MemberRefExprs.

However, in doing so I discovered that the match conditions I thought were catching ObjcIvarRefExpr or MemberRefExpr were sometimes incorrectly matching on the self reference, which is itself a DeclRefExpr to an object with strong lifetime semantics. This meant correct usages would sometimes be caught, because the ObjcIvarRefExpr wouldn't match but the DeclRefExpr (to self) did.

That self (or other object) that is dereferenced (implicity or explicitly) has a parent ImplicitCastExpr in the AST, so I've dealt with this situation by excluding that case from matching the declRefExpr.

mwyman updated this revision to Diff 256210.Apr 9 2020, 1:16 AM

Add matching MemberRefExpr case, not just the non-matching case there was before.

mwyman marked 4 inline comments as done.Apr 9 2020, 1:20 AM
mwyman added inline comments.
clang-tools-extra/clang-tidy/objc/NSInvocationArgumentLifetimeCheck.cpp
50

I don't know why anyone would change the ordering, but FWIW this is the same approach used by the ARC migrator code.

clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m
65–70

This required checking MemberRefExprs too. Added both a matching and non-matching case here.

65–70

Added a non-matching ivar case, and in doing so discovered it was often actually matching the "self" reference, which is an ImplicitParamDecl, itself a type of VarDecl, which matched the declRefExpr(), leading to even cases that shouldn't have produced diagnostics to produce them.

Have fixed this by excluding the self case (by ensuring the declRefExpr's parent is not an implicitCastExpr, which only appears in the AST that I've seen around the self—or other dereferenced object—reference).

mwyman updated this revision to Diff 256332.Apr 9 2020, 10:28 AM
mwyman marked 2 inline comments as done.

Adding non-object types for argument to getArgument:atIndex: to verify they don't cause problems.

Done while investigating Harbormaster failures.

mwyman updated this revision to Diff 256344.Apr 9 2020, 11:35 AM

Add check for isScalarType before getting the type, which seems to be triggering an assert. Maybe this is a Mac vs Linux thing?

benhamilton accepted this revision.Apr 9 2020, 2:19 PM
stephanemoore accepted this revision.Apr 9 2020, 8:59 PM
stephanemoore added inline comments.
clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m
65–70

It might be good to also have a case using the address of a field of a struct local variable.

mwyman updated this revision to Diff 256521.Apr 10 2020, 12:49 AM

Added struct member reference from local variable, per review comment.

mwyman marked an inline comment as done.Apr 10 2020, 12:50 AM
This revision was automatically updated to reflect the committed changes.