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

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore accepted this revision.
stephanemoore added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m:64-69
+- (void)processInvocation:(NSInvocation *)Invocation {
+  [Invocation getArgument:&Argument atIndex:2];
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation '-getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+  [Invocation getArgument:&self->Argument atIndex:2];
+  // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation '-getArgument:atIndex:' should only pass pointers to objects with ownership __unsafe_unretained [objc-nsinvocation-argument-lifetime]
+}
----------------
mwyman wrote:
> mwyman wrote:
> > stephanemoore wrote:
> > > stephanemoore wrote:
> > > > 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
> > > > ```
> > > Maybe worth adding an ivar case that doesn't produce a diagnostic?
> > This required checking MemberRefExprs too. Added both a matching and non-matching case here.
> 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).
It might be good to also have a case using the address of a field of a struct local variable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77571/new/

https://reviews.llvm.org/D77571