Commit 2f0630f8 authored by Anton Dukeman's avatar Anton Dukeman Committed by Piotr Zegar
Browse files

[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
parent a6cd1f62

clang-tools-extra/docs/ReleaseNotes.rst

+1 −1
Original line number Diff line number Diff line
@@ -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

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

+3 −2
Original line number Diff line number Diff line
@@ -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

clang-tools-extra/test/clang-tidy/checkers/bugprone/Inputs/unchecked-optional-access/folly/types/Optional.h

0 → 100644
+56 −0
Original line number Diff line number Diff line 
#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_

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

+23 −0
Original line number Diff line number Diff line 
// 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

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

+27 −21
Original line number Diff line number Diff line
@@ -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"))),

  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,7 +872,8 @@ auto buildTransferMatchSwitch() { 
          })



      // optional::swap

      .CaseOfCFGStmt<CXXMemberCallExpr>(isOptionalMemberCallWithName("swap"),

      .CaseOfCFGStmt<CXXMemberCallExpr>(

          isOptionalMemberCallWithNameMatcher(hasName("swap")),

          transferSwapCall)



      // std::swap