Skip to content

Commit 11c423f

Browse files
ccotterccotter
authored
[clang-tidy] Add support for bsl::optional (#101450)
1 parent 9718949 commit 11c423f

File tree

6 files changed

+267
-16
lines changed

6 files changed

+267
-16
lines changed

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,11 @@ Changes in existing checks
130130
usages of ``sizeof()``, ``alignof()``, and ``offsetof()`` when adding or
131131
subtracting from a pointer.
132132

133+
- Improved :doc:`bugprone-unchecked-optional-access
134+
<clang-tidy/checks/bugprone/unchecked-optional-access>` to support
135+
`bsl::optional` and `bdlb::NullableValue` from
136+
<https://github.com/bloomberg/bde>_.
137+
133138
- Improved :doc:`cert-flp30-c <clang-tidy/checks/cert/flp30-c>` check to
134139
fix false positive that floating point variable is only used in increment
135140
expression.

‎clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,10 @@ results. Therefore, it may be more resource intensive (RAM, CPU) than the
88
average clang-tidy check.
99

1010
This check identifies unsafe accesses to values contained in
11-
``std::optional<T>``, ``absl::optional<T>``, ``base::Optional<T>``, or
12-
``folly::Optional<T>`` objects. Below we will refer to all these types
13-
collectively as ``optional<T>``.
11+
``std::optional<T>``, ``absl::optional<T>``, ``base::Optional<T>``,
12+
``folly::Optional<T>``, ``bsl::optional``, or
13+
``BloombergLP::bdlb::NullableValue`` objects. Below we will refer to all these
14+
types collectively as ``optional<T>``.
1415

1516
An access to the value of an ``optional<T>`` occurs when one of its ``value``,
1617
``operator*``, or ``operator->`` member functions is invoked. To align with

‎clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bdlb_nullablevalue.h

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_NULLABLEVALUE_H_
2+
#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_NULLABLEVALUE_H_
3+
4+
#include "bsl_optional.h"
5+
6+
/// Mock of `bdlb::NullableValue`.
7+
namespace BloombergLP::bdlb {
8+
9+
template <typename T>
10+
class NullableValue : public bsl::optional<T> {
11+
public:
12+
constexpr NullableValue() noexcept;
13+
14+
constexpr NullableValue(bsl::nullopt_t) noexcept;
15+
16+
NullableValue(const NullableValue &) = default;
17+
18+
NullableValue(NullableValue &&) = default;
19+
20+
const T &value() const &;
21+
T &value() &;
22+
23+
// 'operator bool' is inherited from bsl::optional
24+
25+
constexpr bool isNull() const noexcept;
26+
27+
template <typename U>
28+
constexpr T valueOr(U &&v) const &;
29+
30+
// 'reset' is inherited from bsl::optional
31+
32+
template <typename U> NullableValue &operator=(const U &u);
33+
};
34+
35+
36+
} // namespace BloombergLP::bdlb
37+
38+
#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_NULLABLEVALUE_H_

‎clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/bde/types/bsl_optional.h

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_OPTIONAL_H_
2+
#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_OPTIONAL_H_
3+
4+
/// Mock of `bsl::optional`.
5+
namespace bsl {
6+
7+
// clang-format off
8+
template <typename T> struct remove_reference { using type = T; };
9+
template <typename T> struct remove_reference<T&> { using type = T; };
10+
template <typename T> struct remove_reference<T&&> { using type = T; };
11+
// clang-format on
12+
13+
template <typename T>
14+
using remove_reference_t = typename remove_reference<T>::type;
15+
16+
template <typename T>
17+
constexpr T &&forward(remove_reference_t<T> &t) noexcept;
18+
19+
template <typename T>
20+
constexpr T &&forward(remove_reference_t<T> &&t) noexcept;
21+
22+
template <typename T>
23+
constexpr remove_reference_t<T> &&move(T &&x);
24+
25+
struct nullopt_t {
26+
constexpr explicit nullopt_t() {}
27+
};
28+
29+
constexpr nullopt_t nullopt;
30+
31+
template <typename T>
32+
class optional {
33+
public:
34+
constexpr optional() noexcept;
35+
36+
constexpr optional(nullopt_t) noexcept;
37+
38+
optional(const optional &) = default;
39+
40+
optional(optional &&) = default;
41+
42+
const T &operator*() const &;
43+
T &operator*() &;
44+
const T &&operator*() const &&;
45+
T &&operator*() &&;
46+
47+
const T *operator->() const;
48+
T *operator->();
49+
50+
const T &value() const &;
51+
T &value() &;
52+
const T &&value() const &&;
53+
T &&value() &&;
54+
55+
constexpr explicit operator bool() const noexcept;
56+
constexpr bool has_value() const noexcept;
57+
58+
template <typename U>
59+
constexpr T value_or(U &&v) const &;
60+
template <typename U>
61+
T value_or(U &&v) &&;
62+
63+
template <typename... Args>
64+
T &emplace(Args &&...args);
65+
66+
void reset() noexcept;
67+
68+
void swap(optional &rhs) noexcept;
69+
70+
template <typename U> optional &operator=(const U &u);
71+
};
72+
73+
} // namespace bsl
74+
75+
#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_INPUTS_BDE_TYPES_OPTIONAL_H_

‎clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22

33
#include "absl/types/optional.h"
44
#include "folly/types/Optional.h"
5+
#include "bde/types/bsl_optional.h"
6+
#include "bde/types/bdlb_nullablevalue.h"
57

68
void unchecked_value_access(const absl::optional<int> &opt) {
79
opt.value();
@@ -50,6 +52,95 @@ void folly_checked_access(const folly::Optional<int> &opt) {
5052
}
5153
}
5254

55+
void bsl_optional_unchecked_value_access(const bsl::optional<int> &opt) {
56+
opt.value();
57+
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
58+
59+
int x = *opt;
60+
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
61+
62+
if (!opt) {
63+
return;
64+
}
65+
66+
opt.value();
67+
x = *opt;
68+
}
69+
70+
void bsl_optional_checked_access(const bsl::optional<int> &opt) {
71+
if (opt.has_value()) {
72+
opt.value();
73+
}
74+
if (opt) {
75+
opt.value();
76+
}
77+
}
78+
79+
void bsl_optional_value_after_swap(bsl::optional<int> &opt1, bsl::optional<int> &opt2) {
80+
if (opt1) {
81+
opt1.swap(opt2);
82+
opt1.value();
83+
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional value
84+
}
85+
}
86+
87+
void nullable_value_unchecked_value_access(const BloombergLP::bdlb::NullableValue<int> &opt) {
88+
opt.value();
89+
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
90+
91+
int x = *opt;
92+
// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
93+
94+
if (opt.isNull()) {
95+
opt.value();
96+
}
97+
// CHECK-MESSAGES: :[[@LINE-2]]:5: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
98+
99+
if (!opt) {
100+
opt.value();
101+
}
102+
// CHECK-MESSAGES: :[[@LINE-2]]:5: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
103+
104+
if (!opt) {
105+
return;
106+
}
107+
108+
opt.value();
109+
x = *opt;
110+
}
111+
112+
void nullable_value_optional_checked_access(const BloombergLP::bdlb::NullableValue<int> &opt) {
113+
if (opt.has_value()) {
114+
opt.value();
115+
}
116+
if (opt) {
117+
opt.value();
118+
}
119+
if (!opt.isNull()) {
120+
opt.value();
121+
}
122+
}
123+
124+
void nullable_value_emplaced(BloombergLP::bdlb::NullableValue<int> &opt) {
125+
opt.value();
126+
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
127+
128+
opt.emplace(1);
129+
opt.value();
130+
131+
opt.reset();
132+
opt.value();
133+
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
134+
}
135+
136+
void nullable_value_after_swap(BloombergLP::bdlb::NullableValue<int> &opt1, BloombergLP::bdlb::NullableValue<int> &opt2) {
137+
if (opt1) {
138+
opt1.swap(opt2);
139+
opt1.value();
140+
// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional value
141+
}
142+
}
143+
53144
template <typename T>
54145
void function_template_without_user(const absl::optional<T> &opt) {
55146
opt.value(); // no-warning

‎clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp

Lines changed: 54 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,25 @@
3838
namespace clang {
3939
namespace dataflow {
4040

41-
static bool isTopLevelNamespaceWithName(const NamespaceDecl &NS,
42-
llvm::StringRef Name) {
43-
return NS.getDeclName().isIdentifier() && NS.getName() == Name &&
44-
NS.getParent() != nullptr && NS.getParent()->isTranslationUnit();
41+
// Note: the Names appear in reverse order. E.g., to check
42+
// if NS is foo::bar::, call isFullyQualifiedNamespaceEqualTo(NS, "bar", "foo")
43+
template <class... NameTypes>
44+
static bool isFullyQualifiedNamespaceEqualTo(const NamespaceDecl &NS,
45+
llvm::StringRef Name,
46+
NameTypes... Names) {
47+
if (!(NS.getDeclName().isIdentifier() && NS.getName() == Name &&
48+
NS.getParent() != nullptr))
49+
return false;
50+
51+
if constexpr (sizeof...(NameTypes) > 0) {
52+
if (NS.getParent()->isTranslationUnit())
53+
return false;
54+
if (const auto *NextNS = dyn_cast_or_null<NamespaceDecl>(NS.getParent()))
55+
return isFullyQualifiedNamespaceEqualTo(*NextNS, Names...);
56+
return false;
57+
} else {
58+
return NS.getParent()->isTranslationUnit();
59+
}
4560
}
4661

4762
static bool hasOptionalClassName(const CXXRecordDecl &RD) {
@@ -50,15 +65,23 @@ static bool hasOptionalClassName(const CXXRecordDecl &RD) {
5065

5166
if (RD.getName() == "optional") {
5267
if (const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext()))
53-
return N->isStdNamespace() || isTopLevelNamespaceWithName(*N, "absl");
68+
return N->isStdNamespace() ||
69+
isFullyQualifiedNamespaceEqualTo(*N, "absl") ||
70+
isFullyQualifiedNamespaceEqualTo(*N, "bsl");
5471
return false;
5572
}
5673

5774
if (RD.getName() == "Optional") {
5875
// Check whether namespace is "::base" or "::folly".
5976
const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext());
60-
return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") ||
61-
isTopLevelNamespaceWithName(*N, "folly"));
77+
return N != nullptr && (isFullyQualifiedNamespaceEqualTo(*N, "base") ||
78+
isFullyQualifiedNamespaceEqualTo(*N, "folly"));
79+
}
80+
81+
if (RD.getName() == "NullableValue") {
82+
const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext());
83+
return N != nullptr &&
84+
isFullyQualifiedNamespaceEqualTo(*N, "bdlb", "BloombergLP");
6285
}
6386

6487
return false;
@@ -195,22 +218,25 @@ auto isOptionalOperatorCallWithName(
195218
}
196219

197220
auto isMakeOptionalCall() {
198-
return callExpr(callee(functionDecl(hasAnyName(
199-
"std::make_optional", "base::make_optional",
200-
"absl::make_optional", "folly::make_optional"))),
201-
hasOptionalType());
221+
return callExpr(
222+
callee(functionDecl(hasAnyName(
223+
"std::make_optional", "base::make_optional", "absl::make_optional",
224+
"folly::make_optional", "bsl::make_optional"))),
225+
hasOptionalType());
202226
}
203227

204228
auto nulloptTypeDecl() {
205229
return namedDecl(hasAnyName("std::nullopt_t", "absl::nullopt_t",
206-
"base::nullopt_t", "folly::None"));
230+
"base::nullopt_t", "folly::None",
231+
"bsl::nullopt_t"));
207232
}
208233

209234
auto hasNulloptType() { return hasType(nulloptTypeDecl()); }
210235

211236
auto inPlaceClass() {
212237
return recordDecl(hasAnyName("std::in_place_t", "absl::in_place_t",
213-
"base::in_place_t", "folly::in_place_t"));
238+
"base::in_place_t", "folly::in_place_t",
239+
"bsl::in_place_t"));
214240
}
215241

216242
auto isOptionalNulloptConstructor() {
@@ -415,6 +441,15 @@ void transferOptionalHasValueCall(const CXXMemberCallExpr *CallExpr,
415441
}
416442
}
417443

444+
void transferOptionalIsNullCall(const CXXMemberCallExpr *CallExpr,
445+
const MatchFinder::MatchResult &,
446+
LatticeTransferState &State) {
447+
if (auto *HasValueVal = getHasValue(
448+
State.Env, getImplicitObjectLocation(*CallExpr, State.Env))) {
449+
State.Env.setValue(*CallExpr, State.Env.makeNot(*HasValueVal));
450+
}
451+
}
452+
418453
/// `ModelPred` builds a logical formula relating the predicate in
419454
/// `ValueOrPredExpr` to the optional's `has_value` property.
420455
void transferValueOrImpl(
@@ -784,6 +819,12 @@ auto buildTransferMatchSwitch() {
784819
isOptionalMemberCallWithNameMatcher(hasName("operator bool")),
785820
transferOptionalHasValueCall)
786821

822+
// NullableValue::isNull
823+
// Only NullableValue has isNull
824+
.CaseOfCFGStmt<CXXMemberCallExpr>(
825+
isOptionalMemberCallWithNameMatcher(hasName("isNull")),
826+
transferOptionalIsNullCall)
827+
787828
// optional::emplace
788829
.CaseOfCFGStmt<CXXMemberCallExpr>(
789830
isOptionalMemberCallWithNameMatcher(hasName("emplace")),

0 commit comments

Comments
 (0)