[clang-tidy] Add folly::Optional to unchecked-optional-access

[adukeman]

Summary

The unchecked-optional-access check identifies attempted value
unwrapping without checking if the value exists. These changes extend
that support to checking folly::Optional.

[PiotrZSL]

To be honest, all those things just should be configurable instead of hardcoding them.
For example, a project that I work for got 4 different custom optional implementations, and this check is useless for all of them.
And still no boost::optional support.

[gribozavr2]

Thank you! Do you have commit access or do you need me to commit this patch for you?

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

845

[carlosgalvezp]

This should be a configuration option, we should not hardcore project-specific things in the source code.

[ymandel]

In D155890#4521266, @carlosgalvezp wrote:

This should be a configuration option, we should not hardcore project-specific things in the source code.

I agree, but we already are hardcoding specific types -- I think this is a separate (and valid) critique of the design. I'd propose filing an issue on the github tracker and we can follow up there. I, for one, would love to review such a change but don't have the time to write it.

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

846–848

A few concerns:

1. This will allow hasValue on *any* of the optional types. While we know that the other types don't have this call, this is bad hygiene. At the least, we should note this potential problem in the comments.
2. I don't think its worth duplicating the case above just to change the name, given that the action is identical. Instead, please generalize isOptionalMemberCallWithName to take a name matcher, and pass hasAnyName("has_value", "hasValue") for this case. The other calls to isOptionalMemberCallWithName will need to be changed to pass just hasName(...).

[adukeman]

In D155890#4522266, @ymandel wrote:

In D155890#4521266, @carlosgalvezp wrote:

This should be a configuration option, we should not hardcore project-specific things in the source code.

I agree, but we already are hardcoding specific types -- I think this is a separate (and valid) critique of the design. I'd propose filing an issue on the github tracker and we can follow up there. I, for one, would love to review such a change but don't have the time to write it.

Is moving these values to config an appropriate task for somebody like me new to working on clang-tidy? I'd be happy to merge this and then try the transition to a config assuming there's some similar examples I can borrow from elsewhere in the codebase.

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

846–848

Sure. I can make that change.

[ymandel]

In D155890#4523243, @adukeman wrote:

In D155890#4522266, @ymandel wrote:

In D155890#4521266, @carlosgalvezp wrote:

This should be a configuration option, we should not hardcore project-specific things in the source code.

I agree, but we already are hardcoding specific types -- I think this is a separate (and valid) critique of the design. I'd propose filing an issue on the github tracker and we can follow up there. I, for one, would love to review such a change but don't have the time to write it.

Is moving these values to config an appropriate task for somebody like me new to working on clang-tidy? I'd be happy to merge this and then try the transition to a config assuming there's some similar examples I can borrow from elsewhere in the codebase.

This is one of the most complex clang-tidy checks. So, if you're looking for a CT starter task, I wouldn't recommend this particular challenge. That said, I think the clang-tidy side will be relatively easy -- CT has a mature config system/API. The harder part (and not CT relevant) is refactoring this code to consume that config. It's not terribly complicated but will require a bunch of changes and probably some design questions.

[gribozavr2]

In D155890#4523243, @adukeman wrote:

In D155890#4522266, @ymandel wrote:

In D155890#4521266, @carlosgalvezp wrote:

This should be a configuration option, we should not hardcore project-specific things in the source code.

I agree, but we already are hardcoding specific types -- I think this is a separate (and valid) critique of the design. I'd propose filing an issue on the github tracker and we can follow up there. I, for one, would love to review such a change but don't have the time to write it.

Is moving these values to config an appropriate task for somebody like me new to working on clang-tidy? I'd be happy to merge this and then try the transition to a config assuming there's some similar examples I can borrow from elsewhere in the codebase.

I think it can be a good starter task for a new engineer on the project. However, don't underestimate this problem, it will require the code to be refactored a little bit. For example, the function hasOptionalClassName needs restructuring so that it can accept class names from a list. Not a lot of work, but it isn't mechanically replacing string literals with a variable either.

[carlosgalvezp]

In D155890#4523262, @gribozavr2 wrote:

In D155890#4523243, @adukeman wrote:

In D155890#4522266, @ymandel wrote:

In D155890#4521266, @carlosgalvezp wrote:

This should be a configuration option, we should not hardcore project-specific things in the source code.

I agree, but we already are hardcoding specific types -- I think this is a separate (and valid) critique of the design. I'd propose filing an issue on the github tracker and we can follow up there. I, for one, would love to review such a change but don't have the time to write it.

Is moving these values to config an appropriate task for somebody like me new to working on clang-tidy? I'd be happy to merge this and then try the transition to a config assuming there's some similar examples I can borrow from elsewhere in the codebase.

I think it can be a good starter task for a new engineer on the project. However, don't underestimate this problem, it will require the code to be refactored a little bit. For example, the function hasOptionalClassName needs restructuring so that it can accept class names from a list. Not a lot of work, but it isn't mechanically replacing string literals with a variable either.

Indeed this is not "the standard" CT check, the core is part of Clang so I think it'd be good to add reviewers there as well in case this affects other parts of the codebase. In that sense it does not seen as trivial as I thought to make this user configurable, so perhaps opening a ticket and solve it there is a faster way forward.

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

62

If there's no need for absl here, why do we need to add folly?

[carlosgalvezp]

I think it'd be good to add reviewers there

I realize the CodeOwners for Analysis are already in the list of reviewers, I won't interfere then :)

[gribozavr2]

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

62

This is the branch for RD.getName() == "Optional" with a capital "O" because base::Optional, folly;:Optional are spelled with a capital "O".

Abseil provides absl::optional and it is handled under RD.getName() == "optional" together with std::optional above.

[carlosgalvezp]

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

62

Got it, thanks!

[carlosgalvezp]

I have opened a refactoring ticket here: https://github.com/llvm/llvm-project/issues/64037

[ymandel]

In D155890#4525144, @carlosgalvezp wrote:

I have opened a refactoring ticket here: https://github.com/llvm/llvm-project/issues/64037

Thank you!

[carlosgalvezp]

The review is marked as accepted, should we land it? Let me know if you need help with that :)

[ymandel]

In D155890#4528560, @carlosgalvezp wrote:

The review is marked as accepted, should we land it? Let me know if you need help with that :)

We're still waiting on some small changes: https://reviews.llvm.org/D155890#inline-1508661

[adukeman]

Update calls to OptionalMemberCall to take a name matcher

Supports treating calls to optional::has_value and optional::hasValue identically

[adukeman]

Thanks for all the comments! I think I've got them all addressed and tests are passing locally.

In D155890#4528560, @carlosgalvezp wrote:

The review is marked as accepted, should we land it? Let me know if you need help with that :)

Yes, will need help with that since I don't have commit access. Anybody with commit access, feel free to land this for me.

[PiotrZSL]

Some entry in release notes or/and check documentation would be welcome.

[adukeman]

Update docs and run clang-format

[PiotrZSL]

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

10–11

80 characters limit, wrap it

[PiotrZSL]

+- LGTM, I will correct those nits and push it.

clang-tools-extra/docs/ReleaseNotes.rst

- Deprecated check-local options `HeaderFileExtensions` and `ImplementationFileExtensions`
   Global options of the same name should be used instead.
 
 - Improved :doc:`bugprone-too-small-loop-variable
   <clang-tidy/checks/bugprone/too-small-loop-variable>` check. Basic support
   for bit-field and integer members as a loop variable or upper limit were added.
 
 - 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
   with specified return types using the ``CheckedReturnTypes`` option.
 
 - Improved :doc:`bugprone-use-after-move
   <clang-tidy/checks/bugprone/use-after-move>` check. Detect uses and moves in
   constructor initializers. Correctly handle constructor arguments as being

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

 .. title:: clang-tidy - bugprone-unchecked-optional-access
 
 bugprone-unchecked-optional-access
 ==================================
 
 *Note*: This check uses a flow-sensitive static analysis to produce its
 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
 common misconceptions, the check considers these member functions as equivalent,
 even though there are subtle differences related to exceptions versus undefined
 behavior. See *Additional notes*, below, for more information on this topic.
 
 An access to the value of an ``optional<T>`` is considered safe if and only if

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

+#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

 // 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();
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: unchecked access to optional value [bugprone-unchecked-optional-access]
 }
 
 void unchecked_deref_operator_access(const absl::optional<int> &opt) {
   *opt;
   // CHECK-MESSAGES: :[[@LINE-1]]:4: warning: unchecked access to optional value
 }
 
 struct Foo {
   void foo() const {}
 };
 
 void unchecked_arrow_operator_access(const absl::optional<Foo> &opt) {
   opt->foo();
   // 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
 }
 
 template <typename T>
 void function_template_with_user(const absl::optional<T> &opt) {
   opt.value();

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

static bool hasOptionalClassName(const CXXRecordDecl &RD) {
 
   if (RD.getName() == "optional") {
     if (const auto *N = dyn_cast_or_null<NamespaceDecl>(RD.getDeclContext()))
       return N->isStdNamespace() || isTopLevelNamespaceWithName(*N, "absl");
     return false;
   }
 
   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;
 }
 
 namespace {
 
 using namespace ::clang::ast_matchers;

 auto optionalOrAliasType() {
   return hasUnqualifiedDesugaredType(
       recordType(hasDeclaration(optionalClass())));
 }
 
 /// 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());
   return cxxMemberCallExpr(
       on(expr(Exception,
               anyOf(hasOptionalType(),
                     hasType(pointerType(pointee(optionalOrAliasType())))))),
-      callee(cxxMethodDecl(hasName(MemberName))));
+      callee(cxxMethodDecl(matcher)));
 }
 
 auto isOptionalOperatorCallWithName(
     llvm::StringRef operator_name,
     const std::optional<StatementMatcher> &Ignorable = std::nullopt) {
   return cxxOperatorCallExpr(
       hasOverloadedOperatorName(operator_name),
       callee(cxxMethodDecl(ofClass(optionalClass()))),
       Ignorable ? callExpr(unless(hasArgument(0, *Ignorable))) : callExpr());
 }
 
 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()); }
 
 // `optional` or `nullopt_t`
 auto hasAnyOptionalType() {
   return hasType(hasUnqualifiedDesugaredType(
       recordType(hasDeclaration(anyOf(nulloptTypeDecl(), optionalClass())))));
 }
 
 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() {
   return cxxConstructExpr(
       hasOptionalType(),
       hasDeclaration(cxxConstructorDecl(parameterCountIs(1),
                                         hasParameter(0, hasNulloptType()))));
 }
if (Options.IgnoreSmartPointerDereference) {
     return expr(
         anyOf(SmartPtrUse, memberExpr(hasObjectExpression(SmartPtrUse))));
   }
   return std::nullopt;
 }
 
 StatementMatcher
 valueCall(const std::optional<StatementMatcher> &IgnorableOptional) {
-  return isOptionalMemberCallWithName("value", IgnorableOptional);
+  return isOptionalMemberCallWithNameMatcher(hasName("value"),
+                                             IgnorableOptional);
 }
 
 StatementMatcher
 valueOperatorCall(const std::optional<StatementMatcher> &IgnorableOptional) {
   return expr(anyOf(isOptionalOperatorCallWithName("*", IgnorableOptional),
                     isOptionalOperatorCallWithName("->", IgnorableOptional)));
 }
 
return CFGMatchSwitchBuilder<LatticeTransferState>()
       // optional::operator->
       .CaseOfCFGStmt<CallExpr>(isOptionalOperatorCallWithName("->"),
                                [](const CallExpr *E,
                                   const MatchFinder::MatchResult &,
                                   LatticeTransferState &State) {
                                  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 =
                     getImplicitObjectLocation(*E, State.Env)) {
               createOptionalValue(*Loc, State.Env.getBoolLiteralValue(true),
                                   State.Env);
             }
           })
 
       // optional::reset
       .CaseOfCFGStmt<CXXMemberCallExpr>(
-          isOptionalMemberCallWithName("reset"),
+          isOptionalMemberCallWithNameMatcher(hasName("reset")),
           [](const CXXMemberCallExpr *E, const MatchFinder::MatchResult &,
              LatticeTransferState &State) {
             if (AggregateStorageLocation *Loc =
                     getImplicitObjectLocation(*E, State.Env)) {
               createOptionalValue(*Loc, State.Env.getBoolLiteralValue(false),
                                   State.Env);
             }
           })
 
       // optional::swap
-      .CaseOfCFGStmt<CXXMemberCallExpr>(isOptionalMemberCallWithName("swap"),
-                                        transferSwapCall)
+      .CaseOfCFGStmt<CXXMemberCallExpr>(
+          isOptionalMemberCallWithNameMatcher(hasName("swap")),
 
       // std::swap
       .CaseOfCFGStmt<CallExpr>(isStdSwapCall(), transferStdSwapCall)
 
       // std::forward
       .CaseOfCFGStmt<CallExpr>(isStdForwardCall(), transferStdForwardCall)
 
       // opt.value_or("").empty()