[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 ↗

(On Diff #42139)

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 ↗

(On Diff #42139)

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

7682 ↗

(On Diff #42139)

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 ↗

(On Diff #42139)

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 ↗

(On Diff #42139)

Neat. Thanks!

7682 ↗

(On Diff #42139)

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 ↗

(On Diff #42139)

Separated into two lines, like above.

[aaron.ballman]

Thank you for this, LGTM!

lib/Sema/SemaChecking.cpp

7682 ↗

(On Diff #42204)

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.

cfe/trunk/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">,
+def warn_nonnull_expr_compare : Warning<
+    "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 "

cfe/trunk/include/clang/Sema/ScopeInfo.h

public:
 
   /// \brief A list of PartialDiagnostics created but delayed within the
   /// current function scope.  These diagnostics are vetted for reachability
   /// prior to being emitted.
   SmallVector<PossiblyUnreachableDiag, 4> PossiblyUnreachableDiags;
   
   /// \brief A list of parameters which have the nonnull attribute and are
   /// modified in the function.
-  llvm::SmallPtrSet<const ParmVarDecl*, 8>  ModifiedNonNullParams;
+  llvm::SmallPtrSet<const ParmVarDecl*, 8> ModifiedNonNullParams;
 
 public:
   /// Represents a simple identification of a weak object.
   ///
   /// Part of the implementation of -Wrepeated-use-of-weak.
   ///
   /// This is used to determine if two weak accesses refer to the same object.
   /// Here are some examples of how various accesses are "profiled":

cfe/trunk/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_expr_compare
+                                : diag::warn_cast_nonnull_to_bool;
+    Diag(E->getExprLoc(), DiagID) << 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>()) {
+        ComplainAboutNonnullParamOrCall(false);
+        return;
+      }
+    }
+  }
+
   // 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 (getCurFunction() && !getCurFunction()->ModifiedNonNullParams.count(PV))
-      if (const FunctionDecl* FD = dyn_cast<FunctionDecl>(PV->getDeclContext())) {
-        unsigned NumArgs = FD->getNumParams();
-        llvm::SmallBitVector AttrNonNull(NumArgs);
+  if (const auto* PV = dyn_cast<ParmVarDecl>(D)) {
+    if (getCurFunction() &&
+        !getCurFunction()->ModifiedNonNullParams.count(PV)) {
+      if (PV->hasAttr<NonNullAttr>()) {
+        ComplainAboutNonnullParamOrCall(true);
+        return;
+      }
+
+      if (const auto *FD = dyn_cast<FunctionDecl>(PV->getDeclContext())) {
+        auto ParamIter = std::find(FD->param_begin(), FD->param_end(), PV);
+        assert(ParamIter != FD->param_end());
+        unsigned ParamNo = std::distance(FD->param_begin(), ParamIter);
+
         for (const auto *NonNull : FD->specific_attrs<NonNullAttr>()) {
           if (!NonNull->args_size()) {
-            AttrNonNull.set(0, NumArgs);
-            break;
+              ComplainAboutNonnullParamOrCall(true);
+              return;
+          }
+
+          for (unsigned ArgNo : NonNull->args()) {
+            if (ArgNo == ParamNo) {
+              ComplainAboutNonnullParamOrCall(true);
+              return;
             }
-          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;
-            }
-      }
-    }
-  
   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;
   }

cfe/trunk/test/Sema/nonnull.c

void redecl_test(void *p) {
   redecl(0, p); // expected-warning{{null passed}}
 }
 
 // rdar://18712242
 #define NULL (void*)0
 __attribute__((__nonnull__))
 int evil_nonnull_func(int* pointer, void * pv)
 {
-   if (pointer == NULL) {  // expected-warning {{comparison of nonnull parameter 'pointer' equal to a null pointer is false on first encounter}}
+   if (pointer == NULL) {  // expected-warning {{comparison of nonnull parameter 'pointer' equal to a null pointer is 'false' on first encounter}}
      return 0;
    } else {
      return *pointer;
    } 
 
    pointer = pv;
    if (!pointer)
      return 0;
    else
      return *pointer;
 
-   if (pv == NULL) {} // expected-warning {{comparison of nonnull parameter 'pv' equal to a null pointer is false on first encounter}}
+   if (pv == NULL) {} // expected-warning {{comparison of nonnull parameter 'pv' equal to a null pointer is 'false' on first encounter}}
 }
 
 void set_param_to_null(int**);
 int another_evil_nonnull_func(int* pointer, char ch, void * pv) __attribute__((nonnull(1, 3)));
 int another_evil_nonnull_func(int* pointer, char ch, void * pv) {
-   if (pointer == NULL) { // expected-warning {{comparison of nonnull parameter 'pointer' equal to a null pointer is false on first encounter}}
+   if (pointer == NULL) { // expected-warning {{comparison of nonnull parameter 'pointer' equal to a null pointer is 'false' on first encounter}}
      return 0;
    } else {
      return *pointer;
    } 
 
    set_param_to_null(&pointer);
    if (!pointer)
      return 0;
    else
      return *pointer;
 
-   if (pv == NULL) {} // expected-warning {{comparison of nonnull parameter 'pv' equal to a null pointer is false on first encounter}}
+   if (pv == NULL) {} // expected-warning {{comparison of nonnull parameter 'pv' equal to a null pointer is 'false' on first encounter}}
 }
 
 extern void *returns_null(void**);
 extern void FOO();
 extern void FEE();
 
 extern void *pv;
 __attribute__((__nonnull__))
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_warning_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}}
+}