Review 1

* Add handling for `!= nullptr`, `!= ptr` operators
* Re-add TK_IgnoreUnlessSpelledInSource
* Refactor handling for null-check expressions (transfer instead of branchTransfer)
* Fix crash from `&(PrVar)` not always being PrVar
* Fix formatting and docs

Author: Discookie

clang-tools-extra/docs/ReleaseNotes.rst

@@ -130,6 +130,12 @@ New checks
 
   Finds nondeterministic usages of pointers in unordered containers.
 
+- New :doc:`bugprone-null-check-after-dereference
+  <clang-tidy/checks/bugprone/null-check-after-dereference>` check.
+
+  This check identifies redundant pointer null-checks, by finding cases where
+  the pointer cannot be null at the location of the null-check.
+
 - New :doc:`bugprone-tagged-union-member-count
   <clang-tidy/checks/bugprone/tagged-union-member-count>` check.
 
@@ -148,12 +154,6 @@ New checks
   Finds cases when an uninstantiated virtual member function in a template class
   causes cross-compiler incompatibility.
 
-- New :doc:`bugprone-null-check-after-dereference
-  <clang-tidy/checks/bugprone/null-check-after-dereference>` check.
-
-  Finds locations where a pointer's nullability is checked after it has already
-  been dereferenced.
-
 New check aliases
 ^^^^^^^^^^^^^^^^^
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst

@@ -35,7 +35,8 @@ Pointer null-checks
 
 The check currently supports null-checks on pointers that use
 ``operator bool``, such as when being used as the condition
-for an ``if`` statement.
+for an ``if`` statement. It also supports comparisons such as ``!= nullptr``, and
+``== other_ptr``.
 
 .. code-block:: c++
 

clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp

@@ -43,10 +43,44 @@ void no_warning(int *p, bool b) {
   }
 
   if (p) {
+    // no-warning
     *p += 20;
   }
 }
 
+void equals_nullptr(int *p) {
+  *p = 42;
+
+  if (p == nullptr) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+    // CHECK-MESSAGES: :[[@LINE-4]]:3: note: one of the locations where the pointer's value cannot be null
+    return;
+  }
+
+  if (p != nullptr) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+    // CHECK-MESSAGES: :[[@LINE-10]]:3: note: one of the locations where the pointer's value cannot be null
+    *p += 20;
+  }
+
+  if (nullptr != p) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: pointer value is checked even though it cannot be null at this point
+    // CHECK-MESSAGES: :[[@LINE-16]]:3: note: one of the locations where the pointer's value cannot be null
+    *p += 20;
+  }
+}
+
+void equals_other_ptr(int *p, int *q) {
+  if (q)
+    return;
+
+  if (p == q) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: pointer value is checked but it can only be null at this point
+    // CHECK-MESSAGES: :[[@LINE-5]]:7: note: one of the locations where the pointer's value can only be null  
+    return;
+  }
+}
+
 int else_branch_warning(int *p, bool b) {
   if (b) {
     *p = 42;
@@ -105,6 +139,7 @@ int nullptr_assignment(int *nullptr_param, bool b) {
   }
 
   if (nullptr_assigned) {
+    // no-warning
     return *nullptr_assigned;
   } else {
     return 0;
@@ -197,9 +232,8 @@ int chained_if(int *a) {
     return 0;
   }
 
-  // FIXME: Negations are not tracked properly when the previous conditional returns
   if (a) {
-    // --CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
     *a += 20;
     return *a;
   } else {
@@ -212,8 +246,7 @@ int double_if(int *a) {
     if (a) {
       // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: pointer value is checked even though it cannot be null at this point
       // --CHECK-MESSAGES: :[[@LINE-3]]:5: note: one of the locations where the pointer's value cannot be null
-      // FIXME: Add warning for branch statements where pointer is not null afterwards
-      *a += 20;
+      // FIXME: Add warning for branch satements where pointer is not null afterwards
       return *a;
     } else {
       return 0;

clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h

@@ -83,8 +83,8 @@ class NullCheckAfterDereferenceDiagnoser {
     const Environment &Env;
   };
 
-  /// Checked when known to be null, and checked after already dereferenced,
-  /// respectively.
+  /// Returns source locations for pointers that were checked when known to be
+  // null, and checked after already dereferenced, respectively.
   using ResultType =
       std::pair<std::vector<SourceLocation>, std::vector<SourceLocation>>;
 

clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp

@@ -64,7 +64,8 @@ using SR = SatisfiabilityResult;
 // FIXME: These AST matchers should also be exported via the
 // NullPointerAnalysisModel class, for tests
 auto ptrWithBinding(llvm::StringRef VarName = kVar) {
-  return expr(hasType(isAnyPointer())).bind(VarName);
+  return traverse(TK_IgnoreUnlessSpelledInSource,
+      expr(hasType(isAnyPointer())).bind(VarName));
 }
 
 auto derefMatcher() {
@@ -81,8 +82,8 @@ auto castExprMatcher() {
       .bind(kCond);
 }
 
-auto nullptrMatcher() {
-  return castExpr(hasCastKind(CK_NullToPointer)).bind(kVar);
+auto nullptrMatcher(llvm::StringRef VarName = kVar) {
+  return castExpr(hasCastKind(CK_NullToPointer)).bind(VarName);
 }
 
 auto addressofMatcher() {
@@ -99,6 +100,19 @@ auto assignMatcher() {
                          hasRHS(expr().bind(kValue)));
 }
 
+auto nullCheckExprMatcher() {
+  return binaryOperator(hasAnyOperatorName("==", "!="),
+                        hasOperands(ptrWithBinding(), nullptrMatcher(kValue)));
+}
+
+// FIXME: When TK_IgnoreUnlessSpelledInSource is removed from ptrWithBinding(),
+// this matcher should be merged with nullCheckExprMatcher().
+auto equalExprMatcher() {
+  return binaryOperator(hasAnyOperatorName("==", "!="),
+                        hasOperands(ptrWithBinding(kVar),
+                                    ptrWithBinding(kValue)));
+}
+
 auto anyPointerMatcher() { return expr(hasType(isAnyPointer())).bind(kVar); }
 
 // Only computes simple comparisons against the literals True and False in the
@@ -217,34 +231,72 @@ void matchDereferenceExpr(const Stmt *stmt,
   Env.assume(Env.arena().makeNot(getVal(kIsNull, *RootValue).formula()));
 }
 
-void matchCastExpr(const CastExpr *cond, const MatchFinder::MatchResult &Result,
-                   NullPointerAnalysisModel::TransferArgs &Data) {
-  auto [IsNonnullBranch, Env] = Data;
-
+void matchNullCheckExpr(const Expr *NullCheck,
+                    const MatchFinder::MatchResult &Result,
+                    Environment &Env) {
   const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
   assert(Var != nullptr);
 
-  Value *RootValue = getValue(*Var, Env);
-
-  Value *NewRootValue = Env.createValue(Var->getType());
+  // (bool)p or (p != nullptr)
+  bool IsNonnullOp = true;
+  if (auto *BinOp = dyn_cast<BinaryOperator>(NullCheck);
+      BinOp->getOpcode() == BO_EQ) {
+    IsNonnullOp = false;
+  }
 
-  setGLValue(*Var, *NewRootValue, Env);
+  Value *RootValue = getValue(*Var, Env);
 
   Arena &A = Env.arena();
   BoolValue &IsNonnull = getVal(kIsNonnull, *RootValue);
   BoolValue &IsNull = getVal(kIsNull, *RootValue);
+  BoolValue *CondValue = cast_or_null<BoolValue>(Env.getValue(*NullCheck));
+  if (!CondValue) {
+    CondValue = &A.makeAtomValue();
+    Env.setValue(*NullCheck, *CondValue);
+  }
+  const Formula &CondFormula = IsNonnullOp ? CondValue->formula()
+                                             : A.makeNot(CondValue->formula());
+
+  Env.assume(A.makeImplies(CondFormula, A.makeNot(IsNull.formula())));
+  Env.assume(A.makeImplies(A.makeNot(CondFormula),
+                           A.makeNot(IsNonnull.formula())));
+}
+
+void matchEqualExpr(const BinaryOperator *EqualExpr,
+                    const MatchFinder::MatchResult &Result,
+                    Environment &Env) {
+  bool IsNotEqualsOp = EqualExpr->getOpcode() == BO_NE;
+
+  const auto *LHSVar = Result.Nodes.getNodeAs<Expr>(kVar);
+  assert(LHSVar != nullptr);
 
-  if (IsNonnullBranch) {
-    Env.assume(A.makeNot(IsNull.formula()));
-    NewRootValue->setProperty(kIsNull, Env.getBoolLiteralValue(false));
+  const auto *RHSVar = Result.Nodes.getNodeAs<Expr>(kValue);
+  assert(RHSVar != nullptr);
 
-    NewRootValue->setProperty(kIsNonnull, IsNonnull);
-  } else {
-    NewRootValue->setProperty(kIsNull, IsNull);
+  Arena &A = Env.arena();
+  Value *LHSValue = getValue(*LHSVar, Env);
+  Value *RHSValue = getValue(*RHSVar, Env);
 
-    Env.assume(A.makeNot(IsNonnull.formula()));
-    NewRootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(false));
+  BoolValue *CondValue = cast_or_null<BoolValue>(Env.getValue(*EqualExpr));
+  if (!CondValue) {
+    CondValue = &A.makeAtomValue();
+    Env.setValue(*EqualExpr, *CondValue);
   }
+
+  const Formula &CondFormula = IsNotEqualsOp ? A.makeNot(CondValue->formula())
+                                       : CondValue->formula();
+
+  // If the pointers are equal, the nullability properties are the same.
+  Env.assume(A.makeImplies(CondFormula, 
+      A.makeAnd(A.makeEquals(getVal(kIsNull, *LHSValue).formula(),
+                             getVal(kIsNull, *RHSValue).formula()),
+                A.makeEquals(getVal(kIsNonnull, *LHSValue).formula(),
+                             getVal(kIsNonnull, *RHSValue).formula()))));
+
+  // If the pointers are not equal, at most one of the pointers is null.
+  Env.assume(A.makeImplies(A.makeNot(CondFormula),
+      A.makeNot(A.makeAnd(getVal(kIsNull, *LHSValue).formula(),
+                          getVal(kIsNull, *RHSValue).formula()))));
 }
 
 void matchNullptrExpr(const Expr *expr, const MatchFinder::MatchResult &Result,
@@ -255,24 +307,31 @@ void matchNullptrExpr(const Expr *expr, const MatchFinder::MatchResult &Result,
   Value *RootValue = Env.getValue(*PrVar);
   if (!RootValue) {
     RootValue = Env.createValue(PrVar->getType());
+    Env.setValue(*PrVar, *RootValue);
   }
 
   RootValue->setProperty(kIsNull, Env.getBoolLiteralValue(true));
   RootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(false));
-  Env.setValue(*PrVar, *RootValue);
 }
 
 void matchAddressofExpr(const Expr *expr,
                         const MatchFinder::MatchResult &Result,
                         Environment &Env) {
-  const auto *PrVar = Result.Nodes.getNodeAs<Expr>(kVar);
-  assert(PrVar != nullptr);
+  const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
+  assert(Var != nullptr);
+
+  Value *RootValue = Env.getValue(*Var);
+  if (!RootValue) {
+    RootValue = Env.createValue(Var->getType());
 
-  Value *RootValue = Env.createValue(PrVar->getType());
+    if (!RootValue)
+      return;
+
+    setUnknownValue(*Var, *RootValue, Env);
+  }
 
   RootValue->setProperty(kIsNull, Env.getBoolLiteralValue(false));
   RootValue->setProperty(kIsNonnull, Env.getBoolLiteralValue(true));
-  Env.setValue(*PrVar, *RootValue);
 }
 
 void matchAnyPointerExpr(const Expr *fncall,
@@ -336,9 +395,9 @@ diagnoseAssignLocation(const Expr *Assign,
 }
 
 NullCheckAfterDereferenceDiagnoser::ResultType
-diagnoseCastExpr(const CastExpr *Stmt, const MatchFinder::MatchResult &Result,
-                 const NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) {
-
+diagnoseNullCheckExpr(const Expr *NullCheck,
+      const MatchFinder::MatchResult &Result,
+      const NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) {
   auto [ValToDerefLoc, WarningLocToVal, Env] = Data;
 
   const auto *Var = Result.Nodes.getNodeAs<Expr>(kVar);
@@ -352,6 +411,7 @@ diagnoseCastExpr(const CastExpr *Stmt, const MatchFinder::MatchResult &Result,
       bool IsNonnull = Env.allows(getVal(kIsNonnull, *RootValue).formula());
 
       if (!IsNull && IsNonnull) {
+        // FIXME: Separate function
         bool Inserted =
             WarningLocToVal.try_emplace(Var->getBeginLoc(), RootValue).second;
         assert(Inserted && "multiple warnings at the same source location");
@@ -370,16 +430,44 @@ diagnoseCastExpr(const CastExpr *Stmt, const MatchFinder::MatchResult &Result,
       }
     }
 
-    // If no matches are found, the cast itself signals a special location
+    // If no matches are found, the null-check itself signals a special location
     auto [It, Inserted] = ValToDerefLoc.try_emplace(RootValue, nullptr);
 
     if (Inserted)
-      It->second = Stmt;
+      It->second = NullCheck;
   }
 
   return {};
 }
 
+NullCheckAfterDereferenceDiagnoser::ResultType
+diagnoseEqualExpr(const Expr *PtrCheck, const MatchFinder::MatchResult &Result,
+                  NullCheckAfterDereferenceDiagnoser::DiagnoseArgs &Data) {
+  auto [ValToDerefLoc, WarningLocToVal, Env] = Data;
+
+  const auto *LHSVar = Result.Nodes.getNodeAs<Expr>(kVar);
+  assert(LHSVar != nullptr);
+  const auto *RHSVar = Result.Nodes.getNodeAs<Expr>(kValue);
+  assert(RHSVar != nullptr);
+  
+  Arena &A = Env.arena();
+  std::vector<SourceLocation> NullVarLocations;
+
+  if (Value *LHSValue = Env.getValue(*LHSVar);
+      Env.proves(A.makeNot(getVal(kIsNonnull, *LHSValue).formula()))) {
+    WarningLocToVal.try_emplace(LHSVar->getBeginLoc(), LHSValue);
+    NullVarLocations.push_back(LHSVar->getBeginLoc());
+  }
+
+  if (Value *RHSValue = Env.getValue(*RHSVar);
+      Env.proves(A.makeNot(getVal(kIsNonnull, *RHSValue).formula()))) {
+    WarningLocToVal.try_emplace(RHSVar->getBeginLoc(), RHSValue);
+    NullVarLocations.push_back(RHSVar->getBeginLoc());
+  }
+
+  return {NullVarLocations, {}};
+}
+
 auto buildTransferMatchSwitch() {
   return CFGMatchSwitchBuilder<Environment>()
       .CaseOfCFGStmt<Stmt>(derefMatcher(), matchDereferenceExpr)
@@ -388,12 +476,16 @@ auto buildTransferMatchSwitch() {
       .CaseOfCFGStmt<Expr>(addressofMatcher(), matchAddressofExpr)
       .CaseOfCFGStmt<Expr>(functionCallMatcher(), matchAnyPointerExpr)
       .CaseOfCFGStmt<Expr>(anyPointerMatcher(), matchAnyPointerExpr)
+      .CaseOfCFGStmt<Expr>(castExprMatcher(), matchNullCheckExpr)
+      .CaseOfCFGStmt<Expr>(nullCheckExprMatcher(), matchNullCheckExpr)
+      .CaseOfCFGStmt<BinaryOperator>(equalExprMatcher(), matchEqualExpr)
       .Build();
 }
 
 auto buildBranchTransferMatchSwitch() {
   return ASTMatchSwitchBuilder<Stmt, NullPointerAnalysisModel::TransferArgs>()
-      .CaseOf<CastExpr>(castExprMatcher(), matchCastExpr)
+      // .CaseOf<CastExpr>(castExprMatcher(), matchNullCheckExpr)
+      // .CaseOf<BinaryOperator>(equalExprMatcher(), matchEqualExpr)
       .Build();
 }
 
@@ -403,7 +495,9 @@ auto buildDiagnoseMatchSwitch() {
       .CaseOfCFGStmt<Expr>(derefMatcher(), diagnoseDerefLocation)
       .CaseOfCFGStmt<Expr>(arrowMatcher(), diagnoseDerefLocation)
       .CaseOfCFGStmt<Expr>(assignMatcher(), diagnoseAssignLocation)
-      .CaseOfCFGStmt<CastExpr>(castExprMatcher(), diagnoseCastExpr)
+      .CaseOfCFGStmt<Expr>(castExprMatcher(), diagnoseNullCheckExpr)
+      .CaseOfCFGStmt<Expr>(nullCheckExprMatcher(), diagnoseNullCheckExpr)
+      .CaseOfCFGStmt<Expr>(equalExprMatcher(), diagnoseEqualExpr)
       .Build();
 }