diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index d7a525b99fbcf..7c4d06b0d308f 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 ` check to properly handle calls - to ``std::forward``. + to ``std::forward`` and support for ``folly::Optional`` were added. - Extend :doc:`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 47365185e42b0..5a6aaa077d9bf 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``, ``absl::optional``, or ``base::Optional`` -objects. Below we will refer to all these types collectively as ``optional``. +``std::optional``, ``absl::optional``, ``base::Optional``, or +``folly::Optional`` objects. Below we will refer to all these types +collectively as ``optional``. An access to the value of an ``optional`` 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 0000000000000..818b06623cb7f --- /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 +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 + constexpr T value_or(U &&v) const &; + template + T value_or(U &&v) &&; + + template + 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 c1e731f411719..1921291f2187d 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 &opt) { opt.value(); @@ -21,12 +22,34 @@ void unchecked_arrow_operator_access(const absl::optional &opt) { // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value } +void folly_check_value_then_reset(folly::Optional 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 opt1, folly::Optional 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 &opt) { if (opt.has_value()) { opt.value(); } } +void folly_checked_access(const folly::Optional &opt) { + if (opt.hasValue()) { + opt.value(); + } +} + template void function_template_without_user(const absl::optional &opt) { opt.value(); // no-warning diff --git a/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp b/clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp index 1db255dd81f3f..b0a8667f3fe5b 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(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 matcher, const std::optional &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 &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( - isOptionalMemberCallWithName("has_value"), + isOptionalMemberCallWithNameMatcher( + hasAnyName("has_value", "hasValue")), transferOptionalHasValueCall) // optional::operator bool .CaseOfCFGStmt( - isOptionalMemberCallWithName("operator bool"), + isOptionalMemberCallWithNameMatcher(hasName("operator bool")), transferOptionalHasValueCall) // optional::emplace .CaseOfCFGStmt( - 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( - 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(isOptionalMemberCallWithName("swap"), - transferSwapCall) + .CaseOfCFGStmt( + isOptionalMemberCallWithNameMatcher(hasName("swap")), + transferSwapCall) // std::swap .CaseOfCFGStmt(isStdSwapCall(), transferStdSwapCall)