[clang][dataflow] Don't clear cached field state if field is const

[jvoung]

... in the unchecked optional access model.

[llvmbot]

@llvm/pr-subscribers-clang-analysis

Author: Jan Voung (jvoung)

Changes

... in the unchecked optional access model.

---

Full diff: https://github.com/llvm/llvm-project/pull/113698.diff

2 Files Affected:

• (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+6-2)
• (modified) clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp (+22-1)

diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index b0bd8274405d02..31ae2b94f5b617 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -601,10 +601,14 @@ void handleNonConstMemberCall(const CallExpr *CE,
                               dataflow::RecordStorageLocation *RecordLoc,
                               const MatchFinder::MatchResult &Result,
                               LatticeTransferState &State) {
-  // When a non-const member function is called, reset some state.
   if (RecordLoc != nullptr) {
+    // When a non-const member function is called, clear all (non-const)
+    // optional fields of the receiver. Const-qualified fields can't be
+    // changed (at least, not without UB).
     for (const auto &[Field, FieldLoc] : RecordLoc->children()) {
-      if (isSupportedOptionalType(Field->getType())) {
+      QualType FieldType = Field->getType();
+      if (!FieldType.isConstQualified() &&
+          isSupportedOptionalType(Field->getType())) {
         auto *FieldRecordLoc = cast_or_null<RecordStorageLocation>(FieldLoc);
         if (FieldRecordLoc) {
           setHasValue(*FieldRecordLoc, State.Env.makeAtomicBoolValue(),
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 22fe347c425593..5b64eaca0e10d3 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2167,7 +2167,7 @@ TEST_P(UncheckedOptionalAccessTest, OptionalReturnedFromFuntionCall) {
   )");
 }
 
-TEST_P(UncheckedOptionalAccessTest, OptionalFieldModified) {
+TEST_P(UncheckedOptionalAccessTest, NonConstMethodMayClearOptionalField) {
   ExpectDiagnosticsFor(
       R"(
     #include "unchecked_optional_access_test.h"
@@ -2187,6 +2187,27 @@ TEST_P(UncheckedOptionalAccessTest, OptionalFieldModified) {
   )");
 }
 
+TEST_P(UncheckedOptionalAccessTest,
+       NonConstMethodMayNotClearConstOptionalField) {
+  ExpectDiagnosticsFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    struct Foo {
+      const $ns::$optional<std::string> opt;
+      void clear();
+    };
+
+    void target(Foo& foo) {
+      if (foo.opt) {
+        foo.opt.value();
+        foo.clear();
+        foo.opt.value();
+      }
+    }
+  )");
+}
+
 TEST_P(UncheckedOptionalAccessTest, StdSwap) {
   ExpectDiagnosticsFor(
       R"(

[llvmbot]

@llvm/pr-subscribers-clang

Author: Jan Voung (jvoung)

Changes

... in the unchecked optional access model.

---

Full diff: https://github.com/llvm/llvm-project/pull/113698.diff

2 Files Affected:

• (modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+6-2)
• (modified) clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp (+22-1)

diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index b0bd8274405d02..31ae2b94f5b617 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -601,10 +601,14 @@ void handleNonConstMemberCall(const CallExpr *CE,
                               dataflow::RecordStorageLocation *RecordLoc,
                               const MatchFinder::MatchResult &Result,
                               LatticeTransferState &State) {
-  // When a non-const member function is called, reset some state.
   if (RecordLoc != nullptr) {
+    // When a non-const member function is called, clear all (non-const)
+    // optional fields of the receiver. Const-qualified fields can't be
+    // changed (at least, not without UB).
     for (const auto &[Field, FieldLoc] : RecordLoc->children()) {
-      if (isSupportedOptionalType(Field->getType())) {
+      QualType FieldType = Field->getType();
+      if (!FieldType.isConstQualified() &&
+          isSupportedOptionalType(Field->getType())) {
         auto *FieldRecordLoc = cast_or_null<RecordStorageLocation>(FieldLoc);
         if (FieldRecordLoc) {
           setHasValue(*FieldRecordLoc, State.Env.makeAtomicBoolValue(),
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 22fe347c425593..5b64eaca0e10d3 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -2167,7 +2167,7 @@ TEST_P(UncheckedOptionalAccessTest, OptionalReturnedFromFuntionCall) {
   )");
 }
 
-TEST_P(UncheckedOptionalAccessTest, OptionalFieldModified) {
+TEST_P(UncheckedOptionalAccessTest, NonConstMethodMayClearOptionalField) {
   ExpectDiagnosticsFor(
       R"(
     #include "unchecked_optional_access_test.h"
@@ -2187,6 +2187,27 @@ TEST_P(UncheckedOptionalAccessTest, OptionalFieldModified) {
   )");
 }
 
+TEST_P(UncheckedOptionalAccessTest,
+       NonConstMethodMayNotClearConstOptionalField) {
+  ExpectDiagnosticsFor(
+      R"(
+    #include "unchecked_optional_access_test.h"
+
+    struct Foo {
+      const $ns::$optional<std::string> opt;
+      void clear();
+    };
+
+    void target(Foo& foo) {
+      if (foo.opt) {
+        foo.opt.value();
+        foo.clear();
+        foo.opt.value();
+      }
+    }
+  )");
+}
+
 TEST_P(UncheckedOptionalAccessTest, StdSwap) {
   ExpectDiagnosticsFor(
       R"(