[clang-tools-extra] 2f0630f - [clang-tidy] Add folly::Optional to unchecked-optional-access
Piotr Zegar via cfe-commits
cfe-commits at lists.llvm.org
Mon Jul 24 12:42:50 PDT 2023
Author: Anton Dukeman
Date: 2023-07-24T19:42:19Z
New Revision: 2f0630f8bc91e81fd5948e82164cf82ae595caf2
URL: https://github.com/llvm/llvm-project/commit/2f0630f8bc91e81fd5948e82164cf82ae595caf2
DIFF: https://github.com/llvm/llvm-project/commit/2f0630f8bc91e81fd5948e82164cf82ae595caf2.diff
LOG: [clang-tidy] Add folly::Optional to unchecked-optional-access
The unchecked-optional-access check identifies attempted value
unwrapping without checking if the value exists. These changes extend
that support to checking folly::Optional.
Reviewed By: gribozavr2
Differential Revision: https://reviews.llvm.org/D155890
Added:
clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/folly/types/Optional.h
Modified:
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index d7a525b99fbcf1..7c4d06b0d308f9 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -319,7 +319,7 @@ Changes in existing checks
- Improved :doc:`bugprone-unchecked-optional-access
<clang-tidy/checks/bugprone/unchecked-optional-access>` check to properly handle calls
- to ``std::forward``.
+ to ``std::forward`` and support for ``folly::Optional`` were added.
- Extend :doc:`bugprone-unused-return-value
<clang-tidy/checks/bugprone/unused-return-value>` check to check for all functions
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 47365185e42b0f..5a6aaa077d9bff 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,8 +8,9 @@ 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>``, or ``base::Optional<T>``
-objects. Below we will refer to all these types collectively as ``optional<T>``.
+``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>``.
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
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/folly/types/Optional.h b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/folly/types/Optional.h
new file mode 100644
index 00000000000000..818b06623cb7fd
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/folly/types/Optional.h
@@ -0,0 +1,56 @@
+#ifndef LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_BUGPRONE_INPUTS_UNCHECKED_OPTIONAL_ACCESS_FOLLY_TYPES_OPTIONAL_H_
+#define LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_BUGPRONE_INPUTS_UNCHECKED_OPTIONAL_ACCESS_FOLLY_TYPES_OPTIONAL_H_
+
+/// Mock of `folly::Optional`.
+namespace folly {
+
+struct None {
+ constexpr explicit None() {}
+};
+
+constexpr None none;
+
+template <typename T>
+class Optional {
+public:
+ constexpr Optional() noexcept;
+
+ constexpr Optional(None) 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;
+ constexpr bool hasValue() 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;
+};
+
+} // namespace folly
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_TEST_CLANG_TIDY_CHECKERS_BUGPRONE_INPUTS_UNCHECKED_OPTIONAL_ACCESS_FOLLY_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 c1e731f411719e..1921291f2187d9 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
@@ -1,6 +1,7 @@
// RUN: %check_clang_tidy %s bugprone-unchecked-optional-access %t -- -- -I %S/Inputs/unchecked-optional-access
#include "absl/types/optional.h"
+#include "folly/types/Optional.h"
void unchecked_value_access(const absl::optional<int> &opt) {
opt.value();
@@ -21,12 +22,34 @@ void unchecked_arrow_operator_access(const absl::optional<Foo> &opt) {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value
}
+void folly_check_value_then_reset(folly::Optional<int> opt) {
+ if (opt) {
+ opt.reset();
+ opt.value();
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional value
+ }
+}
+
+void folly_value_after_swap(folly::Optional<int> opt1, folly::Optional<int> opt2) {
+ if (opt1) {
+ opt1.swap(opt2);
+ opt1.value();
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: unchecked access to optional value
+ }
+}
+
void checked_access(const absl::optional<int> &opt) {
if (opt.has_value()) {
opt.value();
}
}
+void folly_checked_access(const folly::Optional<int> &opt) {
+ if (opt.hasValue()) {
+ opt.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 1db255dd81f3f8..b0a8667f3fe5be 100644
--- a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -56,9 +56,10 @@ static bool hasOptionalClassName(const CXXRecordDecl &RD) {
}
if (RD.getName() == "Optional") {
- // Check whether namespace is "::base".
+ // Check whether namespace is "::base" or "::folly".
const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext());
- return N != nullptr && isTopLevelNamespaceWithName(*N, "base");
+ return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") ||
+ isTopLevelNamespaceWithName(*N, "folly"));
}
return false;
@@ -87,8 +88,8 @@ auto optionalOrAliasType() {
/// Matches any of the spellings of the optional types and sugar, aliases, etc.
auto hasOptionalType() { return hasType(optionalOrAliasType()); }
-auto isOptionalMemberCallWithName(
- llvm::StringRef MemberName,
+auto isOptionalMemberCallWithNameMatcher(
+ ast_matchers::internal::Matcher<NamedDecl> matcher,
const std::optional<StatementMatcher> &Ignorable = std::nullopt) {
auto Exception = unless(Ignorable ? expr(anyOf(*Ignorable, cxxThisExpr()))
: cxxThisExpr());
@@ -96,7 +97,7 @@ auto isOptionalMemberCallWithName(
on(expr(Exception,
anyOf(hasOptionalType(),
hasType(pointerType(pointee(optionalOrAliasType())))))),
- callee(cxxMethodDecl(hasName(MemberName))));
+ callee(cxxMethodDecl(matcher)));
}
auto isOptionalOperatorCallWithName(
@@ -109,15 +110,15 @@ auto isOptionalOperatorCallWithName(
}
auto isMakeOptionalCall() {
- return callExpr(
- callee(functionDecl(hasAnyName(
- "std::make_optional", "base::make_optional", "absl::make_optional"))),
- hasOptionalType());
+ return callExpr(callee(functionDecl(hasAnyName(
+ "std::make_optional", "base::make_optional",
+ "absl::make_optional", "folly::make_optional"))),
+ hasOptionalType());
}
auto nulloptTypeDecl() {
- return namedDecl(
- hasAnyName("std::nullopt_t", "absl::nullopt_t", "base::nullopt_t"));
+ return namedDecl(hasAnyName("std::nullopt_t", "absl::nullopt_t",
+ "base::nullopt_t", "folly::None"));
}
auto hasNulloptType() { return hasType(nulloptTypeDecl()); }
@@ -129,8 +130,8 @@ auto hasAnyOptionalType() {
}
auto inPlaceClass() {
- return recordDecl(
- hasAnyName("std::in_place_t", "absl::in_place_t", "base::in_place_t"));
+ return recordDecl(hasAnyName("std::in_place_t", "absl::in_place_t",
+ "base::in_place_t", "folly::in_place_t"));
}
auto isOptionalNulloptConstructor() {
@@ -750,7 +751,8 @@ ignorableOptional(const UncheckedOptionalAccessModelOptions &Options) {
StatementMatcher
valueCall(const std::optional<StatementMatcher> &IgnorableOptional) {
- return isOptionalMemberCallWithName("value", IgnorableOptional);
+ return isOptionalMemberCallWithNameMatcher(hasName("value"),
+ IgnorableOptional);
}
StatementMatcher
@@ -832,19 +834,22 @@ auto buildTransferMatchSwitch() {
transferArrowOpCall(E, E->getArg(0), State);
})
- // optional::has_value
+ // optional::has_value, optional::hasValue
+ // Of the supported optionals only folly::Optional uses hasValue, but this
+ // will also pass for other types
.CaseOfCFGStmt<CXXMemberCallExpr>(
- isOptionalMemberCallWithName("has_value"),
+ isOptionalMemberCallWithNameMatcher(
+ hasAnyName("has_value", "hasValue")),
transferOptionalHasValueCall)
// optional::operator bool
.CaseOfCFGStmt<CXXMemberCallExpr>(
- isOptionalMemberCallWithName("operator bool"),
+ isOptionalMemberCallWithNameMatcher(hasName("operator bool")),
transferOptionalHasValueCall)
// optional::emplace
.CaseOfCFGStmt<CXXMemberCallExpr>(
- isOptionalMemberCallWithName("emplace"),
+ isOptionalMemberCallWithNameMatcher(hasName("emplace")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (AggregateStorageLocation *Loc =
@@ -856,7 +861,7 @@ auto buildTransferMatchSwitch() {
// optional::reset
.CaseOfCFGStmt<CXXMemberCallExpr>(
- isOptionalMemberCallWithName("reset"),
+ isOptionalMemberCallWithNameMatcher(hasName("reset")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (AggregateStorageLocation *Loc =
@@ -867,8 +872,9 @@ auto buildTransferMatchSwitch() {
})
// optional::swap
- .CaseOfCFGStmt<CXXMemberCallExpr>(isOptionalMemberCallWithName("swap"),
- transferSwapCall)
+ .CaseOfCFGStmt<CXXMemberCallExpr>(
+ isOptionalMemberCallWithNameMatcher(hasName("swap")),
+ transferSwapCall)
// std::swap
.CaseOfCFGStmt<CallExpr>(isStdSwapCall(), transferStdSwapCall)
More information about the cfe-commits
mailing list