https://github.com/BaLiKfromUA created https://github.com/llvm/llvm-project/pull/168863
### Problem `bugprone-unchecked-optional-access` produces a lot of false positives if type inside of `bsl::optional` or `bdlb::NullableValue` is **allocator-aware**. This is a very common pattern, especially due to frequent use of `bsl::string`. [Compiler explorer example to showcase false-positives with BDE library](https://compiler-explorer.com/z/P4zh7KbGx) ### Context https://github.com/llvm/llvm-project/pull/101450 added support for analysing `bsl::optional` access patterns. However, mock `bsl::optional` type has been very simplified for testing purposes which lead to missing false-positives related to _inheritance_ logic for this type. [According to this article](https://bloomberg.github.io/bde/articles/bsl_optional.html#interoperability-between-bsl-optional-and-bdlb-nullablevalue), there are two ways of inheritance for `bsl::optional` and `bdlb::NullableValue`: 1. C++17 non-allocator-aware type ` bdlb::NullableValue<T> -> bsl::optional<T> -> std::optional<T>` 2. C++17 **Allocator-Aware**, and pre-C++17 `bdlb::NullableValue<T> -> bsl::optional<T>` But this is not a full picture :( In practice, there is an additional layer in the inheritance chain: `BloombergLP::bslstl::Optional_Base`. Thus, the actual inheritance structure is: 1. C++17 non-allocator-aware `bdlb::NullableValue<T> -> bsl::optional<T> -> BloombergLP::bslstl::Optional_Base<T> -> std::optional<T>` 2. C++17 **Allocator-Aware**, and pre-C++17 `bdlb::NullableValue<T> -> bsl::optional<T> -> BloombergLP::bslstl::Optional_Base<T>` [Source code to show this inheritance](https://github.com/bloomberg/bde/blob/f8b09a9298a5a76741c0820344c8850bf0b2e177/groups/bsl/bslstl/bslstl_optional.h#L1851) ### Root cause IIUC, because of this inheritance logic, function calls to `bsl::optional::hasValue()` are processed like: 1. `std::optional::has_value()` for non-allocator-aware type. 2. `BloombergLP::bslstl::Optional_Base::has_value()` for allocator-aware type. Obviously, similar conversion are true for other common methods like `.value()` **This PR tries to solve this issue by improving mocks and adding `BloombergLP::bslstl::Optional_Base<T>` to list of supported optional types** >From 95bb5a4636caae4d82262deadac65f02a93cf763 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Wed, 19 Nov 2025 00:26:47 +0000 Subject: [PATCH 1/5] make first version of mocks to illustrate AA branching during inheritance --- .../bde/types/bsl_optional.h | 76 +++++++++++-------- .../std/types/optional.h | 57 ++++++++++++++ 2 files changed, 103 insertions(+), 30 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/std/types/optional.h diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h index 7e1a129e04a55..241a91fb819a2 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h @@ -1,44 +1,36 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_OPTIONAL_H_ #define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_OPTIONAL_H_ -/// Mock of `bsl::optional`. -namespace bsl { +#include "../../std/types/optional.h" -// clang-format off -template <typename T> struct remove_reference { using type = T; }; -template <typename T> struct remove_reference<T&> { using type = T; }; -template <typename T> struct remove_reference<T&&> { using type = T; }; -// clang-format on -template <typename T> -using remove_reference_t = typename remove_reference<T>::type; +/// Mock of `BloombergLP::bslstl::Optional_Base` +namespace BloombergLP::bslstl { -template <typename T> -constexpr T &&forward(remove_reference_t<T> &t) noexcept; +template <class T> +constexpr bool isAllocatorAware() { + return true; +} -template <typename T> -constexpr T &&forward(remove_reference_t<T> &&t) noexcept; +template <> +constexpr bool isAllocatorAware<int>() { + return false; +} -template <typename T> -constexpr remove_reference_t<T> &&move(T &&x); +// Note: in reality `Optional_Base` checks if type uses bsl::allocator<> +// This is simplified mock to illustrate similar behaviour +template <class T, bool AA = isAllocatorAware<T>()> +class Optional_Base { -struct nullopt_t { - constexpr explicit nullopt_t() {} }; -constexpr nullopt_t nullopt; +template <class T> +class Optional_Base<T, false> : public std::optional<T> { +}; -template <typename T> -class optional { +template <class T> +class Optional_Base<T, true> { public: - constexpr optional() noexcept; - - constexpr optional(nullopt_t) noexcept; - - optional(const optional &) = default; - - optional(optional &&) = default; - const T &operator*() const &; T &operator*() &; const T &&operator*() const &&; @@ -65,9 +57,33 @@ class optional { void reset() noexcept; - void swap(optional &rhs) noexcept; + void swap(Optional_Base &rhs) noexcept; + + template <typename U> Optional_Base &operator=(const U &u); +}; - template <typename U> optional &operator=(const U &u); +} // namespace BloombergLP::bslstl + + +/// Mock of `bsl::optional`. +namespace bsl { + +struct nullopt_t { + constexpr explicit nullopt_t() {} +}; + +constexpr nullopt_t nullopt; + +template <typename T> +class optional : public BloombergLP::bslstl::Optional_Base<T> { +public: + constexpr optional() noexcept; + + constexpr optional(nullopt_t) noexcept; + + optional(const optional &) = default; + + optional(optional &&) = default; }; } // namespace bsl diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/std/types/optional.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/std/types/optional.h new file mode 100644 index 0000000000000..75202b6cf1250 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/std/types/optional.h @@ -0,0 +1,57 @@ +#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_STD_TYPES_OPTIONAL_H_ +#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_STD_TYPES_OPTIONAL_H_ + +namespace std { + +struct nullopt_t { + constexpr explicit nullopt_t() {} +}; + +constexpr nullopt_t nullopt; + +template <typename T> +class optional { +public: + constexpr optional() noexcept; + + constexpr optional(nullopt_t) noexcept; + + optional(const optional &) = default; + + optional(optional &&) = default; + + const T &operator*() const &; + T &operator*() &; + const T &&operator*() const &&; + T &&operator*() &&; + + const T *operator->() const; + T *operator->(); + + const T &value() const &; + T &value() &; + const T &&value() const &&; + T &&value() &&; + + constexpr explicit operator bool() const noexcept; + constexpr bool has_value() const noexcept; + + template <typename U> + constexpr T value_or(U &&v) const &; + template <typename U> + T value_or(U &&v) &&; + + template <typename... Args> + T &emplace(Args &&...args); + + void reset() noexcept; + + void swap(optional &rhs) noexcept; + + template <typename U> optional &operator=(const U &u); +}; + +} // namespace std + + +#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_STD_TYPES_OPTIONAL_H_ \ No newline at end of file >From 42d414c7a4106c68c314f50f5eb0194f734d77ba Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Wed, 19 Nov 2025 22:18:27 +0000 Subject: [PATCH 2/5] add failing test --- .../bde/types/bsl_optional.h | 9 ++++++--- .../bugprone/unchecked-optional-access.cpp | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h index 241a91fb819a2..2db7126267366 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h @@ -3,18 +3,21 @@ #include "../../std/types/optional.h" +namespace bsl { + class string {}; +} /// Mock of `BloombergLP::bslstl::Optional_Base` namespace BloombergLP::bslstl { template <class T> constexpr bool isAllocatorAware() { - return true; + return false; } template <> -constexpr bool isAllocatorAware<int>() { - return false; +constexpr bool isAllocatorAware<bsl::string>() { + return true; } // Note: in reality `Optional_Base` checks if type uses bsl::allocator<> 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 8d70a9dc4861e..37fe24b7fd3c4 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 @@ -67,6 +67,21 @@ void bsl_optional_unchecked_value_access(const bsl::optional<int> &opt) { x = *opt; } +void bsl_optional_unchecked_value_access_for_allocation_aware_class(const bsl::optional<bsl::string> &opt) { + opt.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] + + bsl::string x = *opt; + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: unchecked access to optional value [bugprone-unchecked-optional-access] + + if (!opt) { + return; + } + + opt.value(); + x = *opt; +} + void bsl_optional_checked_access(const bsl::optional<int> &opt) { if (opt.has_value()) { opt.value(); >From e0d7d07ad42e64daf2a4b1ed7e5368e6a5312a68 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Thu, 20 Nov 2025 00:59:05 +0000 Subject: [PATCH 3/5] implement fix --- .../bugprone/unchecked-optional-access.cpp | 29 +++++++++++++++++-- .../Models/UncheckedOptionalAccessModel.cpp | 6 ++++ 2 files changed, 33 insertions(+), 2 deletions(-) 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 37fe24b7fd3c4..4b345cc2c8c5b 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 @@ -67,12 +67,12 @@ void bsl_optional_unchecked_value_access(const bsl::optional<int> &opt) { x = *opt; } -void bsl_optional_unchecked_value_access_for_allocation_aware_class(const bsl::optional<bsl::string> &opt) { +void bsl_optional_unchecked_value_access_for_allocator_aware_class(const bsl::optional<bsl::string> &opt) { opt.value(); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] bsl::string x = *opt; - // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: unchecked access to optional value [bugprone-unchecked-optional-access] + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: unchecked access to optional value [bugprone-unchecked-optional-access] if (!opt) { return; @@ -124,6 +124,31 @@ void nullable_value_unchecked_value_access(const BloombergLP::bdlb::NullableValu x = *opt; } +void nullable_value_unchecked_value_access_for_allocator_aware_type(const BloombergLP::bdlb::NullableValue<bsl::string> &opt) { + opt.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] + + bsl::string x = *opt; + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: unchecked access to optional value [bugprone-unchecked-optional-access] + + if (opt.isNull()) { + opt.value(); + } + // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: unchecked access to optional value [bugprone-unchecked-optional-access] + + if (!opt) { + opt.value(); + } + // CHECK-MESSAGES: :[[@LINE-2]]:5: warning: unchecked access to optional value [bugprone-unchecked-optional-access] + + if (!opt.has_value()) { + return; + } + + opt.value(); + x = *opt; +} + void nullable_value_optional_checked_access(const BloombergLP::bdlb::NullableValue<int> &opt) { if (opt.has_value()) { opt.value(); diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index d90f5d4eaf7bb..1a4409f277c18 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -78,6 +78,12 @@ static bool hasOptionalClassName(const CXXRecordDecl &RD) { isFullyQualifiedNamespaceEqualTo(*N, "folly")); } + if (RD.getName() == "Optional_Base") { + const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext()); + return N != nullptr && + isFullyQualifiedNamespaceEqualTo(*N, "bslstl", "BloombergLP"); + } + if (RD.getName() == "NullableValue") { const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext()); return N != nullptr && >From 6ba2c7a64a6440d856d36104e8b248ce4f1c96a2 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Thu, 20 Nov 2025 01:07:50 +0000 Subject: [PATCH 4/5] update note --- .../unchecked-optional-access/bde/types/bsl_optional.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h index 2db7126267366..7d7d14cde6429 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h @@ -20,8 +20,9 @@ constexpr bool isAllocatorAware<bsl::string>() { return true; } -// Note: in reality `Optional_Base` checks if type uses bsl::allocator<> -// This is simplified mock to illustrate similar behaviour +// Note: real `Optional_Base` uses `BloombergLP::bslma::UsesBslmaAllocator` +// to check if type is alocator-aware. +// This is simplified mock to illustrate similar behaviour. template <class T, bool AA = isAllocatorAware<T>()> class Optional_Base { >From 92aacef502b22743d9db6aa4074dbb39d5af8484 Mon Sep 17 00:00:00 2001 From: Valentyn Yukhymenko <[email protected]> Date: Thu, 20 Nov 2025 01:16:19 +0000 Subject: [PATCH 5/5] release note --- clang-tools-extra/docs/ReleaseNotes.rst | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index b982216297919..7226a4eb0ab59 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -377,8 +377,9 @@ Changes in existing checks - Improved :doc:`bugprone-unchecked-optional-access <clang-tidy/checks/bugprone/unchecked-optional-access>` check by supporting ``NullableValue::makeValue`` and ``NullableValue::makeValueInplace`` to - prevent false-positives for ``BloombergLP::bdlb::NullableValue`` type, and by - adding the `IgnoreValueCalls` option to suppress diagnostics for + prevent false-positives for ``BloombergLP::bdlb::NullableValue``. Fixed + false-positives for ``bsl::optional`` containing allocator-aware type. + Added the `IgnoreValueCalls` option to suppress diagnostics for ``optional::value()`` and the `IgnoreSmartPointerDereference` option to ignore optionals reached via smart-pointer-like dereference, while still diagnosing UB-prone dereferences via ``operator*`` and ``operator->``. _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
