[analyzer] Fix false negative when accessing a nonnull property from …

[tripleCC]

@interface A : NSObject
@property (nonnull, nonatomic, strong) NSString *name;
+ (nullable instancetype)shared;
@end

@[[A shared].name];

Consider the code above, the nullability of the name property should depend on the result of the shared method. A warning is expected because of adding a nullable object to array.
ObjCMessageExpr gets the actual type through Sema::getMessageSendResultType, instead of using the return type of MethodDecl directly. The final type is generated by considering the nullability of receiver and MethodDecl together.
Thus, the RetType in NullabilityChecker should all be replaced with M.getOriginExpr()->getType().

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Changes

@<!-- -->interface A : NSObject
@<!-- -->property (nonnull, nonatomic, strong) NSString *name;
+ (nullable instancetype)shared;
@<!-- -->end

@[[A shared].name];

Consider the code above, the nullability of the name property should depend on the result of the shared method. A warning is expected because of adding a nullable object to array.
ObjCMessageExpr gets the actual type through Sema::getMessageSendResultType, instead of using the return type of MethodDecl directly. The final type is generated by considering the nullability of receiver and MethodDecl together.
Thus, the RetType in NullabilityChecker should all be replaced with M.getOriginExpr()->getType().

---

Full diff: https://github.com/llvm/llvm-project/pull/67563.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp (+4-1)
• (modified) clang/test/Analysis/nullability.mm (+7)

diff --git a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
index 906f4e85a8e5b5b..462fa16a540c121 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
@@ -899,6 +899,9 @@ void NullabilityChecker::checkPostCall(const CallEvent &Call,
   const NullabilityState *TrackedNullability =
       State->get<NullabilityMap>(Region);
 
+  if (const Expr *E = Call.getOriginExpr())
+    ReturnType = E->getType();
+
   if (!TrackedNullability &&
       getNullabilityAnnotation(ReturnType) == Nullability::Nullable) {
     State = State->set<NullabilityMap>(Region, Nullability::Nullable);
@@ -1053,7 +1056,7 @@ void NullabilityChecker::checkPostObjCMessage(const ObjCMethodCall &M,
   }
 
   // No tracked information. Use static type information for return value.
-  Nullability RetNullability = getNullabilityAnnotation(RetType);
+  Nullability RetNullability = getNullabilityAnnotation(Message->getType());
 
   // Properties might be computed, which means the property value could
   // theoretically change between calls even in commonly-observed cases like
diff --git a/clang/test/Analysis/nullability.mm b/clang/test/Analysis/nullability.mm
index 06bb9912296e32f..d69116d03df7465 100644
--- a/clang/test/Analysis/nullability.mm
+++ b/clang/test/Analysis/nullability.mm
@@ -55,6 +55,7 @@ - (void)takesUnspecified:(int *)p;
 @property(readonly, nullable) void (^propReturnsNullableBlock)(void);
 @property(readonly, nullable) int *propReturnsNullable;
 @property(readonly) int *propReturnsUnspecified;
++ (nullable TestObject *)getNullableObject;
 @end
 
 TestObject * getUnspecifiedTestObject();
@@ -256,6 +257,12 @@ void testObjCPropertyReadNullability() {
   case 8:
     [o takesNonnullBlock:o.propReturnsNullableBlock]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
     break;
+  case 9:
+    [o takesNonnull:getNullableTestObject().propReturnsNonnull]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+    break;
+  case 10:
+    [o takesNonnull:[TestObject getNullableObject].propReturnsNonnull]; // expected-warning {{Nullable pointer is passed to a callee that requires a non-null 1st parameter}}
+    break;
   }
 }
 

[Xazax-hun]

Looks good, thanks! I think a comment about how the types in the AST already has the nullability propagated correctly might be helpful for the future readers, otherwise it looks good to me!

[tripleCC]

Looks good, thanks! I think a comment about how the types in the AST already has the nullability propagated correctly might be helpful for the future readers, otherwise it looks good to me!

Thanks for reviewing, I added a comment for nullability propagated now.