[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
BaLiKfromUA wrote:
Thanks, I will try to look if I can apply your ideas.
Will resolve this thread here to make any new follow-ups on Discourse 👍🏻
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
ymand wrote:
Replied on the discourse discussion. Sorry for the delay -- missed this one!
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
haoNoQ wrote: Hi! Yes, the usual RFC process can probably handle that, it just needs to go to the [Clang Frontend](https://discourse.llvm.org/c/clang/6) section instead of the clang-tidy section. Just plan ahead a little bit, think about the motivation and the prior art, pros and cons and potential caveats, think how far you're willing to go on your own in this direction and how committed you'd be to maintaining this facility in the future, and lay it out in the RFC. It may be a good idea to have a tiny proof-of-concept patch (implement the attribute and teach the clang-tidy check to consume it) (it's very easy to implement an attribute in clang - just add it to the list) but don't write too much code before you're able to confirm that clang maintainers are interested. When it comes to motivation, yeah, every time you find yourself hardcoding a somewhat-standard function or class name in the checker code, it gets better when you have ways to annotate custom classes or replacements. Also the library headers thing is really useful. Off the top of my head, here's a few places that could benefit: - Clang Static Analyzer people such as myself, `@Xazax-hun`, `@steakhal`. (If you're mostly a `clang-tidy` person you know us as `clang-analyzer-*`.) Over the years it's been clear to us that many standard classes require some sort of manual modeling or summary-based modeling in order to handle them correctly. Every time we do that, we might as well support non-standard classes that behave the same way. Two particular pain points for us are: - Smart pointers that perform reference counting, such as `std::shared_ptr` or `llvm::IntrusiveRefCntPtr`. We produce weird use-after-free false positives when we misunderstand the initial reference count inside the smart pointer. - Collections and iterators. Collections such as `std::map` way are too complex for the analyzer to model manually by reading their source code. Custom collection classes are incredibly common too. - The `-Wunsafe-buffer-usage` fixit machine: `@jkorous-apple`, `@dtarditi`. I'm not actively following their progress anymore so you gotta ask them, don't quote me as a confirmation that they actually need it. But I remember them expressing interest in recognizing custom replacements for `std::span` and other buffer containers, mostly for the purposes of suggesting replacements that are appropriate for the codebase that doesn't have access to the standard library. Identifying the appropriate replacement via attributes is one way to do that. If they still need that, an attribute-based solution would be particularly useful for them because hardcoding custom class names in the compiler proper is frowned upon. Like, it'd be super weird if compiler-proper worked differently depending on how you name your classes. That's only acceptable in static analysis tools. - Not exactly a precise hit but: Any kind of use-after-move analysis ([bugprone-use-after-move](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/use-after-move.html), [clang-analyzer-cplusplus.Move](https://clang.llvm.org/docs/analyzer/checkers.html#cplusplus-move-c)) benefits from recognizing methods that "reset" the object to a valid state, making it legal to use after move. This usually includes assignments to the object and methods such as `.reset()` or `.clear()`. If custom classes can be annotated to tell the compiler about such methods the checker can be made significantly more powerful. Note that in this case the nature of the class itself isn't important, only the vague "role" of the method. So this is a slightly different problem but you can probably see how it's closely related and may potentially be addressed by a slight variation of your solution. When it comes to caveats, it's somewhat important to recognize that many custom replacement classes *slightly diverge* from their standard counterparts. - For example, is it really true that `llvm::IntrusiveRefCntPtr` "is basically a" `std::shared_ptr`? They may be "close enough" for some purposes (eg. they're both reference-counted smart pointers) but not for other purposes (eg. `shared_ptr` cannot be unwrapped into a raw pointer and later reconstructed from that raw pointer alone, say after tunneling through some plain C API). Maybe the attribute needs an extra "...for the purposes of" clause? The use-after-move example is kinda the opposite of this problem: it can be seen as "this method is basically a `.reset()` for an *extremely vague* purpose of movable class analysis regardless of all other properties of that class". - Additionally, some custom classes would have methods that have absolutely no counterparts in the original class. For example, they could perform multiple standard operations in one step. Or they may perform an operation to which your checker is sensitive in a very weird way. Depending on how your checker works and what practical tradeoffs you're making in you
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
BaLiKfromUA wrote: @haoNoQ sorry for late reply, want to respond to this idea! I agree that attribute-based solution gives much more flexibility and could benefit not only `bugprone-unchecked-optional-access` check but also to the other existing and future clang-tidy checks. > But, of course, that's a matter of a much broader discussion. I don't think > your work should be blocked on implementing any of this. I'm just saying that > you're not alone in this struggle 😅 **If I were interested in starting such a discussion, what would be the right process to follow?** >From what I understand, it might begin with an RFC on Discourse — but since >this is related to introducing new attributes, it seems like it could also >involve the broader Clang community, not just clang-tidy. Will be interested to hear your opinion on this process, so I can access my capacity and the scope of work! Thanks! cc @vbvictor https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -201,6 +201,11 @@ Changes in existing checks namespace are treated as the tag or the data part of a user-defined tagged union respectively. +- Improved :doc:`bugprone-unchecked-optional-access + ` check by supporting + `NullableValue::makeValue` and `NullableValue::makeValueInplace` to prevent + false-positives for `BloombergLP::bdlb::NullableValue` type. vbvictor wrote: @BaLiKfromUA, sorry I overlooked these ticks. Could you apply this (we use double-ticks for language construct and single tick for general words like options, etc..) https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/vbvictor deleted https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -201,6 +201,11 @@ Changes in existing checks namespace are treated as the tag or the data part of a user-defined tagged union respectively. +- Improved :doc:`bugprone-unchecked-optional-access + ` check by supporting + `NullableValue::makeValue` and `NullableValue::makeValueInplace` to prevent + false-positives for `BloombergLP::bdlb::NullableValue` type. vbvictor wrote: ```suggestion ``NullableValue::makeValue`` and ``NullableValue::makeValueInplace`` to prevent false-positives for ``BloombergLP::bdlb::NullableValue`` type. ``` https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -201,6 +201,11 @@ Changes in existing checks namespace are treated as the tag or the data part of a user-defined tagged union respectively. +- Improved :doc:`bugprone-unchecked-optional-access + ` check by supporting + `NullableValue::makeValue` and `NullableValue::makeValueInplace` to prevent + false-positives for `BloombergLP::bdlb::NullableValue` type. vbvictor wrote: Strangely, "apply from github UI" doesn't work for this diff https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/vbvictor closed https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -201,6 +201,11 @@ Changes in existing checks namespace are treated as the tag or the data part of a user-defined tagged union respectively. +- Improved :doc:`bugprone-unchecked-optional-access + ` check by supporting + `NullableValue::makeValue` and `NullableValue::makeValueInplace` to prevent + false-positives for `BloombergLP::bdlb::NullableValue` type. BaLiKfromUA wrote: Sorry, I was not aware of this style requirement. Applied your diff https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA updated
https://github.com/llvm/llvm-project/pull/144313
>From b2e5ee18bf7f03bbf5f99ef124ef707dc3116886 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 08:55:06 +0100
Subject: [PATCH 1/6] first iteration of fix
---
.../bde/types/bdlb_nullablevalue.h | 8
.../bugprone/unchecked-optional-access.cpp | 14 ++
.../Models/UncheckedOptionalAccessModel.cpp| 13 +
3 files changed, 35 insertions(+)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
index 4411bcfd60a74..0812677111995 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional {
const T &value() const &;
T &value() &;
+ constexpr T &makeValue();
+
+ template
+ constexpr T &makeValue(U&& v);
+
+ template
+ constexpr T &makeValueInplace(ARGS &&... args);
+
// 'operator bool' is inherited from bsl::optional
constexpr bool isNull() const noexcept;
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..b910db20b3c2e 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,20 @@ void
nullable_value_after_swap(BloombergLP::bdlb::NullableValue &opt1, Bloo
}
}
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue &opt1,
BloombergLP::bdlb::NullableValue &opt2) {
+ if (opt1.isNull()) {
+opt1.makeValue(42);
+ }
+
+ opt1.value();
+
+ if (opt2.isNull()) {
+opt2.makeValueInplace(42);
+ }
+
+ opt2.value();
+}
+
template
void function_template_without_user(const absl::optional &opt) {
opt.value(); // no-warning
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
// optional::emplace
.CaseOfCFGStmt(
isOptionalMemberCallWithNameMatcher(hasName("emplace")),
>From cacdf75791044720c51357cc977ed5dac6489a06 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:08:23 +0100
Subject: [PATCH 2/6] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index decb32daa9410..75f193c973784 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -989,7 +989,8 @@ auto buildTransferMatchSwitch() {
// Only NullableValue has these methods, but this
// will also pass for other types
.CaseOfCFGStmt(
- isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ isOptionalMemberCallWithNameMatcher(
+hasAnyName("makeValue", "makeValueInplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (RecordStorageLocation *Loc =
>From fc0262b596b6cc1e11760b7b105514cda23d7694 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:13:36 +0100
Subject: [PATCH 3/6] format
---
.../FlowSensitive/Models/UncheckedOptional
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -201,6 +201,11 @@ Changes in existing checks namespace are treated as the tag or the data part of a user-defined tagged union respectively. +- Improved :doc:`bugprone-unchecked-optional-access + ` check by supporting + `NullableValue::makeValue` and `NullableValue::makeValueInplace` to prevent + false-positives for `BloombergLP::bdlb::NullableValue` type. vbvictor wrote: ```suggestion ``NullableValue::makeValue`` and ``NullableValue::makeValueInplace`` to prevent false-positives for ``BloombergLP::bdlb::NullableValue`` type. ``` https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
BaLiKfromUA wrote: @vbvictor thanks for approval! I addressed your remark about the release note. Could you merge this PR, please, when you are available? I don't have merge permissions https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA updated
https://github.com/llvm/llvm-project/pull/144313
>From b2e5ee18bf7f03bbf5f99ef124ef707dc3116886 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 08:55:06 +0100
Subject: [PATCH 1/5] first iteration of fix
---
.../bde/types/bdlb_nullablevalue.h | 8
.../bugprone/unchecked-optional-access.cpp | 14 ++
.../Models/UncheckedOptionalAccessModel.cpp| 13 +
3 files changed, 35 insertions(+)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
index 4411bcfd60a74..0812677111995 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional {
const T &value() const &;
T &value() &;
+ constexpr T &makeValue();
+
+ template
+ constexpr T &makeValue(U&& v);
+
+ template
+ constexpr T &makeValueInplace(ARGS &&... args);
+
// 'operator bool' is inherited from bsl::optional
constexpr bool isNull() const noexcept;
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..b910db20b3c2e 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,20 @@ void
nullable_value_after_swap(BloombergLP::bdlb::NullableValue &opt1, Bloo
}
}
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue &opt1,
BloombergLP::bdlb::NullableValue &opt2) {
+ if (opt1.isNull()) {
+opt1.makeValue(42);
+ }
+
+ opt1.value();
+
+ if (opt2.isNull()) {
+opt2.makeValueInplace(42);
+ }
+
+ opt2.value();
+}
+
template
void function_template_without_user(const absl::optional &opt) {
opt.value(); // no-warning
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
// optional::emplace
.CaseOfCFGStmt(
isOptionalMemberCallWithNameMatcher(hasName("emplace")),
>From cacdf75791044720c51357cc977ed5dac6489a06 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:08:23 +0100
Subject: [PATCH 2/5] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index decb32daa9410..75f193c973784 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -989,7 +989,8 @@ auto buildTransferMatchSwitch() {
// Only NullableValue has these methods, but this
// will also pass for other types
.CaseOfCFGStmt(
- isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ isOptionalMemberCallWithNameMatcher(
+hasAnyName("makeValue", "makeValueInplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (RecordStorageLocation *Loc =
>From fc0262b596b6cc1e11760b7b105514cda23d7694 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:13:36 +0100
Subject: [PATCH 3/5] format
---
.../FlowSensitive/Models/UncheckedOptional
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA updated
https://github.com/llvm/llvm-project/pull/144313
>From b2e5ee18bf7f03bbf5f99ef124ef707dc3116886 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 08:55:06 +0100
Subject: [PATCH 1/5] first iteration of fix
---
.../bde/types/bdlb_nullablevalue.h | 8
.../bugprone/unchecked-optional-access.cpp | 14 ++
.../Models/UncheckedOptionalAccessModel.cpp| 13 +
3 files changed, 35 insertions(+)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
index 4411bcfd60a74..0812677111995 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional {
const T &value() const &;
T &value() &;
+ constexpr T &makeValue();
+
+ template
+ constexpr T &makeValue(U&& v);
+
+ template
+ constexpr T &makeValueInplace(ARGS &&... args);
+
// 'operator bool' is inherited from bsl::optional
constexpr bool isNull() const noexcept;
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..b910db20b3c2e 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,20 @@ void
nullable_value_after_swap(BloombergLP::bdlb::NullableValue &opt1, Bloo
}
}
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue &opt1,
BloombergLP::bdlb::NullableValue &opt2) {
+ if (opt1.isNull()) {
+opt1.makeValue(42);
+ }
+
+ opt1.value();
+
+ if (opt2.isNull()) {
+opt2.makeValueInplace(42);
+ }
+
+ opt2.value();
+}
+
template
void function_template_without_user(const absl::optional &opt) {
opt.value(); // no-warning
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
// optional::emplace
.CaseOfCFGStmt(
isOptionalMemberCallWithNameMatcher(hasName("emplace")),
>From cacdf75791044720c51357cc977ed5dac6489a06 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:08:23 +0100
Subject: [PATCH 2/5] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index decb32daa9410..75f193c973784 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -989,7 +989,8 @@ auto buildTransferMatchSwitch() {
// Only NullableValue has these methods, but this
// will also pass for other types
.CaseOfCFGStmt(
- isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ isOptionalMemberCallWithNameMatcher(
+hasAnyName("makeValue", "makeValueInplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (RecordStorageLocation *Loc =
>From fc0262b596b6cc1e11760b7b105514cda23d7694 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:13:36 +0100
Subject: [PATCH 3/5] format
---
.../FlowSensitive/Models/UncheckedOptional
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/vbvictor approved this pull request. LGTM, just formatting need to be fixed https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -245,6 +245,9 @@ Changes in existing checks ` check by adding the option `IgnoreAliasing`, that allows not looking at underlying types of type aliases. +- Improved :doc:`bugprone-unchecked-optional-access + ` check by supporting `NullableValue::makeValue` and `NullableValue::makeValueInplace` to prevent false-positives for `BloombergLP::bdlb::NullableValue` type. + vbvictor wrote: Please make lines no more than 80 characters long in ReleaseNotes. https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
haoNoQ wrote:
> > At the moment of the previous PR, there was a concern about configurability
> > of check
>
> I think I was about adding an option to configure list of optional classes.
> Probably `OptionalClasses` or similar. I haven't seen issues about it, so it
> may not be a high priority.
All warnings/checks focused on misuse of specific standard C++ classes have to
deal with that same problem at some point. If only there was a universal,
reusable solution in Clang! Like, it'd be nice if there was some kind of fancy
attribute to annotate classes as "similar" to standard classes:
```c++
[[clang::this_class_is_basically_a_custom("std::optional")]]
class NullableValue {
public:
[[clang::this_method_basically_works_like("std::optional::emplace")]]
template T &makeValueInplace(T &&... args);
};
```
An attribute-based solution would be particularly useful when you're shipping a
library and you get to add the attributes to your headers. This way you don't
have to ask your users to include stuff in their respective clang-tidy configs
whenever they include your library. It'd simply work "out of the box" for all
users.
But, of course, that's a matter of a much broader discussion. I don't think
your work should be blocked on implementing any of this. I'm just saying that
you're not alone in this struggle 😅
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA updated
https://github.com/llvm/llvm-project/pull/144313
>From b2e5ee18bf7f03bbf5f99ef124ef707dc3116886 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 08:55:06 +0100
Subject: [PATCH 1/4] first iteration of fix
---
.../bde/types/bdlb_nullablevalue.h | 8
.../bugprone/unchecked-optional-access.cpp | 14 ++
.../Models/UncheckedOptionalAccessModel.cpp| 13 +
3 files changed, 35 insertions(+)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
index 4411bcfd60a74..0812677111995 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional {
const T &value() const &;
T &value() &;
+ constexpr T &makeValue();
+
+ template
+ constexpr T &makeValue(U&& v);
+
+ template
+ constexpr T &makeValueInplace(ARGS &&... args);
+
// 'operator bool' is inherited from bsl::optional
constexpr bool isNull() const noexcept;
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..b910db20b3c2e 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,20 @@ void
nullable_value_after_swap(BloombergLP::bdlb::NullableValue &opt1, Bloo
}
}
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue &opt1,
BloombergLP::bdlb::NullableValue &opt2) {
+ if (opt1.isNull()) {
+opt1.makeValue(42);
+ }
+
+ opt1.value();
+
+ if (opt2.isNull()) {
+opt2.makeValueInplace(42);
+ }
+
+ opt2.value();
+}
+
template
void function_template_without_user(const absl::optional &opt) {
opt.value(); // no-warning
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
// optional::emplace
.CaseOfCFGStmt(
isOptionalMemberCallWithNameMatcher(hasName("emplace")),
>From cacdf75791044720c51357cc977ed5dac6489a06 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:08:23 +0100
Subject: [PATCH 2/4] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index decb32daa9410..75f193c973784 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -989,7 +989,8 @@ auto buildTransferMatchSwitch() {
// Only NullableValue has these methods, but this
// will also pass for other types
.CaseOfCFGStmt(
- isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ isOptionalMemberCallWithNameMatcher(
+hasAnyName("makeValue", "makeValueInplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (RecordStorageLocation *Loc =
>From fc0262b596b6cc1e11760b7b105514cda23d7694 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:13:36 +0100
Subject: [PATCH 3/4] format
---
.../FlowSensitive/Models/UncheckedOptional
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA updated
https://github.com/llvm/llvm-project/pull/144313
>From b2e5ee18bf7f03bbf5f99ef124ef707dc3116886 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 08:55:06 +0100
Subject: [PATCH 1/4] first iteration of fix
---
.../bde/types/bdlb_nullablevalue.h | 8
.../bugprone/unchecked-optional-access.cpp | 14 ++
.../Models/UncheckedOptionalAccessModel.cpp| 13 +
3 files changed, 35 insertions(+)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
index 4411bcfd60a74..0812677111995 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional {
const T &value() const &;
T &value() &;
+ constexpr T &makeValue();
+
+ template
+ constexpr T &makeValue(U&& v);
+
+ template
+ constexpr T &makeValueInplace(ARGS &&... args);
+
// 'operator bool' is inherited from bsl::optional
constexpr bool isNull() const noexcept;
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..b910db20b3c2e 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,20 @@ void
nullable_value_after_swap(BloombergLP::bdlb::NullableValue &opt1, Bloo
}
}
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue &opt1,
BloombergLP::bdlb::NullableValue &opt2) {
+ if (opt1.isNull()) {
+opt1.makeValue(42);
+ }
+
+ opt1.value();
+
+ if (opt2.isNull()) {
+opt2.makeValueInplace(42);
+ }
+
+ opt2.value();
+}
+
template
void function_template_without_user(const absl::optional &opt) {
opt.value(); // no-warning
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
// optional::emplace
.CaseOfCFGStmt(
isOptionalMemberCallWithNameMatcher(hasName("emplace")),
>From cacdf75791044720c51357cc977ed5dac6489a06 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:08:23 +0100
Subject: [PATCH 2/4] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index decb32daa9410..75f193c973784 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -989,7 +989,8 @@ auto buildTransferMatchSwitch() {
// Only NullableValue has these methods, but this
// will also pass for other types
.CaseOfCFGStmt(
- isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ isOptionalMemberCallWithNameMatcher(
+hasAnyName("makeValue", "makeValueInplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (RecordStorageLocation *Loc =
>From fc0262b596b6cc1e11760b7b105514cda23d7694 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:13:36 +0100
Subject: [PATCH 3/4] format
---
.../FlowSensitive/Models/UncheckedOptional
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA updated
https://github.com/llvm/llvm-project/pull/144313
>From b2e5ee18bf7f03bbf5f99ef124ef707dc3116886 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 08:55:06 +0100
Subject: [PATCH 1/4] first iteration of fix
---
.../bde/types/bdlb_nullablevalue.h | 8
.../bugprone/unchecked-optional-access.cpp | 14 ++
.../Models/UncheckedOptionalAccessModel.cpp| 13 +
3 files changed, 35 insertions(+)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
index 4411bcfd60a74..0812677111995 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional {
const T &value() const &;
T &value() &;
+ constexpr T &makeValue();
+
+ template
+ constexpr T &makeValue(U&& v);
+
+ template
+ constexpr T &makeValueInplace(ARGS &&... args);
+
// 'operator bool' is inherited from bsl::optional
constexpr bool isNull() const noexcept;
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..b910db20b3c2e 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,20 @@ void
nullable_value_after_swap(BloombergLP::bdlb::NullableValue &opt1, Bloo
}
}
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue &opt1,
BloombergLP::bdlb::NullableValue &opt2) {
+ if (opt1.isNull()) {
+opt1.makeValue(42);
+ }
+
+ opt1.value();
+
+ if (opt2.isNull()) {
+opt2.makeValueInplace(42);
+ }
+
+ opt2.value();
+}
+
template
void function_template_without_user(const absl::optional &opt) {
opt.value(); // no-warning
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
// optional::emplace
.CaseOfCFGStmt(
isOptionalMemberCallWithNameMatcher(hasName("emplace")),
>From cacdf75791044720c51357cc977ed5dac6489a06 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:08:23 +0100
Subject: [PATCH 2/4] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index decb32daa9410..75f193c973784 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -989,7 +989,8 @@ auto buildTransferMatchSwitch() {
// Only NullableValue has these methods, but this
// will also pass for other types
.CaseOfCFGStmt(
- isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ isOptionalMemberCallWithNameMatcher(
+hasAnyName("makeValue", "makeValueInplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (RecordStorageLocation *Loc =
>From fc0262b596b6cc1e11760b7b105514cda23d7694 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:13:36 +0100
Subject: [PATCH 3/4] format
---
.../FlowSensitive/Models/UncheckedOptional
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
BaLiKfromUA wrote:
@vbvictor, sorry, got distracted last week by other work!
I created a [topic on
discourse](https://discourse.llvm.org/t/should-dataflowanalysis-diagnosefunction-allow-to-pass-analysis-instance-created-by-caller/88134)
with a summary of the problem, and updated the release notes in this PR.
Please let me know if I need to take any further action to resolve this bug fix.
Thank you!
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA updated
https://github.com/llvm/llvm-project/pull/144313
>From b2e5ee18bf7f03bbf5f99ef124ef707dc3116886 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 08:55:06 +0100
Subject: [PATCH 1/3] first iteration of fix
---
.../bde/types/bdlb_nullablevalue.h | 8
.../bugprone/unchecked-optional-access.cpp | 14 ++
.../Models/UncheckedOptionalAccessModel.cpp| 13 +
3 files changed, 35 insertions(+)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
index 4411bcfd60a74..0812677111995 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional {
const T &value() const &;
T &value() &;
+ constexpr T &makeValue();
+
+ template
+ constexpr T &makeValue(U&& v);
+
+ template
+ constexpr T &makeValueInplace(ARGS &&... args);
+
// 'operator bool' is inherited from bsl::optional
constexpr bool isNull() const noexcept;
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..b910db20b3c2e 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,20 @@ void
nullable_value_after_swap(BloombergLP::bdlb::NullableValue &opt1, Bloo
}
}
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue &opt1,
BloombergLP::bdlb::NullableValue &opt2) {
+ if (opt1.isNull()) {
+opt1.makeValue(42);
+ }
+
+ opt1.value();
+
+ if (opt2.isNull()) {
+opt2.makeValueInplace(42);
+ }
+
+ opt2.value();
+}
+
template
void function_template_without_user(const absl::optional &opt) {
opt.value(); // no-warning
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
// optional::emplace
.CaseOfCFGStmt(
isOptionalMemberCallWithNameMatcher(hasName("emplace")),
>From cacdf75791044720c51357cc977ed5dac6489a06 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:08:23 +0100
Subject: [PATCH 2/3] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index decb32daa9410..75f193c973784 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -989,7 +989,8 @@ auto buildTransferMatchSwitch() {
// Only NullableValue has these methods, but this
// will also pass for other types
.CaseOfCFGStmt(
- isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ isOptionalMemberCallWithNameMatcher(
+hasAnyName("makeValue", "makeValueInplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (RecordStorageLocation *Loc =
>From fc0262b596b6cc1e11760b7b105514cda23d7694 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:13:36 +0100
Subject: [PATCH 3/3] format
---
.../FlowSensitive/Models/UncheckedOptional
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA updated
https://github.com/llvm/llvm-project/pull/144313
>From b2e5ee18bf7f03bbf5f99ef124ef707dc3116886 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 08:55:06 +0100
Subject: [PATCH 1/3] first iteration of fix
---
.../bde/types/bdlb_nullablevalue.h | 8
.../bugprone/unchecked-optional-access.cpp | 14 ++
.../Models/UncheckedOptionalAccessModel.cpp| 13 +
3 files changed, 35 insertions(+)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
index 4411bcfd60a74..0812677111995 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional {
const T &value() const &;
T &value() &;
+ constexpr T &makeValue();
+
+ template
+ constexpr T &makeValue(U&& v);
+
+ template
+ constexpr T &makeValueInplace(ARGS &&... args);
+
// 'operator bool' is inherited from bsl::optional
constexpr bool isNull() const noexcept;
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..b910db20b3c2e 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,20 @@ void
nullable_value_after_swap(BloombergLP::bdlb::NullableValue &opt1, Bloo
}
}
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue &opt1,
BloombergLP::bdlb::NullableValue &opt2) {
+ if (opt1.isNull()) {
+opt1.makeValue(42);
+ }
+
+ opt1.value();
+
+ if (opt2.isNull()) {
+opt2.makeValueInplace(42);
+ }
+
+ opt2.value();
+}
+
template
void function_template_without_user(const absl::optional &opt) {
opt.value(); // no-warning
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
// optional::emplace
.CaseOfCFGStmt(
isOptionalMemberCallWithNameMatcher(hasName("emplace")),
>From cacdf75791044720c51357cc977ed5dac6489a06 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:08:23 +0100
Subject: [PATCH 2/3] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index decb32daa9410..75f193c973784 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -989,7 +989,8 @@ auto buildTransferMatchSwitch() {
// Only NullableValue has these methods, but this
// will also pass for other types
.CaseOfCFGStmt(
- isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ isOptionalMemberCallWithNameMatcher(
+hasAnyName("makeValue", "makeValueInplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (RecordStorageLocation *Loc =
>From fc0262b596b6cc1e11760b7b105514cda23d7694 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:13:36 +0100
Subject: [PATCH 3/3] format
---
.../FlowSensitive/Models/UncheckedOptional
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA updated
https://github.com/llvm/llvm-project/pull/144313
>From b2e5ee18bf7f03bbf5f99ef124ef707dc3116886 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 08:55:06 +0100
Subject: [PATCH 1/3] first iteration of fix
---
.../bde/types/bdlb_nullablevalue.h | 8
.../bugprone/unchecked-optional-access.cpp | 14 ++
.../Models/UncheckedOptionalAccessModel.cpp| 13 +
3 files changed, 35 insertions(+)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
index 4411bcfd60a74..0812677111995 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional {
const T &value() const &;
T &value() &;
+ constexpr T &makeValue();
+
+ template
+ constexpr T &makeValue(U&& v);
+
+ template
+ constexpr T &makeValueInplace(ARGS &&... args);
+
// 'operator bool' is inherited from bsl::optional
constexpr bool isNull() const noexcept;
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..b910db20b3c2e 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,20 @@ void
nullable_value_after_swap(BloombergLP::bdlb::NullableValue &opt1, Bloo
}
}
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue &opt1,
BloombergLP::bdlb::NullableValue &opt2) {
+ if (opt1.isNull()) {
+opt1.makeValue(42);
+ }
+
+ opt1.value();
+
+ if (opt2.isNull()) {
+opt2.makeValueInplace(42);
+ }
+
+ opt2.value();
+}
+
template
void function_template_without_user(const absl::optional &opt) {
opt.value(); // no-warning
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
// optional::emplace
.CaseOfCFGStmt(
isOptionalMemberCallWithNameMatcher(hasName("emplace")),
>From cacdf75791044720c51357cc977ed5dac6489a06 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:08:23 +0100
Subject: [PATCH 2/3] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index decb32daa9410..75f193c973784 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -989,7 +989,8 @@ auto buildTransferMatchSwitch() {
// Only NullableValue has these methods, but this
// will also pass for other types
.CaseOfCFGStmt(
- isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ isOptionalMemberCallWithNameMatcher(
+hasAnyName("makeValue", "makeValueInplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (RecordStorageLocation *Loc =
>From fc0262b596b6cc1e11760b7b105514cda23d7694 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:13:36 +0100
Subject: [PATCH 3/3] format
---
.../FlowSensitive/Models/UncheckedOptional
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
BaLiKfromUA wrote:
Thanks!
I think discourse.llvm.org is a better venue for keeping this discussion.
I'll create a topic tomorrow with a problem statement and context from this PR.
Regarding the PR, I believe that I need to add a release note about the bugfix
before merging, so I will add it as well.
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
vbvictor wrote:
Hi, I don't see any drawbacks in fixing it hardcoded for now.
But I think we should have a different PR to leave this branch for discussion
as is or move discussion to https://discourse.llvm.org/ for further feedback.
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
BaLiKfromUA wrote:
Still waiting for advice
Ping @ymand
@vbvictor WDYT about
https://github.com/llvm/llvm-project/pull/144313#issuecomment-3158000456?
Thank you in advance!
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
Xazax-hun wrote:
Ping @ymand
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
BaLiKfromUA wrote: Still interested in addressing this issue! Is it possible to move the configuration decision to a separate issue or PR? Or is it a blocker? I'm happy to work on the configuration, just don't want to delay the fix to false-positive while we're waiting for decision on the configuration approach. Thanks! https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
BaLiKfromUA wrote: Friendly ping to not lose the visibility of conversation. Waiting for follow up on https://github.com/llvm/llvm-project/pull/144313#discussion_r2204249040 Thank you for review! https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
Xazax-hun wrote:
I'll defer to @ymand on the final decision what is the best way to do this. But
in my opinion, this make a strong argument that the analysis instance should
always be created by the client (e.g., we should pass the analysis instance to
`diagnoseFunction` and similar) as all analyses might need different
configuration options and we definitely don't want to deal with all that in a
generic way in the framework.
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
vbvictor wrote: Analysis reviewers, could you please take a look at this https://github.com/llvm/llvm-project/pull/144313#discussion_r2188585118. Your opinion is highly appreciated! https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
vbvictor wrote:
I'm not familiar with dataflow framework and I can't give a good direction for
implementing this.
Added [ymand](https://github.com/ymand) as one of the previous reviewers of the
code and [Xazax-hun](https://github.com/Xazax-hun),
[gribozavr](https://github.com/gribozavr) as maintainers of analysis framework.
They may share an opinion on this matter.
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
BaLiKfromUA wrote:
I started experimenting with implementing the mentioned configuration and have
a doubt that I would like to talk about.
**What is the preferred way to pass `UncheckedOptionalAccessModelOptions`
inside `UncheckedOptionalAccessModel`?**
Currently, the existing configuration (`IgnoreSmartPointerDereference`) is
being passed into `UncheckedOptionalAccessDiagnoser`, which is not what I need
in my case.
If I understand correctly, passing `Options` into `Model` requires changes to
`DataflowAnalysis.h`.
The "cheapest" approach might be to implement code like this
[here](https://github.com/llvm/llvm-project/blob/c35fbb5460b476ecd210da024eb7c0dda4ad662b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h#L295):
```cpp
template
auto createAnalysis(ASTContext &ASTCtx, Environment &Env, OptionsT &Options)
-> decltype(AnalysisT(ASTCtx, Env, OptionsT)) {
return AnalysisT(ASTCtx, Env, Options);
}
```
Plus, modify the code that calls it and extend the constructor of the
`UncheckedOptionalAccessModel`.
But, [the `FIXME` comment
nearby](https://github.com/llvm/llvm-project/blob/c35fbb5460b476ecd210da024eb7c0dda4ad662b/clang/include/clang/Analysis/FlowSensitive/DataflowAnalysis.h#L292)
highlights that this approach is rather a hack and should be refactored later.
I also see that there is `DataflowAnalysisContext::Options`, but I am not sure
if it's super useful for my case.
@vbvictor would appreciate your feedback!
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
vbvictor wrote:
I'd prefer 2nd option because we have more control over what method belong to
what class. Imagine this situation:
```cpp
struct optional1() {
hasNull(); // check that it has value but it's nullptr
isNull(); // check that it has no value inside
}
struct optional2() {
hasNull(); // check that it has no value inside
}
```
If we go with 1st option, we would write `NonStdNoValue: hasNull;isNull`, but
that would produce errors because `optional1::hasNull` doesn't actually check
for no value, `optional1::isNull` does.
To express intentional behavior, we need to write `NonStdNoValue:
optional2::hasNull;optional1::isNull`.
I don't think we need to worry too much about the length of the default value.
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
vbvictor wrote:
But if it works like option1 right now, I don't oppose implementing option1.
I suppose we could harden it in the future if ever needed. My example with
`optional1` and `optional2` wouldn't appear in real life IMO.
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
vbvictor wrote:
You can look at [no-malloc
check](https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/no-malloc.html)
for similar option with function names.
You can add function-lists both for `bdlb` and other libraries. The code must
be very similar.
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
BaLiKfromUA wrote:
@HerrCai0907 I am happy to do so. Are there any examples how it looks like for
other clang-tidy checks?
Also, do you want to do it in scope of this PR or as a separate one?
Since non-std function names are also processed for `folly::Optional` in code
above my change.
Thanks for review!
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
@@ -985,6 +985,20 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
HerrCai0907 wrote:
I think configurable option is needed instead of hard code function name for
non-std libarary.
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
vbvictor wrote: > At the moment of the previous PR, there was a concern about configurability > of check I think I was about adding an option to configure list of optional classes. Probably `OptionalClasses` or similar. I haven't seen issues about it, so it may not be a high priority. https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA edited https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA updated
https://github.com/llvm/llvm-project/pull/144313
>From b2e5ee18bf7f03bbf5f99ef124ef707dc3116886 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 08:55:06 +0100
Subject: [PATCH 1/3] first iteration of fix
---
.../bde/types/bdlb_nullablevalue.h | 8
.../bugprone/unchecked-optional-access.cpp | 14 ++
.../Models/UncheckedOptionalAccessModel.cpp| 13 +
3 files changed, 35 insertions(+)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
index 4411bcfd60a74..0812677111995 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional {
const T &value() const &;
T &value() &;
+ constexpr T &makeValue();
+
+ template
+ constexpr T &makeValue(U&& v);
+
+ template
+ constexpr T &makeValueInplace(ARGS &&... args);
+
// 'operator bool' is inherited from bsl::optional
constexpr bool isNull() const noexcept;
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..b910db20b3c2e 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,20 @@ void
nullable_value_after_swap(BloombergLP::bdlb::NullableValue &opt1, Bloo
}
}
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue &opt1,
BloombergLP::bdlb::NullableValue &opt2) {
+ if (opt1.isNull()) {
+opt1.makeValue(42);
+ }
+
+ opt1.value();
+
+ if (opt2.isNull()) {
+opt2.makeValueInplace(42);
+ }
+
+ opt2.value();
+}
+
template
void function_template_without_user(const absl::optional &opt) {
opt.value(); // no-warning
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
// optional::emplace
.CaseOfCFGStmt(
isOptionalMemberCallWithNameMatcher(hasName("emplace")),
>From cacdf75791044720c51357cc977ed5dac6489a06 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:08:23 +0100
Subject: [PATCH 2/3] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index decb32daa9410..75f193c973784 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -989,7 +989,8 @@ auto buildTransferMatchSwitch() {
// Only NullableValue has these methods, but this
// will also pass for other types
.CaseOfCFGStmt(
- isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ isOptionalMemberCallWithNameMatcher(
+hasAnyName("makeValue", "makeValueInplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (RecordStorageLocation *Loc =
>From fc0262b596b6cc1e11760b7b105514cda23d7694 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:13:36 +0100
Subject: [PATCH 3/3] format
---
.../FlowSensitive/Models/UncheckedOptional
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
https://github.com/BaLiKfromUA updated
https://github.com/llvm/llvm-project/pull/144313
>From b2e5ee18bf7f03bbf5f99ef124ef707dc3116886 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 08:55:06 +0100
Subject: [PATCH 1/2] first iteration of fix
---
.../bde/types/bdlb_nullablevalue.h | 8
.../bugprone/unchecked-optional-access.cpp | 14 ++
.../Models/UncheckedOptionalAccessModel.cpp| 13 +
3 files changed, 35 insertions(+)
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
index 4411bcfd60a74..0812677111995 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional {
const T &value() const &;
T &value() &;
+ constexpr T &makeValue();
+
+ template
+ constexpr T &makeValue(U&& v);
+
+ template
+ constexpr T &makeValueInplace(ARGS &&... args);
+
// 'operator bool' is inherited from bsl::optional
constexpr bool isNull() const noexcept;
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..b910db20b3c2e 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,20 @@ void
nullable_value_after_swap(BloombergLP::bdlb::NullableValue &opt1, Bloo
}
}
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue &opt1,
BloombergLP::bdlb::NullableValue &opt2) {
+ if (opt1.isNull()) {
+opt1.makeValue(42);
+ }
+
+ opt1.value();
+
+ if (opt2.isNull()) {
+opt2.makeValueInplace(42);
+ }
+
+ opt2.value();
+}
+
template
void function_template_without_user(const absl::optional &opt) {
opt.value(); // no-warning
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
// optional::emplace
.CaseOfCFGStmt(
isOptionalMemberCallWithNameMatcher(hasName("emplace")),
>From cacdf75791044720c51357cc977ed5dac6489a06 Mon Sep 17 00:00:00 2001
From: Valentyn Yukhymenko
Date: Mon, 16 Jun 2025 09:08:23 +0100
Subject: [PATCH 2/2] format
---
.../FlowSensitive/Models/UncheckedOptionalAccessModel.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index decb32daa9410..75f193c973784 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -989,7 +989,8 @@ auto buildTransferMatchSwitch() {
// Only NullableValue has these methods, but this
// will also pass for other types
.CaseOfCFGStmt(
- isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ isOptionalMemberCallWithNameMatcher(
+hasAnyName("makeValue", "makeValueInplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (RecordStorageLocation *Loc =
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
BaLiKfromUA wrote: At the moment of the previous PR, there was a concern about configurability of check -- https://github.com/llvm/llvm-project/pull/101450#pullrequestreview-2213363345 I am happy to address it separately if it's still a concern and if someone can guide me for first steps :) https://github.com/llvm/llvm-project/pull/144313 ___ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff HEAD~1 HEAD --extensions h,cpp --
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
``
View the diff from clang-format here.
``diff
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index bebcc71f7..bb703eff4 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -984,9 +984,10 @@ auto buildTransferMatchSwitch() {
// NullableValue::makeValue, NullableValue::makeValueInplace
// Only NullableValue has these methods, but this
- // will also pass for other types
+ // will also pass for other types
.CaseOfCFGStmt(
- isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("makeValue", "makeValueInplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (RecordStorageLocation *Loc =
``
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang-tools-extra] [clang-tidy] `bugprone-unchecked-optional-access`: handle `BloombergLP::bdlb:NullableValue::makeValue` to prevent false-positives (PR #144313)
llvmbot wrote:
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-analysis
Author: Valentyn Yukhymenko (BaLiKfromUA)
Changes
https://github.com/llvm/llvm-project/pull/101450 added support for
`BloombergLP::bdlb::NullableValue`.
However, `NullableValue::makeValue` and `NullableValue::makeValueInplace` have
been missed which impacts code like this:
```cpp
if (opt.isNull()) {
opt.makeValue(42);
}
opt.value(); // triggers false positive warning from
`bugprone-unchecked-optional-access`
```
My patch addresses this issue.
---
Full diff: https://github.com/llvm/llvm-project/pull/144313.diff
3 Files Affected:
- (modified)
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
(+8)
- (modified)
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
(+14)
- (modified)
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp (+13)
``diff
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
index 4411bcfd60a74..0812677111995 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h
@@ -20,6 +20,14 @@ class NullableValue : public bsl::optional {
const T &value() const &;
T &value() &;
+ constexpr T &makeValue();
+
+ template
+ constexpr T &makeValue(U&& v);
+
+ template
+ constexpr T &makeValueInplace(ARGS &&... args);
+
// 'operator bool' is inherited from bsl::optional
constexpr bool isNull() const noexcept;
diff --git
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..b910db20b3c2e 100644
---
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,20 @@ void
nullable_value_after_swap(BloombergLP::bdlb::NullableValue &opt1, Bloo
}
}
+void nullable_value_make_value(BloombergLP::bdlb::NullableValue &opt1,
BloombergLP::bdlb::NullableValue &opt2) {
+ if (opt1.isNull()) {
+opt1.makeValue(42);
+ }
+
+ opt1.value();
+
+ if (opt2.isNull()) {
+opt2.makeValueInplace(42);
+ }
+
+ opt2.value();
+}
+
template
void function_template_without_user(const absl::optional &opt) {
opt.value(); // no-warning
diff --git
a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
index 164d2574132dd..decb32daa9410 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -985,6 +985,19 @@ auto buildTransferMatchSwitch() {
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
transferOptionalIsNullCall)
+ // NullableValue::makeValue, NullableValue::makeValueInplace
+ // Only NullableValue has these methods, but this
+ // will also pass for other types
+ .CaseOfCFGStmt(
+ isOptionalMemberCallWithNameMatcher(hasAnyName("makeValue",
"makeValueInplace")),
+ [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
+ LatticeTransferState &State) {
+if (RecordStorageLocation *Loc =
+getImplicitObjectLocation(*E, State.Env)) {
+ setHasValue(*Loc, State.Env.getBoolLiteralValue(true),
State.Env);
+}
+ })
+
// optional::emplace
.CaseOfCFGStmt(
isOptionalMemberCallWithNameMatcher(hasName("emplace")),
``
https://github.com/llvm/llvm-project/pull/144313
___
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
