[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
Wed Feb 26 14:59:44 PST 2025
https://github.com/BaLiKfromUA updated https://github.com/llvm/llvm-project/pull/128437
>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/5] 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/5] 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/5] 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"
>From 011f8065e5c2671c933bcf4be18193e89e468ab5 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko <valentin.yukhymenko at gmail.com>
Date: Wed, 26 Feb 2025 22:57:53 +0000
Subject: [PATCH 4/5] remove redundant reference check and add one more test
---
.../Models/UncheckedOptionalAccessModel.cpp | 18 ++++-----
.../UncheckedOptionalAccessModelTest.cpp | 39 ++++++++++++++++++-
2 files changed, 44 insertions(+), 13 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index dccf5ee7f94c2..f9b10bdb62154 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -586,18 +586,14 @@ void handleConstMemberCall(const CallExpr *CE,
if (DirectCallee == nullptr)
return;
- bool isReference =
- DirectCallee->getReturnType().getTypePtr()->isReferenceType();
- if (isReference) {
- StorageLocation &Loc =
- State.Lattice.getOrCreateConstMethodReturnStorageLocation(
- *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
- // no-op
- });
+ StorageLocation &Loc =
+ State.Lattice.getOrCreateConstMethodReturnStorageLocation(
+ *RecordLoc, DirectCallee, State.Env, [&](StorageLocation &Loc) {
+ // no-op
+ });
- State.Env.setStorageLocation(*CE, Loc);
- return;
- }
+ State.Env.setStorageLocation(*CE, Loc);
+ return;
}
// Cache if the const method returns a boolean or pointer type.
diff --git a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
index ddecab3af449d..0aee8f0067242 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -4010,7 +4010,7 @@ TEST_P(UncheckedOptionalAccessTest,
TEST_P(
UncheckedOptionalAccessTest,
- ConstRefAccessorToOptionalViaNonConstRefAccessorToHoldingObjectWithModAfterCheck) {
+ ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithModAfterCheck) {
ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
@@ -4027,7 +4027,7 @@ TEST_P(
A& getA() { return a; }
- void clear() { a = A{}; };
+ void clear() { a = A{}; }
private:
A a;
@@ -4049,6 +4049,41 @@ TEST_P(
)cc");
}
+
+TEST_P(
+ UncheckedOptionalAccessTest,
+ ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithAnotherConstCallAfterCheck) {
+ 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; }
+
+ void callWithoutChanges() const {
+ // no-op
+ }
+
+ private:
+ 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)
>From a46121eb8b6016779790741cf8744cfe38319aa8 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko <valentin.yukhymenko at gmail.com>
Date: Wed, 26 Feb 2025 22:59:33 +0000
Subject: [PATCH 5/5] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 2 +-
.../FlowSensitive/UncheckedOptionalAccessModelTest.cpp | 7 +++----
2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index f9b10bdb62154..9381c5c42e566 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -590,7 +590,7 @@ void handleConstMemberCall(const CallExpr *CE,
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 0aee8f0067242..07269efaece26 100644
--- a/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp
@@ -4049,11 +4049,10 @@ TEST_P(
)cc");
}
-
TEST_P(
- UncheckedOptionalAccessTest,
- ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithAnotherConstCallAfterCheck) {
- ExpectDiagnosticsFor(R"cc(
+ UncheckedOptionalAccessTest,
+ ConstRefAccessorToOptionalViaConstRefAccessorToHoldingObjectWithAnotherConstCallAfterCheck) {
+ ExpectDiagnosticsFor(R"cc(
#include "unchecked_optional_access_test.h"
class A {
More information about the cfe-commits
mailing list