https://github.com/ccotter updated https://github.com/llvm/llvm-project/pull/101450
>From f7e7681db6ad83878fd00cf250047c98b1b4f051 Mon Sep 17 00:00:00 2001 From: Chris Cotter <ccotte...@bloomberg.net> Date: Tue, 23 Jul 2024 10:30:54 -0400 Subject: [PATCH 1/3] [clang-tidy] Add support for bsl::optional --- clang-tools-extra/docs/ReleaseNotes.rst | 5 + .../bde/types/bdlb_nullablevalue.h | 38 ++++++++ .../bde/types/bsl_optional.h | 75 +++++++++++++++ .../bugprone/unchecked-optional-access.cpp | 91 +++++++++++++++++++ .../Models/UncheckedOptionalAccessModel.cpp | 62 ++++++++++--- 5 files changed, 258 insertions(+), 13 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 642ad39cc0c1c5..cc999b142561f0 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -104,6 +104,11 @@ New check aliases Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Improved :doc:`bugprone-unchecked-optional-access + <clang-tidy/checks/bugprone/unchecked-optional-access>` to support + `bsl::optional` and `bdlb::NullableValue` from + <https://github.com/bloomberg/bde>_. + Removed checks ^^^^^^^^^^^^^^ 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 new file mode 100644 index 00000000000000..53efebba1bb9f3 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h @@ -0,0 +1,38 @@ +#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_NULLABLEVALUE_H_ +#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_NULLABLEVALUE_H_ + +#include "bsl_optional.h" + +/// Mock of `bdbl::NullableValue`. +namespace BloombergLP::bdlb { + +template <typename T> +class NullableValue : public bsl::optional<T> { +public: + constexpr NullableValue() noexcept; + + constexpr NullableValue(bsl::nullopt_t) noexcept; + + NullableValue(const NullableValue &) = default; + + NullableValue(NullableValue &&) = default; + + const T &value() const &; + T &value() &; + + // 'operator bool' is inherited from bsl::optional + + constexpr bool isNull() const noexcept; + + template <typename U> + constexpr T valueOr(U &&v) const &; + + // 'reset' is inherited from bsl::optional + + template <typename U> NullableValue &operator=(const U &u); +}; + + +} // namespace BloombergLP::bdlb + +#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_NULLABLEVALUE_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 new file mode 100644 index 00000000000000..7e1a129e04a55f --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h @@ -0,0 +1,75 @@ +#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 { + +// 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; + +template <typename T> +constexpr T &&forward(remove_reference_t<T> &t) noexcept; + +template <typename T> +constexpr T &&forward(remove_reference_t<T> &&t) noexcept; + +template <typename T> +constexpr remove_reference_t<T> &&move(T &&x); + +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 bsl + +#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_OPTIONAL_H_ 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 13a3ff52f3ebc5..3167b85f0e0242 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 @@ -2,6 +2,8 @@ #include "absl/types/optional.h" #include "folly/types/Optional.h" +#include "bde/types/bsl_optional.h" +#include "bde/types/bdlb_nullablevalue.h" void unchecked_value_access(const absl::optional<int> &opt) { opt.value(); @@ -50,6 +52,95 @@ void folly_checked_access(const folly::Optional<int> &opt) { } } +void bsl_optional_unchecked_value_access(const bsl::optional<int> &opt) { + opt.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] + + int 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(); + } + if (opt) { + opt.value(); + } +} + +void bsl_optional_value_after_swap(bsl::optional<int> &opt1, bsl::optional<int> &opt2) { + if (opt1) { + opt1.swap(opt2); + opt1.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional value + } +} + +void nullable_value_unchecked_value_access(const BloombergLP::bdlb::NullableValue<int> &opt) { + opt.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] + + int x = *opt; + // CHECK-MESSAGES: :[[@LINE-1]]:12: 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) { + return; + } + + opt.value(); + x = *opt; +} + +void nullable_value_optional_checked_access(const BloombergLP::bdlb::NullableValue<int> &opt) { + if (opt.has_value()) { + opt.value(); + } + if (opt) { + opt.value(); + } + if (!opt.isNull()) { + opt.value(); + } +} + +void nullable_value_emplaced(BloombergLP::bdlb::NullableValue<int> &opt) { + opt.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] + + opt.emplace(1); + opt.value(); + + opt.reset(); + opt.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access] +} + +void nullable_value_after_swap(BloombergLP::bdlb::NullableValue<int> &opt1, BloombergLP::bdlb::NullableValue<int> &opt2) { + if (opt1) { + opt1.swap(opt2); + opt1.value(); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional value + } +} + template <typename T> void function_template_without_user(const absl::optional<T> &opt) { opt.value(); // no-warning diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 0707aa662e4cc2..9d88754639fa39 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -38,10 +38,22 @@ namespace clang { namespace dataflow { -static bool isTopLevelNamespaceWithName(const NamespaceDecl &NS, - llvm::StringRef Name) { - return NS.getDeclName().isIdentifier() && NS.getName() == Name && - NS.getParent() != nullptr && NS.getParent()->isTranslationUnit(); +template <class... NameTypes> +static bool hasNestedNamespace(const NamespaceDecl &NS, llvm::StringRef Name, + NameTypes... Names) { + if (!(NS.getDeclName().isIdentifier() && NS.getName() == Name && + NS.getParent() != nullptr)) + return false; + + if constexpr (sizeof...(NameTypes) > 0) { + if (NS.getParent()->isTranslationUnit()) + return false; + if (const auto *NextNS = dyn_cast_or_null<NamespaceDecl>(NS.getParent())) + return hasNestedNamespace(*NextNS, Names...); + return false; + } else { + return NS.getParent()->isTranslationUnit(); + } } static bool hasOptionalClassName(const CXXRecordDecl &RD) { @@ -50,15 +62,21 @@ static bool hasOptionalClassName(const CXXRecordDecl &RD) { if (RD.getName() == "optional") { if (const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext())) - return N->isStdNamespace() || isTopLevelNamespaceWithName(*N, "absl"); + return N->isStdNamespace() || hasNestedNamespace(*N, "absl") || + hasNestedNamespace(*N, "bsl"); return false; } if (RD.getName() == "Optional") { // Check whether namespace is "::base" or "::folly". const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext()); - return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") || - isTopLevelNamespaceWithName(*N, "folly")); + return N != nullptr && + (hasNestedNamespace(*N, "base") || hasNestedNamespace(*N, "folly")); + } + + if (RD.getName() == "NullableValue") { + const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext()); + return N != nullptr && hasNestedNamespace(*N, "bdlb", "BloombergLP"); } return false; @@ -195,22 +213,25 @@ auto isOptionalOperatorCallWithName( } auto isMakeOptionalCall() { - return callExpr(callee(functionDecl(hasAnyName( - "std::make_optional", "base::make_optional", - "absl::make_optional", "folly::make_optional"))), - hasOptionalType()); + return callExpr( + callee(functionDecl(hasAnyName( + "std::make_optional", "base::make_optional", "absl::make_optional", + "folly::make_optional", "bsl::make_optional"))), + hasOptionalType()); } auto nulloptTypeDecl() { return namedDecl(hasAnyName("std::nullopt_t", "absl::nullopt_t", - "base::nullopt_t", "folly::None")); + "base::nullopt_t", "folly::None", + "bsl::nullopt_t")); } auto hasNulloptType() { return hasType(nulloptTypeDecl()); } auto inPlaceClass() { return recordDecl(hasAnyName("std::in_place_t", "absl::in_place_t", - "base::in_place_t", "folly::in_place_t")); + "base::in_place_t", "folly::in_place_t", + "bsl::in_place_t")); } auto isOptionalNulloptConstructor() { @@ -415,6 +436,15 @@ void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr, } } +void transferOptionalIsNullCall(const CXXMemberCallExpr *CallExpr, + const MatchFinder::MatchResult &, + LatticeTransferState &State) { + if (auto *HasValueVal = getHasValue( + State.Env, getImplicitObjectLocation(*CallExpr, State.Env))) { + State.Env.setValue(*CallExpr, State.Env.makeNot(*HasValueVal)); + } +} + /// `ModelPred` builds a logical formula relating the predicate in /// `ValueOrPredExpr` to the optional's `has_value` property. void transferValueOrImpl( @@ -784,6 +814,12 @@ auto buildTransferMatchSwitch() { isOptionalMemberCallWithNameMatcher(hasName("operator bool")), transferOptionalHasValueCall) + // NullableValue::isNull + // Only NullableValue has isNull + .CaseOfCFGStmt<CXXMemberCallExpr>( + isOptionalMemberCallWithNameMatcher(hasAnyName("isNull")), + transferOptionalIsNullCall) + // optional::emplace .CaseOfCFGStmt<CXXMemberCallExpr>( isOptionalMemberCallWithNameMatcher(hasName("emplace")), >From 5ed4ef3d97f3e24af1c671ed2cca7dffcff67933 Mon Sep 17 00:00:00 2001 From: Chris Cotter <ccotte...@bloomberg.net> Date: Thu, 1 Aug 2024 14:10:19 -0400 Subject: [PATCH 2/3] Update docs --- .../checks/bugprone/unchecked-optional-access.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst index 5a6aaa077d9bff..97fe37b5353560 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst @@ -8,9 +8,10 @@ results. Therefore, it may be more resource intensive (RAM, CPU) than the average clang-tidy check. This check identifies unsafe accesses to values contained in -``std::optional<T>``, ``absl::optional<T>``, ``base::Optional<T>``, or -``folly::Optional<T>`` objects. Below we will refer to all these types -collectively as ``optional<T>``. +``std::optional<T>``, ``absl::optional<T>``, ``base::Optional<T>``, +``folly::Optional<T>``, ``bsl::optional``, or +``BloombergLP::bdlb::NullableValue`` objects. Below we will refer to all these +types collectively as ``optional<T>``. An access to the value of an ``optional<T>`` occurs when one of its ``value``, ``operator*``, or ``operator->`` member functions is invoked. To align with >From bd2aa7d955f66ac3a67e6a665ba14a464ee62eb6 Mon Sep 17 00:00:00 2001 From: Chris Cotter <ccotte...@bloomberg.net> Date: Thu, 1 Aug 2024 14:10:19 -0400 Subject: [PATCH 3/3] Review feedback --- .../Models/UncheckedOptionalAccessModel.cpp | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 9d88754639fa39..70ffe92753e053 100644 --- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp +++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp @@ -38,9 +38,12 @@ namespace clang { namespace dataflow { +// Note: the Names appear in reverse order. E.g., to check +// if NS is foo::bar::, call isFullyQualifiedNamespaceEqualTo(NS, "bar", "foo") template <class... NameTypes> -static bool hasNestedNamespace(const NamespaceDecl &NS, llvm::StringRef Name, - NameTypes... Names) { +static bool isFullyQualifiedNamespaceEqualTo(const NamespaceDecl &NS, + llvm::StringRef Name, + NameTypes... Names) { if (!(NS.getDeclName().isIdentifier() && NS.getName() == Name && NS.getParent() != nullptr)) return false; @@ -49,7 +52,7 @@ static bool hasNestedNamespace(const NamespaceDecl &NS, llvm::StringRef Name, if (NS.getParent()->isTranslationUnit()) return false; if (const auto *NextNS = dyn_cast_or_null<NamespaceDecl>(NS.getParent())) - return hasNestedNamespace(*NextNS, Names...); + return isFullyQualifiedNamespaceEqualTo(*NextNS, Names...); return false; } else { return NS.getParent()->isTranslationUnit(); @@ -62,21 +65,23 @@ static bool hasOptionalClassName(const CXXRecordDecl &RD) { if (RD.getName() == "optional") { if (const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext())) - return N->isStdNamespace() || hasNestedNamespace(*N, "absl") || - hasNestedNamespace(*N, "bsl"); + return N->isStdNamespace() || + isFullyQualifiedNamespaceEqualTo(*N, "absl") || + isFullyQualifiedNamespaceEqualTo(*N, "bsl"); return false; } if (RD.getName() == "Optional") { // Check whether namespace is "::base" or "::folly". const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext()); - return N != nullptr && - (hasNestedNamespace(*N, "base") || hasNestedNamespace(*N, "folly")); + return N != nullptr && (isFullyQualifiedNamespaceEqualTo(*N, "base") || + isFullyQualifiedNamespaceEqualTo(*N, "folly")); } if (RD.getName() == "NullableValue") { const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext()); - return N != nullptr && hasNestedNamespace(*N, "bdlb", "BloombergLP"); + return N != nullptr && + isFullyQualifiedNamespaceEqualTo(*N, "bdlb", "BloombergLP"); } return false; @@ -817,7 +822,7 @@ auto buildTransferMatchSwitch() { // NullableValue::isNull // Only NullableValue has isNull .CaseOfCFGStmt<CXXMemberCallExpr>( - isOptionalMemberCallWithNameMatcher(hasAnyName("isNull")), + isOptionalMemberCallWithNameMatcher(hasName("isNull")), transferOptionalIsNullCall) // optional::emplace _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
