[clang-tidy] [dataflow] Cache reference accessors for `bugprone-unchecked-optional-access` #128437

BaLiKfromUA · 2025-02-23T21:50:21Z

Fixes #126283

Extending #112605 to cache const getters which return references.

Fixes false positives from const reference accessors to object containing optional member

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

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

jvoung

Thanks!

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

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

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

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

BaLiKfromUA · 2025-02-27T00:07:14Z

Addressed all comments. Thanks for the initial feedback!

Regarding documentation: looks like we need to merge #122290 first and then update docs in my PR

llvmbot · 2025-02-27T00:08:06Z

@llvm/pr-subscribers-clang

Author: Valentyn Yukhymenko (BaLiKfromUA)

Changes

Fixes #126283

Extending #112605 to cache const getters which return references.

This should fix false positive cases when we check optional via the chain of const getter calls.

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

3 Files Affected:

(modified) clang-tools-extra/docs/ReleaseNotes.rst (+2)
(modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+16)
(modified) clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp (+194)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3bddeeda06e06..dfa4cb1d47150 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -100,6 +100,8 @@ Changes in existing checks
 - Improved :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` check to allow specifying
   additional C++ member functions to match.
+- Improved :doc:`bugprone-unchecked-optional-access
+  <clang-tidy/checks/bugprone/unchecked-optional-access>` by fixing false positive from const reference accessors to objects containing optional member.
 
 Removed checks
 ^^^^^^^^^^^^^^
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index e1394e28cd49a..9381c5c42e566 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -580,6 +580,22 @@ void handleConstMemberCall(const CallExpr *CE,
     return;
   }
 
+  // Cache if the const method returns a reference
+  if (RecordLoc != nullptr && CE->isGLValue()) {
+    const FunctionDecl *DirectCallee = CE->getDirectCallee();
+    if (DirectCallee == nullptr)
+      return;
+
+    StorageLocation &Loc =
+        State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+            *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
+              // no-op
+            });
+
+    State.Env.setStorageLocation(*CE, Loc);
+    return;
+  }
+
   // Cache if the const method returns a boolean or pointer type.
   // We may decide to cache other return types in the future.
   if (RecordLoc != nullptr &&
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 19c3ff49eab27..5031e17188e17 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -3863,6 +3863,200 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessorWithModInBetween) {
   )cc");
 }
 
+TEST_P(UncheckedOptionalAccessTest,
+       ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObject) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A& getA() const { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      if (b.getA().get().has_value()) {
+        b.getA().get().value();
+      }
+    }
+  )cc");
+}
+
+TEST_P(
+    UncheckedOptionalAccessTest,
+    ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithoutValueCheck) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A& getA() const { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      b.getA().get().value(); // [[unsafe]]
+    }
+  )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest,
+       ConstRefToOptionalSavedAsTemporaryVariable) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A& getA() const { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      const auto& opt = b.getA().get();
+      if (opt.has_value()) {
+        opt.value();
+      }
+    }
+  )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest,
+       ConstRefAccessorToOptionalViaAccessorToHoldingObjectByValue) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A copyA() const { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      if (b.copyA().get().has_value()) {
+        b.copyA().get().value(); // [[unsafe]]
+      }
+    }
+  )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest,
+       ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObject) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      A& getA() { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      if (b.getA().get().has_value()) {
+        b.getA().get().value(); // [[unsafe]]
+      }
+    }
+  )cc");
+}
+
+TEST_P(
+    UncheckedOptionalAccessTest,
+    ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithModAfterCheck) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A& getA() const { return a; }
+
+      A& getA() { return a; }
+
+      void clear() { a = A{}; }
+
+      A a;
+    };
+
+    void target(B& b) {
+      // changing field A via non-const getter after const getter check
+      if (b.getA().get().has_value()) {
+        b.getA() = A{};
+        b.getA().get().value(); // [[unsafe]]
+      }
+
+      // calling non-const method which might change field A
+      if (b.getA().get().has_value()) {
+        b.clear();
+        b.getA().get().value(); // [[unsafe]]
+      }
+    }
+  )cc");
+}
+
+TEST_P(
+    UncheckedOptionalAccessTest,
+    ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithAnotherConstCallAfterCheck) {
+  ExpectDiagnosticsFor(R"cc(
+      #include "unchecked_optional_access_test.h"
+  
+      struct A {
+        const $ns::$optional<int>& get() const { return x; }
+      
+        $ns::$optional<int> x;
+      };
+  
+      struct B {
+        const A& getA() const { return a; }
+  
+        void callWithoutChanges() const { 
+          // no-op 
+        }
+  
+        A a;
+      };
+  
+      void target(B& b) {  
+        if (b.getA().get().has_value()) {
+          b.callWithoutChanges(); // calling const method which cannot change A
+          b.getA().get().value();
+        }
+      }
+    )cc");
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)

llvmbot · 2025-02-27T00:08:06Z

@llvm/pr-subscribers-clang-analysis

Author: Valentyn Yukhymenko (BaLiKfromUA)

Changes

Fixes #126283

Extending #112605 to cache const getters which return references.

This should fix false positive cases when we check optional via the chain of const getter calls.

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

3 Files Affected:

(modified) clang-tools-extra/docs/ReleaseNotes.rst (+2)
(modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+16)
(modified) clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp (+194)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3bddeeda06e06..dfa4cb1d47150 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -100,6 +100,8 @@ Changes in existing checks
 - Improved :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` check to allow specifying
   additional C++ member functions to match.
+- Improved :doc:`bugprone-unchecked-optional-access
+  <clang-tidy/checks/bugprone/unchecked-optional-access>` by fixing false positive from const reference accessors to objects containing optional member.
 
 Removed checks
 ^^^^^^^^^^^^^^
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index e1394e28cd49a..9381c5c42e566 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -580,6 +580,22 @@ void handleConstMemberCall(const CallExpr *CE,
     return;
   }
 
+  // Cache if the const method returns a reference
+  if (RecordLoc != nullptr && CE->isGLValue()) {
+    const FunctionDecl *DirectCallee = CE->getDirectCallee();
+    if (DirectCallee == nullptr)
+      return;
+
+    StorageLocation &Loc =
+        State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+            *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
+              // no-op
+            });
+
+    State.Env.setStorageLocation(*CE, Loc);
+    return;
+  }
+
   // Cache if the const method returns a boolean or pointer type.
   // We may decide to cache other return types in the future.
   if (RecordLoc != nullptr &&
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 19c3ff49eab27..5031e17188e17 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -3863,6 +3863,200 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessorWithModInBetween) {
   )cc");
 }
 
+TEST_P(UncheckedOptionalAccessTest,
+       ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObject) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A& getA() const { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      if (b.getA().get().has_value()) {
+        b.getA().get().value();
+      }
+    }
+  )cc");
+}
+
+TEST_P(
+    UncheckedOptionalAccessTest,
+    ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithoutValueCheck) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A& getA() const { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      b.getA().get().value(); // [[unsafe]]
+    }
+  )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest,
+       ConstRefToOptionalSavedAsTemporaryVariable) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A& getA() const { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      const auto& opt = b.getA().get();
+      if (opt.has_value()) {
+        opt.value();
+      }
+    }
+  )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest,
+       ConstRefAccessorToOptionalViaAccessorToHoldingObjectByValue) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A copyA() const { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      if (b.copyA().get().has_value()) {
+        b.copyA().get().value(); // [[unsafe]]
+      }
+    }
+  )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest,
+       ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObject) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      A& getA() { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      if (b.getA().get().has_value()) {
+        b.getA().get().value(); // [[unsafe]]
+      }
+    }
+  )cc");
+}
+
+TEST_P(
+    UncheckedOptionalAccessTest,
+    ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithModAfterCheck) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A& getA() const { return a; }
+
+      A& getA() { return a; }
+
+      void clear() { a = A{}; }
+
+      A a;
+    };
+
+    void target(B& b) {
+      // changing field A via non-const getter after const getter check
+      if (b.getA().get().has_value()) {
+        b.getA() = A{};
+        b.getA().get().value(); // [[unsafe]]
+      }
+
+      // calling non-const method which might change field A
+      if (b.getA().get().has_value()) {
+        b.clear();
+        b.getA().get().value(); // [[unsafe]]
+      }
+    }
+  )cc");
+}
+
+TEST_P(
+    UncheckedOptionalAccessTest,
+    ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithAnotherConstCallAfterCheck) {
+  ExpectDiagnosticsFor(R"cc(
+      #include "unchecked_optional_access_test.h"
+  
+      struct A {
+        const $ns::$optional<int>& get() const { return x; }
+      
+        $ns::$optional<int> x;
+      };
+  
+      struct B {
+        const A& getA() const { return a; }
+  
+        void callWithoutChanges() const { 
+          // no-op 
+        }
+  
+        A a;
+      };
+  
+      void target(B& b) {  
+        if (b.getA().get().has_value()) {
+          b.callWithoutChanges(); // calling const method which cannot change A
+          b.getA().get().value();
+        }
+      }
+    )cc");
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)

llvmbot · 2025-02-27T00:08:06Z

@llvm/pr-subscribers-clang-tools-extra

Author: Valentyn Yukhymenko (BaLiKfromUA)

Changes

Fixes #126283

Extending #112605 to cache const getters which return references.

This should fix false positive cases when we check optional via the chain of const getter calls.

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

3 Files Affected:

(modified) clang-tools-extra/docs/ReleaseNotes.rst (+2)
(modified) clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+16)
(modified) clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp (+194)

diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 3bddeeda06e06..dfa4cb1d47150 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -100,6 +100,8 @@ Changes in existing checks
 - Improved :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` check to allow specifying
   additional C++ member functions to match.
+- Improved :doc:`bugprone-unchecked-optional-access
+  <clang-tidy/checks/bugprone/unchecked-optional-access>` by fixing false positive from const reference accessors to objects containing optional member.
 
 Removed checks
 ^^^^^^^^^^^^^^
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index e1394e28cd49a..9381c5c42e566 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -580,6 +580,22 @@ void handleConstMemberCall(const CallExpr *CE,
     return;
   }
 
+  // Cache if the const method returns a reference
+  if (RecordLoc != nullptr && CE->isGLValue()) {
+    const FunctionDecl *DirectCallee = CE->getDirectCallee();
+    if (DirectCallee == nullptr)
+      return;
+
+    StorageLocation &Loc =
+        State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+            *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
+              // no-op
+            });
+
+    State.Env.setStorageLocation(*CE, Loc);
+    return;
+  }
+
   // Cache if the const method returns a boolean or pointer type.
   // We may decide to cache other return types in the future.
   if (RecordLoc != nullptr &&
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 19c3ff49eab27..5031e17188e17 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -3863,6 +3863,200 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessorWithModInBetween) {
   )cc");
 }
 
+TEST_P(UncheckedOptionalAccessTest,
+       ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObject) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A& getA() const { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      if (b.getA().get().has_value()) {
+        b.getA().get().value();
+      }
+    }
+  )cc");
+}
+
+TEST_P(
+    UncheckedOptionalAccessTest,
+    ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithoutValueCheck) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A& getA() const { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      b.getA().get().value(); // [[unsafe]]
+    }
+  )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest,
+       ConstRefToOptionalSavedAsTemporaryVariable) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A& getA() const { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      const auto& opt = b.getA().get();
+      if (opt.has_value()) {
+        opt.value();
+      }
+    }
+  )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest,
+       ConstRefAccessorToOptionalViaAccessorToHoldingObjectByValue) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A copyA() const { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      if (b.copyA().get().has_value()) {
+        b.copyA().get().value(); // [[unsafe]]
+      }
+    }
+  )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest,
+       ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObject) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+      
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      A& getA() { return a; }
+    
+      A a;
+    };
+
+    void target(B& b) {
+      if (b.getA().get().has_value()) {
+        b.getA().get().value(); // [[unsafe]]
+      }
+    }
+  )cc");
+}
+
+TEST_P(
+    UncheckedOptionalAccessTest,
+    ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithModAfterCheck) {
+  ExpectDiagnosticsFor(R"cc(
+    #include "unchecked_optional_access_test.h"
+
+    struct A {
+      const $ns::$optional<int>& get() const { return x; }
+
+      $ns::$optional<int> x;
+    };
+
+    struct B {
+      const A& getA() const { return a; }
+
+      A& getA() { return a; }
+
+      void clear() { a = A{}; }
+
+      A a;
+    };
+
+    void target(B& b) {
+      // changing field A via non-const getter after const getter check
+      if (b.getA().get().has_value()) {
+        b.getA() = A{};
+        b.getA().get().value(); // [[unsafe]]
+      }
+
+      // calling non-const method which might change field A
+      if (b.getA().get().has_value()) {
+        b.clear();
+        b.getA().get().value(); // [[unsafe]]
+      }
+    }
+  )cc");
+}
+
+TEST_P(
+    UncheckedOptionalAccessTest,
+    ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithAnotherConstCallAfterCheck) {
+  ExpectDiagnosticsFor(R"cc(
+      #include "unchecked_optional_access_test.h"
+  
+      struct A {
+        const $ns::$optional<int>& get() const { return x; }
+      
+        $ns::$optional<int> x;
+      };
+  
+      struct B {
+        const A& getA() const { return a; }
+  
+        void callWithoutChanges() const { 
+          // no-op 
+        }
+  
+        A a;
+      };
+  
+      void target(B& b) {  
+        if (b.getA().get().has_value()) {
+          b.callWithoutChanges(); // calling const method which cannot change A
+          b.getA().get().value();
+        }
+      }
+    )cc");
+}
+
 // FIXME: Add support for:
 // - constructors (copy, move)
 // - assignment operators (default, copy, move)

jvoung · 2025-02-27T15:53:51Z

Addressed all comments. Thanks for the initial feedback!

Regarding documentation: looks like we need to merge #122290 first and then update docs in my PR

Thanks for the reminder about #122290 ! Got back to that and merged. Can you update? It looked like the latest release notes had more entries under "Changes in existing checks" like "bugprone-unsafe-functions" ?

BaLiKfromUA · 2025-02-27T23:35:58Z

Updated release notes. Not sure if I need to update clang-tidy/checks/bugprone/unchecked-optional-access.rst. For me it looks like current Exception: accessor methods section covers my fix as well.

clang-tools-extra/docs/ReleaseNotes.rst

jvoung · 2025-02-28T03:20:21Z

Updated release notes. Not sure if I need to update clang-tidy/checks/bugprone/unchecked-optional-access.rst. For me it looks like current Exception: accessor methods section covers my fix as well.

Agreed that the "Exception: accessor methods section covers" your fix as well, so no need to update. Thank you!

jvoung

Thanks!

BaLiKfromUA · 2025-02-28T16:22:39Z

Could someone merge it?

Or do we need another reviewer?

I don't have merge permissions :)

…cked-optional-access` (llvm#128437) Fixes llvm#126283 Extending llvm#112605 to cache const getters which return references. Fixes false positives from const reference accessors to object containing optional member

…r handling Add test for llvm#125589 The crash is actually incidentally fixed by llvm#128437 since it added a branch for the reference case and would no longer fall through when the return type is a reference to a pointer. Clean up a bit as well: - make the fallback for early returns more consistent (check if returning optional and call transfer function for that case) - check RecordLoc == nullptr in one place Add some init for the reference to pointer/bool cases.

…r handling (#129930) Add test for #125589 The crash is actually incidentally fixed by #128437 since it added a branch for the reference case and would no longer fall through when the return type is a reference to a pointer. Clean up a bit as well: - make the fallback for early returns more consistent (check if returning optional and call transfer function for that case) - check RecordLoc == nullptr in one place - clean up extra spaces in test - clean up parameterization in test of `std::` vs `$ns::$`

…nst accessor handling (#129930) Add test for llvm/llvm-project#125589 The crash is actually incidentally fixed by llvm/llvm-project#128437 since it added a branch for the reference case and would no longer fall through when the return type is a reference to a pointer. Clean up a bit as well: - make the fallback for early returns more consistent (check if returning optional and call transfer function for that case) - check RecordLoc == nullptr in one place - clean up extra spaces in test - clean up parameterization in test of `std::` vs `$ns::$`

…r handling (llvm#129930) Add test for llvm#125589 The crash is actually incidentally fixed by llvm#128437 since it added a branch for the reference case and would no longer fall through when the return type is a reference to a pointer. Clean up a bit as well: - make the fallback for early returns more consistent (check if returning optional and call transfer function for that case) - check RecordLoc == nullptr in one place - clean up extra spaces in test - clean up parameterization in test of `std::` vs `$ns::$`

BaLiKfromUA added 3 commits February 23, 2025 12:08

first implementation and basic tests

319ad0b

more tests

d7e3105

format

9608e95

BaLiKfromUA commented Feb 23, 2025

View reviewed changes

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp Outdated Show resolved Hide resolved

BaLiKfromUA commented Feb 23, 2025

View reviewed changes

clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp Outdated Show resolved Hide resolved

BaLiKfromUA commented Feb 23, 2025

View reviewed changes

clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp Show resolved Hide resolved

jvoung reviewed Feb 25, 2025

View reviewed changes

BaLiKfromUA added 4 commits February 26, 2025 22:57

remove redundant reference check and add one more test

011f806

format

a46121e

simplify tests

cf35ecf

update release notes

ad9bbd7

BaLiKfromUA marked this pull request as ready for review February 27, 2025 00:07

llvmbot added clang Clang issues not falling into any other category clang-tools-extra clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html clang:analysis labels Feb 27, 2025

BaLiKfromUA requested a review from jvoung February 27, 2025 00:07

BaLiKfromUA added 2 commits February 27, 2025 23:16

Merge branch 'main' into issue-126283

c46c186

reorder release notes

0e103f6

jvoung reviewed Feb 28, 2025

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

Update ReleaseNotes.rst

36cf5eb

BaLiKfromUA requested a review from jvoung February 28, 2025 14:56

jvoung approved these changes Feb 28, 2025

View reviewed changes

jvoung merged commit 818bca8 into llvm:main Feb 28, 2025
12 checks passed

BaLiKfromUA deleted the issue-126283 branch March 3, 2025 21:16

jvoung mentioned this pull request Mar 5, 2025

[clang][dataflow] Add test for crash repro and clean up const accessor handling #129930

Merged

jvoung mentioned this pull request Mar 7, 2025

[clang-tidy] bugprone-unchecked-optional-access crash on clang/lib/Sema/SemaExpr.cpp #125589

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang-tidy] [dataflow] Cache reference accessors for `bugprone-unchecked-optional-access` #128437

[clang-tidy] [dataflow] Cache reference accessors for `bugprone-unchecked-optional-access` #128437

BaLiKfromUA commented Feb 23, 2025 •

edited

Loading

jvoung left a comment

BaLiKfromUA commented Feb 27, 2025

llvmbot commented Feb 27, 2025

llvmbot commented Feb 27, 2025

llvmbot commented Feb 27, 2025

jvoung commented Feb 27, 2025

BaLiKfromUA commented Feb 27, 2025

jvoung commented Feb 28, 2025

jvoung left a comment

BaLiKfromUA commented Feb 28, 2025

[clang-tidy] [dataflow] Cache reference accessors for bugprone-unchecked-optional-access #128437

[clang-tidy] [dataflow] Cache reference accessors for bugprone-unchecked-optional-access #128437

Conversation

BaLiKfromUA commented Feb 23, 2025 • edited Loading

jvoung left a comment

Choose a reason for hiding this comment

BaLiKfromUA commented Feb 27, 2025

llvmbot commented Feb 27, 2025

llvmbot commented Feb 27, 2025

llvmbot commented Feb 27, 2025

jvoung commented Feb 27, 2025

BaLiKfromUA commented Feb 27, 2025

jvoung commented Feb 28, 2025

jvoung left a comment

Choose a reason for hiding this comment

BaLiKfromUA commented Feb 28, 2025

[clang-tidy] [dataflow] Cache reference accessors for `bugprone-unchecked-optional-access` #128437

[clang-tidy] [dataflow] Cache reference accessors for `bugprone-unchecked-optional-access` #128437

BaLiKfromUA commented Feb 23, 2025 •

edited

Loading