Reverse-null surface-level fixes

Discookie · Discookie · commit ba2f02d6fc94 · 2024-12-12T08:30:19.000Z
diff --git a/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NullCheckAfterDereferenceCheck.cpp
@@ -66,18 +66,18 @@ analyzeFunction(const FunctionDecl &FuncDecl) {
   NullCheckAfterDereferenceDiagnoser Diagnoser;
   NullCheckAfterDereferenceDiagnoser::ResultType Diagnostics;
 
-  using LatticeState = DataflowAnalysisState<NullPointerAnalysisModel::Lattice>;
-  using DetailMaybeLatticeStates = std::vector<std::optional<LatticeState>>;
+  using State = DataflowAnalysisState<NullPointerAnalysisModel::Lattice>;
+  using DetailMaybeStates = std::vector<std::optional<State>>;
 
   auto DiagnoserImpl = [&ASTCtx, &Diagnoser,
                         &Diagnostics](const CFGElement &Elt,
-                                      const LatticeState &S) mutable -> void {
+                                      const State &S) mutable -> void {
     auto EltDiagnostics = Diagnoser.diagnose(ASTCtx, &Elt, S.Env);
     llvm::move(EltDiagnostics.first, std::back_inserter(Diagnostics.first));
     llvm::move(EltDiagnostics.second, std::back_inserter(Diagnostics.second));
   };
 
-  Expected<DetailMaybeLatticeStates> BlockToOutputState =
+  Expected<DetailMaybeStates> BlockToOutputState =
       dataflow::runDataflowAnalysis(*Context, Analysis, Env, DiagnoserImpl);
 
   if (llvm::Error E = BlockToOutputState.takeError()) {
@@ -116,16 +116,16 @@ analyzeFunction(const FunctionDecl &FuncDecl) {
 void NullCheckAfterDereferenceCheck::registerMatchers(MatchFinder *Finder) {
   using namespace ast_matchers;
 
-  auto hasPointerValue =
+  auto containsPointerValue =
       hasDescendant(NullPointerAnalysisModel::ptrValueMatcher());
   Finder->addMatcher(
       decl(anyOf(functionDecl(unless(isExpansionInSystemHeader()),
                               // FIXME: Remove the filter below when lambdas are
                               // well supported by the check.
                               unless(hasDeclContext(cxxRecordDecl(isLambda()))),
-                              hasBody(hasPointerValue)),
+                              hasBody(containsPointerValue)),
                  cxxConstructorDecl(hasAnyConstructorInitializer(
-                     withInitializer(hasPointerValue)))))
+                     withInitializer(containsPointerValue)))))
           .bind(FuncID),
       this);
 }
@@ -141,10 +141,10 @@ void NullCheckAfterDereferenceCheck::check(
     return;
 
   if (const auto Diagnostics = analyzeFunction(*FuncDecl)) {
-    const auto &[CheckWhenNullLocations, CheckAfterDereferenceLocations] =
+    const auto &[CheckWhenNullLocations, CheckWhenNonnullLocations] =
         *Diagnostics;
 
-    for (const auto [WarningLoc, DerefLoc] : CheckAfterDereferenceLocations) {
+    for (const auto [WarningLoc, DerefLoc] : CheckWhenNonnullLocations) {
       diag(WarningLoc, "pointer value is checked even though "
                        "it cannot be null at this point");
 
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -148,6 +148,12 @@ 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
 ^^^^^^^^^^^^^^^^^
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/null-check-after-dereference.rst
@@ -7,7 +7,7 @@ bugprone-null-check-after-dereference
 
    This check uses a flow-sensitive static analysis to produce its
    results. Therefore, it may be more resource intensive (RAM, CPU) than the
-   average clang-tidy check.
+   average Clang-tidy check.
 
 This check identifies redundant pointer null-checks, by finding cases where the
 pointer cannot be null at the location of the null-check.
@@ -33,9 +33,9 @@ Supported pointer operations
 Pointer null-checks
 -------------------
 
-The checker currently supports null-checks on pointers that use
+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.
 
 .. code-block:: c++
 
@@ -69,8 +69,8 @@ Pointer star- and arrow-dereferences are supported.
 Null-pointer and other value assignments
 ----------------------------------------
 
-The checker supports assigning various values to pointers, making them *null*
-or *non-null*. The checker also supports passing pointers of a pointer to
+The check supports assigning various values to pointers, making them *null*
+or *non-null*. The check also supports passing pointers of a pointer to
 external functions.
 
 .. code-block:: c++
@@ -103,7 +103,7 @@ Limitations
 
 The check only supports C++ due to limitations in the data-flow framework.
 
-The annotations ``_nullable`` and ``_nonnull`` are not supported.
+The annotations ``_Nullable`` and ``_Nonnull`` are not supported.
 
 .. code-block:: c++
 
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/null-check-after-dereference.cpp
@@ -70,7 +70,6 @@ int else_branch_warning(int *p, bool b) {
     // CHECK-MESSAGES: :[[@LINE-7]]:5: note: one of the locations where the pointer's value cannot be null
     return 0;
   } else {
-    *p += 20;
     return *p;
   }
 }
@@ -89,39 +88,17 @@ int two_branches_warning(int *p, bool b) {
     // CHECK-MESSAGES: :[[@LINE-9]]:5: note: one of the locations where the pointer's value cannot be null
     return 0;
   } else {
-    *p += 20;
     return *p;
   }
 }
 
-int two_branches_reversed(int *p, bool b) {
-  if (!b) {
-    *p = 42;
-  }
-  
-  if (b) {
-    *p = 20;
-  }
-
-  if (p) {
-    // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
-    // CHECK-MESSAGES: :[[@LINE-9]]:5: note: one of the locations where the pointer's value cannot be null
-    return 0;
-  } else {
-    *p += 20;
-    return *p;
-  }
-}
-
-
 int regular_assignment(int *p, int *q) {
   *p = 42;
   q = p;
 
   if (q) {
     // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
     // CHECK-MESSAGES: :[[@LINE-5]]:3: note: one of the locations where the pointer's value cannot be null
-    *p += 20; 
     return *p;
   } else {
     return 0;
@@ -139,29 +116,26 @@ int nullptr_assignment(int *nullptr_param, bool b) {
   }
 
   if (nullptr_assigned) {
-    // CHECK-MESSAGES-NOT: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
-    *nullptr_assigned = 20;
     return *nullptr_assigned;
   } else {
     return 0;
   }
 }
 
-extern int *fncall();
-extern void refresh_ref(int *&ptr);
-extern void refresh_ptr(int **ptr);
+extern int *external_fn();
+extern void ref_fn(int *&ptr);
+extern void ptr_fn(int **ptr);
 
 int fncall_reassignment(int *fncall_reassigned) {
   *fncall_reassigned = 42;
 
-  fncall_reassigned = fncall();
+  fncall_reassigned = external_fn();
 
   if (fncall_reassigned) {
-    // CHECK-MESSAGES-NOT: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
     *fncall_reassigned = 42;
   }
   
-  fncall_reassigned = fncall();
+  fncall_reassigned = external_fn();
 
   *fncall_reassigned = 42;
 
@@ -171,19 +145,33 @@ int fncall_reassignment(int *fncall_reassigned) {
     *fncall_reassigned = 42;
   }
   
-  refresh_ptr(&fncall_reassigned);
+  ptr_fn(&fncall_reassigned);
 
   if (fncall_reassigned) {
-    // CHECK-MESSAGES-NOT: :[[@LINE-1]]:7: warning: pointer value is checked even though it cannot be null at this point
+    // FIXME: References of a pointer passed to external functions do not invalidate its value
+    // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked even though it cannot be null at this point
+    // CHECK-MESSAGES: :[[@LINE-8]]:5: note: one of the locations where the pointer's value cannot be null
+    *fncall_reassigned = 42;
+  }
+
+  *fncall_reassigned = 42;
+
+  ref_fn(fncall_reassigned);
+
+  if (fncall_reassigned) {
+    // FIXME: References of a pointer passed to external functions do not invalidate its value
+    // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked even though it cannot be null at this point
+    // CHECK-MESSAGES: :[[@LINE-19]]:5: note: one of the locations where the pointer's value cannot be null
     *fncall_reassigned = 42;
   }
   
-  refresh_ptr(&fncall_reassigned);
+  ptr_fn(&fncall_reassigned);
   *fncall_reassigned = 42;
 
   if (fncall_reassigned) {
     // 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
+    // FIXME: Better note tag support, preferably after the reassignment/refresh
+    // CHECK-MESSAGES: :[[@LINE-29]]:5: note: one of the locations where the pointer's value cannot be null
     *fncall_reassigned = 42;
     return *fncall_reassigned;
   } else {
@@ -294,33 +282,6 @@ int cxx17_crash(int *p) {
   return 0;
 }
 
-void external_by_ref(int *&p);
-void external_by_ptr(int **p);
-
-int external_invalidates() {
-  int *p = nullptr;
-
-  external_by_ref(p);
-
-  if (p) {
-    // FIXME: References of a pointer passed to external functions do not invalidate its value
-    // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked but it can only be null at this point
-    return *p;
-  }
-
-  p = nullptr;
-
-  external_by_ptr(&p);
-
-  if (p) {
-    // FIXME: References of a pointer passed to external functions do not invalidate its value
-    // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: pointer value is checked but it can only be null at this point
-    return *p;
-  } else {
-    return 0;
-  }
-}
-
 int note_tags() {
   // FIXME: Note tags are not appended to declarations
   int *ptr = nullptr;
diff --git a/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h b/clang/include/clang/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.h
@@ -58,11 +58,6 @@ class NullPointerAnalysisModel
 
   static ast_matchers::StatementMatcher ptrValueMatcher();
 
-  // Used to initialize the storage locations of function arguments.
-  // Required to merge these values within the environment.
-  void initializeFunctionParameters(const ControlFlowContext &CFCtx,
-                                    Environment &Env);
-
   void transfer(const CFGElement &E, NoopLattice &State, Environment &Env);
 
   void transferBranch(bool Branch, const Stmt *E, NoopLattice &State,
@@ -88,6 +83,8 @@ class NullCheckAfterDereferenceDiagnoser {
     const Environment &Env;
   };
 
+  /// Checked when known to be null, and checked after already dereferenced,
+  /// respectively.
   using ResultType =
       std::pair<std::vector<SourceLocation>, std::vector<SourceLocation>>;
 
diff --git a/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/NullPointerAnalysisModel.cpp
