[clang-tools-extra] 818bca8 - [clang-tidy] [dataflow] Cache reference accessors for `bugprone-unchecked-optional-access` (#128437)

[via cfe-commits]

Author: Valentyn Yukhymenko
Date: 2025-02-28T13:27:20-05:00
New Revision: 818bca820ffd3e30fbd3852da0436c24ff15f8a3

URL: https://github.com/llvm/llvm-project/commit/818bca820ffd3e30fbd3852da0436c24ff15f8a3
DIFF: https://github.com/llvm/llvm-project/commit/818bca820ffd3e30fbd3852da0436c24ff15f8a3.diff

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

Fixes https://github.com/llvm/llvm-project/issues/126283

Extending https://github.com/llvm/llvm-project/pull/112605 to cache
const getters which return references.

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

Added: 
    

Modified: 
    clang-tools-extra/docs/ReleaseNotes.rst
    clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
    clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index a8d17d19fda1d..07a79d6bbe807 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -112,7 +112,8 @@ Changes in existing checks
   <clang-tidy/checks/bugprone/unchecked-optional-access>` fixing false
   positives from smart pointer accessors repeated in checking ``has_value``
   and accessing ``value``. The option `IgnoreSmartPointerDereference` should
-  no longer be needed and will be removed.
+  no longer be needed and will be removed. Also fixing false positive from 
+  const reference accessors to objects containing optional member.
 
 - Improved :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` check to allow specifying

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)