[analyzer] Fix false negative when pass implicit cast nil to nonnull

[tripleCC]

Summary

We should look through implicit casts before determining the type of the arguments, and only allow explicit cast to _Nonnull to suppress warning

void foo(NSString *_Nonnull);

foo((NSString * _Nonnull)nil); // no-warning
id obj = nil; 
foo(obj); // should warning here (implicit cast id to NSString *_Nonnull)

[tripleCC]

This comment was removed by tripleCC.

[steakhal]

The tests are failing on main.
Check the CI bots if they pass on your machine.

[tripleCC]

try to fix nullability-arc.mm test case error

[tripleCC]

clang/test/Analysis/nullability-arc.mm

26

I'm not sure if this change fixes the issue in D54017 , and may require some further discussion

[tripleCC]

clang/test/Analysis/nullability-arc.mm

25

I think there should be a warning when we call the foo: method

[steakhal]

@tripleCC Could you clarify the status. Do you need some help or review? Do you have concerns?
You made some inline comments that I couldn't put into context.

[tripleCC]

In D154221#4472112, @steakhal wrote:

@tripleCC Could you clarify the status. Do you need some help or review? Do you have concerns?
You made some inline comments that I couldn't put into context.

@steakhal Yes, I need some review, especially concerning the modifications made to file nullability-arc.mm.

If @NoQ could join in the review, that would be even better , as he created this file in D54017.

[steakhal]

I'd say, as tests pass and I and @xazax.hun think that this is an improvement - along with you - then we should merge this as-is and do corrections once reported.

clang/test/Analysis/nullability-arc.mm

25

I'm probably a bit naive, but it feels like passing nil to a function that expects _Nonnull seems like a precondition violation no matter if objc-arc is enabled or not. TBH I'm not even sure how that comes into play in this context in the first place.

clang/lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp

if (!isValidPointerType(Param->getType()) &&
         !Param->getType()->isReferenceType())
       continue;
 
     NullConstraint Nullness = getNullConstraint(*ArgSVal, State);
 
     Nullability RequiredNullability =
         getNullabilityAnnotation(Param->getType());
     Nullability ArgExprTypeLevelNullability =
-        getNullabilityAnnotation(ArgExpr->getType());
+        getNullabilityAnnotation(lookThroughImplicitCasts(ArgExpr)->getType());
 
     unsigned ParamIdx = Param->getFunctionScopeIndex() + 1;
 
     if (ChecksEnabled[CK_NullPassedToNonnull] &&
         Nullness == NullConstraint::IsNull &&
         ArgExprTypeLevelNullability != Nullability::Nonnull &&
         RequiredNullability == Nullability::Nonnull &&
         isDiagnosableCall(Call)) {

clang/test/Analysis/nullability-arc.mm

 // RUN: %clang_analyze_cc1 -w -analyzer-checker=core,nullability\
 // RUN:                       -analyzer-output=text -verify %s
 // RUN: %clang_analyze_cc1 -w -analyzer-checker=core,nullability\
 // RUN:                       -analyzer-output=text -verify %s -fobjc-arc
 
-#if !__has_feature(objc_arc)
-// expected-no-diagnostics
-#endif
 
 
 #define nil ((id)0)
 
 @interface Param
 @end
 
 @interface Base
 - (void)foo:(Param *_Nonnull)param;
 @end
 
 @interface Derived : Base
 @end
 
 @implementation Derived
 - (void)foo:(Param *)param {
   // FIXME: Why do we not emit the warning under ARC?
   [super foo:param];
-#if __has_feature(objc_arc)
-  // expected-warning@-2{{nil passed to a callee that requires a non-null 1st parameter}}
-  // expected-note@-3   {{nil passed to a callee that requires a non-null 1st parameter}}
-#endif
 
   [self foo:nil];
-#if __has_feature(objc_arc)
-  // expected-note@-2{{Calling 'foo:'}}
-  // expected-note@-3{{Passing nil object reference via 1st parameter 'param'}}
-#endif
+  // expected-warning@-1{{nil passed to a callee that requires a non-null 1st parameter}}
+  // expected-note@-2   {{nil passed to a callee that requires a non-null 1st parameter}}
 }
 @end
 

clang/test/Analysis/nullability.mm

 int getRandom();
 
 typedef struct Dummy { int val; } Dummy;
 
 void takesNullable(Dummy *_Nullable);
 void takesNonnull(Dummy *_Nonnull);
 void takesUnspecified(Dummy *);
 void takesNonnullBlock(void (^ _Nonnull)(void));
+void takesNonnullObject(NSObject *_Nonnull);
 
 Dummy *_Nullable returnsNullable();
 Dummy *_Nonnull returnsNonnull();
 Dummy *returnsUnspecified();
 int *_Nullable returnsNullableInt();
 
 template <typename T> T *eraseNullab(T *p) { return p; }
 
Dummy * _Nonnull testIndirectCastNullableToNonnull() {
   return p;         // no-warning
 }
 
 Dummy * _Nonnull testDirectCastNilToNonnull() {
   takesNonnull((Dummy * _Nonnull)0);  // no-warning
   return (Dummy * _Nonnull)0;         // no-warning
 }
 
+void testImplicitCastNilToNonnull() {
+  id obj = nil;
+  takesNonnullObject(obj); // expected-warning {{nil passed to a callee that requires a non-null 1st parameter}}
+}
+
+void testImplicitCastNullableArgToNonnull(TestObject *_Nullable obj) {
+  if (!obj) {
+    takesNonnullObject(obj); // expected-warning {{nil passed to a callee that requires a non-null 1st parameter}}
+  }
+}
+
 void testIndirectCastNilToNonnullAndPass() {
   Dummy *p = (Dummy * _Nonnull)0;
   // FIXME: Ideally the cast above would suppress this warning.
   takesNonnull(p);  // expected-warning {{Null passed to a callee that requires a non-null 1st parameter}}
 }
 
 void testDirectCastNilToNonnullAndAssignToLocalInInitializer() {
   Dummy * _Nonnull nonnullLocalWithAssignmentInInitializer = (Dummy * _Nonnull)0; // no-warning