This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2f0630f8bc91: [clang-tidy] Add folly::Optional to
unchecked-optional-access (authored by Anton Dukeman
<antonduke...@fb.com>, committed by PiotrZSL).
Changed prior to commit:
https://reviews.llvm.org/D155890?vs=543659&id=543678#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155890/new/
https://reviews.llvm.org/D155890
Files:
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/Inputs/unchecked-optional-access/folly/types/Optional.h
clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
Index: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
===================================================================
--- clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
+++ clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp
@@ -56,9 +56,10 @@
}
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 @@
/// 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 @@
on(expr(Exception,
anyOf(hasOptionalType(),
hasType(pointerType(pointee(optionalOrAliasType())))))),
- callee(cxxMethodDecl(hasName(MemberName))));
+ callee(cxxMethodDecl(matcher)));
}
auto isOptionalOperatorCallWithName(
@@ -109,15 +110,15 @@
}
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 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 @@
StatementMatcher
valueCall(const std::optional<StatementMatcher> &IgnorableOptional) {
- return isOptionalMemberCallWithName("value", IgnorableOptional);
+ return isOptionalMemberCallWithNameMatcher(hasName("value"),
+ IgnorableOptional);
}
StatementMatcher
@@ -832,19 +834,22 @@
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 @@
// optional::reset
.CaseOfCFGStmt<CXXMemberCallExpr>(
- isOptionalMemberCallWithName("reset"),
+ isOptionalMemberCallWithNameMatcher(hasName("reset")),
[](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
LatticeTransferState &State) {
if (AggregateStorageLocation *Loc =
@@ -867,8 +872,9 @@
})
// optional::swap
- .CaseOfCFGStmt<CXXMemberCallExpr>(isOptionalMemberCallWithName("swap"),
- transferSwapCall)
+ .CaseOfCFGStmt<CXXMemberCallExpr>(
+ isOptionalMemberCallWithNameMatcher(hasName("swap")),
+ transferSwapCall)
// std::swap
.CaseOfCFGStmt<CallExpr>(isStdSwapCall(), transferStdSwapCall)
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++ 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 @@
// 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
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/folly/types/Optional.h
===================================================================
--- /dev/null
+++ 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_
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
@@ -8,8 +8,9 @@
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
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -319,7 +319,7 @@
- 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
