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) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
