[clang] [clang-tidy] [dataflow] Cache reference accessors for `bugprone-unchecked-optional-access` (PR #128437)
Valentyn Yukhymenko via cfe-commits
cfe-commits at lists.llvm.org
Sun Feb 23 13:50:28 PST 2025
https://github.com/BaLiKfromUA created https://github.com/llvm/llvm-project/pull/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.
This should fix false positive cases when we check optional via the chain of const getter calls.
>From 319ad0b803b8c6c6c5405178335bd1f2258be4b8 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko <valentin.yukhymenko at gmail.com>
Date: Sun, 23 Feb 2025 12:08:02 +0000
Subject: [PATCH 1/3] first implementation and basic tests
---
.../Models/UncheckedOptionalAccessModel.cpp | 20 +++++++
.../UncheckedOptionalAccessModelTest.cpp | 59 +++++++++++++++++++
2 files changed, 79 insertions(+)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index e1394e28cd49a..993967e0c3edd 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -580,6 +580,26 @@ void handleConstMemberCall(const CallExpr *CE,
return;
}
+ // if const method returns a reference
+ if (CE->isGLValue()) {
+ const FunctionDecl *DirectCallee = CE->getDirectCallee();
+ if (DirectCallee == nullptr)
+ return;
+
+ QualType DeclaredReturnType = DirectCallee->getReturnType();
+
+ if (DeclaredReturnType.getTypePtr()->isReferenceType()) {
+ 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..7140040022794 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -3863,6 +3863,65 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessorWithModInBetween) {
)cc");
}
+
+TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObject) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ class A {
+ public:
+ const $ns::$optional<int>& get() const { return x; }
+
+ private:
+ $ns::$optional<int> x;
+ };
+
+ class B {
+ public:
+ const A& getA() const { return a; }
+
+ private:
+ 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"
+
+ class A {
+ public:
+ const $ns::$optional<int>& get() const { return x; }
+
+ private:
+ $ns::$optional<int> x;
+ };
+
+ class B {
+ public:
+ const A& getA() const { return a; }
+
+ private:
+ A a;
+ };
+
+ void target(B& b) {
+ b.getA().get().value(); // [[unsafe]]
+ }
+ )cc");
+}
+
+// todo: non const accessor
+// todo: different accessor in between
+// todo: const copy
+
// FIXME: Add support for:
// - constructors (copy, move)
// - assignment operators (default, copy, move)
>From d7e3105087d5347fe100f0a567c1538c1a3673c0 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko <valentin.yukhymenko at gmail.com>
Date: Sun, 23 Feb 2025 21:37:54 +0000
Subject: [PATCH 2/3] more tests
---
.../Models/UncheckedOptionalAccessModel.cpp | 11 +-
.../UncheckedOptionalAccessModelTest.cpp | 126 +++++++++++++++++-
2 files changed, 128 insertions(+), 9 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 993967e0c3edd..a35ac09b15502 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -580,19 +580,18 @@ void handleConstMemberCall(const CallExpr *CE,
return;
}
- // if const method returns a reference
- if (CE->isGLValue()) {
+ // Cache if the const method returns a reference
+ if (RecordLoc != nullptr && CE->isGLValue()) {
const FunctionDecl *DirectCallee = CE->getDirectCallee();
if (DirectCallee == nullptr)
return;
- QualType DeclaredReturnType = DirectCallee->getReturnType();
-
- if (DeclaredReturnType.getTypePtr()->isReferenceType()) {
+ bool isReference = DirectCallee->getReturnType().getTypePtr()->isReferenceType();
+ if (isReference) {
StorageLocation &Loc =
State.Lattice.getOrCreateConstMethodReturnStorageLocation(
*RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
- // No-op
+ // no-op
});
State.Env.setStorageLocation(*CE, Loc);
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 7140040022794..4cec24829885c 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -3918,9 +3918,129 @@ TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccesso
)cc");
}
-// todo: non const accessor
-// todo: different accessor in between
-// todo: const copy
+TEST_P(UncheckedOptionalAccessTest, ConstRefToOptionalSavedAsTemporaryVariable) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ class A {
+ public:
+ const $ns::$optional<int>& get() const { return x; }
+
+ private:
+ $ns::$optional<int> x;
+ };
+
+ class B {
+ public:
+ const A& getA() const { return a; }
+
+ private:
+ A a;
+ };
+
+ void target(B& b) {
+ const auto& opt = b.getA().get();
+ if (opt.has_value()) {
+ opt.value();
+ }
+ }
+ )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstValueAccessorToHoldingObject) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ class A {
+ public:
+ const $ns::$optional<int>& get() const { return x; }
+
+ private:
+ $ns::$optional<int> x;
+ };
+
+ class B {
+ public:
+ const A copyA() const { return a; }
+
+ private:
+ 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"
+
+ class A {
+ public:
+ const $ns::$optional<int>& get() const { return x; }
+
+ private:
+ $ns::$optional<int> x;
+ };
+
+ class B {
+ public:
+ A& getA() { return a; }
+
+ private:
+ A a;
+ };
+
+ void target(B& b) {
+ if (b.getA().get().has_value()) {
+ b.getA().get().value(); // [[unsafe]]
+ }
+ }
+ )cc");
+}
+
+TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObjectWithModAfterCheck) {
+ ExpectDiagnosticsFor(R"cc(
+ #include "unchecked_optional_access_test.h"
+
+ class A {
+ public:
+ const $ns::$optional<int>& get() const { return x; }
+ private:
+ $ns::$optional<int> x;
+ };
+
+ class B {
+ public:
+ const A& getA() const { return a; }
+
+ A& getA() { return a; }
+
+ void clear() { a = A{}; };
+
+ private:
+ 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");
+}
// FIXME: Add support for:
// - constructors (copy, move)
>From 9608e954136b6cd8ee51ce5a301b828caadb314e Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko <valentin.yukhymenko at gmail.com>
Date: Sun, 23 Feb 2025 21:42:36 +0000
Subject: [PATCH 3/3] format
---
.../Models/UncheckedOptionalAccessModel.cpp | 13 ++++++------
.../UncheckedOptionalAccessModelTest.cpp | 21 ++++++++++++-------
2 files changed, 21 insertions(+), 13 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index a35ac09b15502..dccf5ee7f94c2 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -586,14 +586,15 @@ void handleConstMemberCall(const CallExpr *CE,
if (DirectCallee == nullptr)
return;
- bool isReference = DirectCallee->getReturnType().getTypePtr()->isReferenceType();
+ bool isReference =
+ DirectCallee->getReturnType().getTypePtr()->isReferenceType();
if (isReference) {
StorageLocation &Loc =
- State.Lattice.getOrCreateConstMethodReturnStorageLocation(
- *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
- // no-op
- });
-
+ State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+ *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
+ // no-op
+ });
+
State.Env.setStorageLocation(*CE, Loc);
return;
}
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index 4cec24829885c..ddecab3af449d 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -3863,8 +3863,8 @@ TEST_P(UncheckedOptionalAccessTest, ConstBoolAccessorWithModInBetween) {
)cc");
}
-
-TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObject) {
+TEST_P(UncheckedOptionalAccessTest,
+ ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObject) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
@@ -3892,7 +3892,9 @@ TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccesso
)cc");
}
-TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithoutValueCheck) {
+TEST_P(
+ UncheckedOptionalAccessTest,
+ ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithoutValueCheck) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
@@ -3918,7 +3920,8 @@ TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstRefAccesso
)cc");
}
-TEST_P(UncheckedOptionalAccessTest, ConstRefToOptionalSavedAsTemporaryVariable) {
+TEST_P(UncheckedOptionalAccessTest,
+ ConstRefToOptionalSavedAsTemporaryVariable) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
@@ -3947,7 +3950,8 @@ TEST_P(UncheckedOptionalAccessTest, ConstRefToOptionalSavedAsTemporaryVariable)
)cc");
}
-TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstValueAccessorToHoldingObject) {
+TEST_P(UncheckedOptionalAccessTest,
+ ConstRefAccessorToOptionalViaConstValueAccessorToHoldingObject) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
@@ -3975,7 +3979,8 @@ TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaConstValueAcces
)cc");
}
-TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObject) {
+TEST_P(UncheckedOptionalAccessTest,
+ ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObject) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
@@ -4003,7 +4008,9 @@ TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaNonConstRefAcce
)cc");
}
-TEST_P(UncheckedOptionalAccessTest, ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObjectWithModAfterCheck) {
+TEST_P(
+ UncheckedOptionalAccessTest,
+ ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObjectWithModAfterCheck) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
More information about the cfe-commits
mailing list