llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: Valentyn Yukhymenko (BaLiKfromUA) <details> <summary>Changes</summary> 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. 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 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) `````````` </details> https://github.com/llvm/llvm-project/pull/128437 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
