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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Check for Objective-C block pointers as well, which also require __unsafe_unretained when retrieved as arguments from NSInvocation under ARC.
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. |
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. |
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. |
Renamed NsinvocationArgumentLifetimeCheck -> NSInvocationArgumentLifetimeCheck to match other ObjC checker names.
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? |
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.
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). |
Adding non-object types for argument to getArgument:atIndex: to verify they don't cause problems.
Done while investigating Harbormaster failures.
Add check for isScalarType before getting the type, which seems to be triggering an assert. Maybe this is a Mac vs Linux thing?
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. |
I think the files should be renamed to NSInvocationArgumentLifetimeCheck based on typical naming conventions for NS identifiers.