[clang] 85f47fd - [clang][nullability] Improve modeling of `++`/`--` operators. (#96601)

[via cfe-commits]

Author: martinboehme
Date: 2024-06-26T15:03:37+02:00
New Revision: 85f47fdd039549ed7e89b53ca34b0b35456ffe3d

URL: https://github.com/llvm/llvm-project/commit/85f47fdd039549ed7e89b53ca34b0b35456ffe3d
DIFF: https://github.com/llvm/llvm-project/commit/85f47fdd039549ed7e89b53ca34b0b35456ffe3d.diff

LOG: [clang][nullability] Improve modeling of `++`/`--` operators. (#96601)

We definitely know that these operations change the value of their
operand, so
clear out any value associated with it. We don't create a new value,
instead
leaving it to the analysis to do this if desired.

Added: 
    

Modified: 
    clang/lib/Analysis/FlowSensitive/Transfer.cpp
    clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Analysis/FlowSensitive/Transfer.cpp b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
index 8109ac6a781e7..3c896d373a211 100644
--- a/clang/lib/Analysis/FlowSensitive/Transfer.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Transfer.cpp
@@ -391,17 +391,22 @@ class TransferVisitor : public ConstStmtVisitor<TransferVisitor> {
     }
     case UO_PreInc:
     case UO_PreDec:
-      // Propagate the storage location, but don't create a new value; to
-      // avoid generating unnecessary values, we leave it to the specific
-      // analysis to do this if desired.
+      // Propagate the storage location and clear out any value associated with
+      // it (to represent the fact that the value has definitely changed).
+      // To avoid generating unnecessary values, we leave it to the specific
+      // analysis to create a new value if desired.
       propagateStorageLocation(*S->getSubExpr(), *S, Env);
+      if (StorageLocation *Loc = Env.getStorageLocation(*S->getSubExpr()))
+        Env.clearValue(*Loc);
       break;
     case UO_PostInc:
     case UO_PostDec:
-      // Propagate the old value, but don't create a new value; to avoid
-      // generating unnecessary values, we leave it to the specific analysis
-      // to do this if desired.
+      // Propagate the old value, then clear out any value associated with the
+      // storage location (to represent the fact that the value has definitely
+      // changed). See above for rationale.
       propagateValue(*S->getSubExpr(), *S, Env);
+      if (StorageLocation *Loc = Env.getStorageLocation(*S->getSubExpr()))
+        Env.clearValue(*Loc);
       break;
     default:
       break;

diff  --git a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
index 2ce83399c5453..39e7001393e5e 100644
--- a/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
@@ -3832,36 +3832,51 @@ TEST(TransferTest, AddrOfReference) {
 TEST(TransferTest, Preincrement) {
   std::string Code = R"(
     void target(int I) {
+      (void)0; // [[before]]
       int &IRef = ++I;
-      // [[p]]
+      // [[after]]
     }
   )";
   runDataflow(
       Code,
       [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
          ASTContext &ASTCtx) {
-        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+        const Environment &EnvBefore =
+            getEnvironmentAtAnnotation(Results, "before");
+        const Environment &EnvAfter =
+            getEnvironmentAtAnnotation(Results, "after");
 
-        EXPECT_EQ(&getLocForDecl(ASTCtx, Env, "IRef"),
-                  &getLocForDecl(ASTCtx, Env, "I"));
+        EXPECT_EQ(&getLocForDecl(ASTCtx, EnvAfter, "IRef"),
+                  &getLocForDecl(ASTCtx, EnvBefore, "I"));
+
+        const ValueDecl *IDecl = findValueDecl(ASTCtx, "I");
+        EXPECT_NE(EnvBefore.getValue(*IDecl), nullptr);
+        EXPECT_EQ(EnvAfter.getValue(*IDecl), nullptr);
       });
 }
 
 TEST(TransferTest, Postincrement) {
   std::string Code = R"(
     void target(int I) {
+      (void)0; // [[before]]
       int OldVal = I++;
-      // [[p]]
+      // [[after]]
     }
   )";
   runDataflow(
       Code,
       [](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
          ASTContext &ASTCtx) {
-        const Environment &Env = getEnvironmentAtAnnotation(Results, "p");
+        const Environment &EnvBefore =
+            getEnvironmentAtAnnotation(Results, "before");
+        const Environment &EnvAfter =
+            getEnvironmentAtAnnotation(Results, "after");
+
+        EXPECT_EQ(&getValueForDecl(ASTCtx, EnvBefore, "I"),
+                  &getValueForDecl(ASTCtx, EnvAfter, "OldVal"));
 
-        EXPECT_EQ(&getValueForDecl(ASTCtx, Env, "OldVal"),
-                  &getValueForDecl(ASTCtx, Env, "I"));
+        const ValueDecl *IDecl = findValueDecl(ASTCtx, "I");
+        EXPECT_EQ(EnvAfter.getValue(*IDecl), nullptr);
       });
 }