[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 marked an inline comment as done.
stephanemoore added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/objc/NSInvocationArgumentLifetimeCheck.cpp:49
+          QT->getScalarTypeKind() == Type::STK_BlockPointer) &&
+         QT.getQualifiers().getObjCLifetime() > Qualifiers::OCL_ExplicitNone;
+}
----------------
I'm not sure but I guess it's okay to assume that the enumeration ordering will remain stable?


================
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]
+}
----------------
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
```


================
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]
+}
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77571