[clang] [clang-tools-extra] [clang-tidy] Add support for bsl::optional (PR #101450)
Chris Cotter via cfe-commits
cfe-commits at lists.llvm.org
Sun Aug 18 19:41:14 PDT 2024
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 <ccotter14 at 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 <ccotter14 at 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 <ccotter14 at 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
More information about the cfe-commits
mailing list