[Sema] Emit warnings when comparing result of a function with `returns_nonnull` to null

[george.burgess.iv]

Summary

Addresses a problem brought up by Xavier here: http://permalink.gmane.org/gmane.comp.compilers.clang.devel/46163

tl;dr: We get a warning on the following code

void foo(void *p) __attribute__((nonnull(1))) {
	if (p == NULL) {} // warning: always equal to false
}

…But not this code:

void foo() __attribute__((returns_nonnull));
int main() {
	if (foo() == NULL) {} // no warning
}

This patch makes us give a warning in the second case.

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

2747

I know this is existing behavior, but can you quote true and false in this diagnostic, similar to the way true is quoted above?

lib/Sema/SemaChecking.cpp

7674

No need to cast IsParam to int; the diagnostic builder already does the right thing here.

7682

I think this should be:

if (Callee->hasAttr<ReturnsNonNullAttr>() &&
    ComplainAboutNonnullParamOrCall(false))
  return;

Otherwise, we skip out on the rest of the checking in the presence of ReturnsNonNullAttr.

7716

Same here as above. The return should only be on failure.

[george.burgess.iv]

• Addressed all feedback
• Refactored a loop to make its intent more clear

lib/Sema/SemaChecking.cpp

7674

Neat. Thanks!

7682

The function this is in returns void, not bool. I've separated the return from the function call in order to hopefully make things a bit more clear. :)

7716

Separated into two lines, like above.

[aaron.ballman]

Thank you for this, LGTM!

lib/Sema/SemaChecking.cpp

7682

Oops, I didn't notice that the function returns void, sorry about that. I also didn't notice that early return here is the correct thing because we're only checking one expression at a time within this function.

include/clang/Basic/DiagnosticSemaKinds.td

def ext_ms_impcast_fn_obj : ExtWarn<
   "implicit conversion between pointer-to-function and pointer-to-object is a "
   "Microsoft extension">, InGroup<MicrosoftCast>;
 
 def warn_impcast_pointer_to_bool : Warning<
     "address of%select{| function| array}0 '%1' will always evaluate to "
     "'true'">,
     InGroup<PointerBoolConversion>;
 def warn_cast_nonnull_to_bool : Warning<
-    "nonnull parameter '%0' will evaluate to "
+    "nonnull %select{function call|parameter}0 '%1' will evaluate to "
     "'true' on first encounter">,
     InGroup<PointerBoolConversion>;
 def warn_this_bool_conversion : Warning<
   "'this' pointer cannot be null in well-defined C++ code; pointer may be "
   "assumed to always convert to true">, InGroup<UndefinedBoolConversion>;
 def warn_address_of_reference_bool_conversion : Warning<
   "reference cannot be bound to dereferenced null pointer in well-defined C++ "
   "code; pointer may be assumed to always convert to true">,
   InGroup<UndefinedBoolConversion>;
 
 def warn_null_pointer_compare : Warning<
     "comparison of %select{address of|function|array}0 '%1' %select{not |}2"
     "equal to a null pointer is always %select{true|false}2">,
     InGroup<TautologicalPointerCompare>;
 def warn_nonnull_parameter_compare : Warning<
-    "comparison of nonnull parameter '%0' %select{not |}1"
-    "equal to a null pointer is %select{true|false}1 on first encounter">,
+    "comparison of nonnull %select{function call|parameter}0 '%1' "
+    "%select{not |}2equal to a null pointer is %select{true|false}2 on first "
+    "encounter">,
     InGroup<TautologicalPointerCompare>;
 def warn_this_null_compare : Warning<
   "'this' pointer cannot be null in well-defined C++ code; comparison may be "
   "assumed to always evaluate to %select{true|false}0">,
   InGroup<TautologicalUndefinedCompare>;
 def warn_address_of_reference_null_compare : Warning<
   "reference cannot be bound to dereferenced null pointer in well-defined C++ "
   "code; comparison may be assumed to always evaluate to "

lib/Sema/SemaChecking.cpp

if (FSI->FirstDataArg != 0)
       --FSI->FirstDataArg;
   }
   return true;
 }
 
 /// Checks if a the given expression evaluates to null.
 ///
 /// \brief Returns true if the value evaluates to null.
-static bool CheckNonNullExpr(Sema &S,
-                             const Expr *Expr) {
+static bool CheckNonNullExpr(Sema &S, const Expr *Expr) {
   // If the expression has non-null type, it doesn't evaluate to null.
   if (auto nullability
         = Expr->IgnoreImplicit()->getType()->getNullability(S.Context)) {
     if (*nullability == NullabilityKind::NonNull)
       return false;
   }
 
   // As a special case, transparent unions initialized with zero are
unsigned DiagID = IsCompare
                           : diag::warn_address_of_reference_bool_conversion;
     PartialDiagnostic PD = PDiag(DiagID) << E->getSourceRange() << Range
                                          << IsEqual;
     if (CheckForReference(*this, E, PD)) {
       return;
     }
   }
 
+  auto ComplainAboutNonnullParamOrCall = [&](bool IsParam) {
+    std::string Str;
+    llvm::raw_string_ostream S(Str);
+    E->printPretty(S, nullptr, getPrintingPolicy());
+    unsigned DiagID = IsCompare ? diag::warn_nonnull_parameter_compare
+                                : diag::warn_cast_nonnull_to_bool;
+    Diag(E->getExprLoc(), DiagID) << int(IsParam) << S.str()
+      << E->getSourceRange() << Range << IsEqual;
+  };
+
+  // If we have a CallExpr that is tagged with returns_nonnull, we can complain.
+  if (auto *Call = dyn_cast<CallExpr>(E->IgnoreParenImpCasts()))
+    if (auto *Callee = Call->getDirectCallee())
+      if (Callee->hasAttr<ReturnsNonNullAttr>())
+        return ComplainAboutNonnullParamOrCall(false);
+
   // Expect to find a single Decl.  Skip anything more complicated.
   ValueDecl *D = nullptr;
   if (DeclRefExpr *R = dyn_cast<DeclRefExpr>(E)) {
     D = R->getDecl();
   } else if (MemberExpr *M = dyn_cast<MemberExpr>(E)) {
     D = M->getMemberDecl();
   }
 
   // Weak Decls can be null.
   if (!D || D->isWeak())
     return;
   
   // Check for parameter decl with nonnull attribute
-  if (const ParmVarDecl* PV = dyn_cast<ParmVarDecl>(D)) {
+  if (const auto *PV = dyn_cast<ParmVarDecl>(D)) {
     if (getCurFunction() && !getCurFunction()->ModifiedNonNullParams.count(PV))
-      if (const FunctionDecl* FD = dyn_cast<FunctionDecl>(PV->getDeclContext())) {
+      if (const auto *FD = dyn_cast<FunctionDecl>(PV->getDeclContext())) {
         unsigned NumArgs = FD->getNumParams();
         llvm::SmallBitVector AttrNonNull(NumArgs);
         for (const auto *NonNull : FD->specific_attrs<NonNullAttr>()) {
           if (!NonNull->args_size()) {
             AttrNonNull.set(0, NumArgs);
             break;
           }
           for (unsigned Val : NonNull->args()) {
             if (Val >= NumArgs)
               continue;
             AttrNonNull.set(Val);
           }
         }
-        if (!AttrNonNull.empty())
-          for (unsigned i = 0; i < NumArgs; ++i)
-            if (FD->getParamDecl(i) == PV &&
-                (AttrNonNull[i] || PV->hasAttr<NonNullAttr>())) {
-              std::string Str;
-              llvm::raw_string_ostream S(Str);
-              E->printPretty(S, nullptr, getPrintingPolicy());
-              unsigned DiagID = IsCompare ? diag::warn_nonnull_parameter_compare
-                                          : diag::warn_cast_nonnull_to_bool;
-              Diag(E->getExprLoc(), DiagID) << S.str() << E->getSourceRange()
-                << Range << IsEqual;
-              return;
-            }
+              (AttrNonNull[i] || PV->hasAttr<NonNullAttr>()))
+            return ComplainAboutNonnullParamOrCall(true);
       }
-    }
   
   QualType T = D->getType();
   const bool IsArray = T->isArrayType();
   const bool IsFunction = T->isFunctionType();
 
   // Address of function is used to silence the function warning.
   if (IsAddressOf && IsFunction) {
     return;

test/Sema/nonnull.c

if (s) // No warning
     ;
 }
 
 void pr21668_2(__attribute__((nonnull)) const char *p) {
   p = 0;
   if (p) // No warning
     ;
 }
+
+__attribute__((returns_nonnull)) void *returns_nonnull_whee();
+
+void returns_nonnull_tests() {
+  if (returns_nonnull_whee() == NULL) {} // expected-warning {{comparison of nonnull function call 'returns_nonnull_whee()' equal to a null pointer is false on first encounter}}
+
+  if (returns_nonnull_whee() != NULL) {} // expected-warning {{comparison of nonnull function call 'returns_nonnull_whee()' not equal to a null pointer is true on first encounter}}
+
+  if (returns_nonnull_whee()) {} // expected-warning {{nonnull function call 'returns_nonnull_whee()' will evaluate to 'true' on first encounter}}
+  if (!returns_nonnull_whee()) {} // expected-warning {{nonnull function call 'returns_nonnull_whee()' will evaluate to 'true' on first encounter}}
+
+  int and_again = !returns_nonnull_whee(); // expected-warning {{nonnull function call 'returns_nonnull_whee()' will evaluate to 'true' on first encounter}}
+  and_again = !returns_nonnull_whee(); // expected-warning {{nonnull function call 'returns_nonnull_whee()' will evaluate to 'true' on first encounter}}
+}