[clang-tidy] Add bugprone-optional-value-conversion check

Detects potentially unintentional and redundant conversions where a value is
extracted from an optional-like type and then used to create a new instance of
the same optional-like type.

Reviewed By: xgupta

Differential Revision: https://reviews.llvm.org/D147357

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

@@ -44,6 +44,7 @@
 #include "NoEscapeCheck.h"
 #include "NonZeroEnumToBoolConversionCheck.h"
 #include "NotNullTerminatedResultCheck.h"
+#include "OptionalValueConversionCheck.h"
 #include "ParentVirtualCallCheck.h"
 #include "PosixReturnCheck.h"
 #include "RedundantBranchConditionCheck.h"
@@ -150,6 +151,8 @@ class BugproneModule : public ClangTidyModule {
         "bugprone-multiple-new-in-one-expression");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
+    CheckFactories.registerCheck<OptionalValueConversionCheck>(
+        "bugprone-optional-value-conversion");
     CheckFactories.registerCheck<RedundantBranchConditionCheck>(
         "bugprone-redundant-branch-condition");
     CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(

clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt

@@ -40,6 +40,7 @@ add_clang_library(clangTidyBugproneModule
   NoEscapeCheck.cpp
   NonZeroEnumToBoolConversionCheck.cpp
   NotNullTerminatedResultCheck.cpp
+  OptionalValueConversionCheck.cpp
   ParentVirtualCallCheck.cpp
   PosixReturnCheck.cpp
   RedundantBranchConditionCheck.cpp

clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp

@@ -0,0 +1,126 @@
+//===--- OptionalValueConversionCheck.cpp - clang-tidy --------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "OptionalValueConversionCheck.h"
+#include "../utils/LexerUtils.h"
+#include "../utils/Matchers.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+
+AST_MATCHER_P(QualType, hasCleanType, ast_matchers::internal::Matcher<QualType>,
+              InnerMatcher) {
+  return InnerMatcher.matches(
+      Node.getNonReferenceType().getUnqualifiedType().getCanonicalType(),
+      Finder, Builder);
+}
+
+} // namespace
+
+OptionalValueConversionCheck::OptionalValueConversionCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      OptionalTypes(utils::options::parseStringList(
+          Options.get("OptionalTypes",
+                      "::std::optional;::absl::optional;::boost::optional"))),
+      ValueMethods(utils::options::parseStringList(
+          Options.get("ValueMethods", "::value$;::get$"))) {}
+
+std::optional<TraversalKind>
+OptionalValueConversionCheck::getCheckTraversalKind() const {
+  return TK_AsIs;
+}
+
+void OptionalValueConversionCheck::registerMatchers(MatchFinder *Finder) {
+  auto ConstructTypeMatcher =
+      qualType(hasCleanType(qualType().bind("optional-type")));
+
+  auto CallTypeMatcher =
+      qualType(hasCleanType(equalsBoundNode("optional-type")));
+
+  auto OptionalDereferenceMatcher = callExpr(
+      anyOf(
+          cxxOperatorCallExpr(hasOverloadedOperatorName("*"),
+                              hasUnaryOperand(hasType(CallTypeMatcher)))
+              .bind("op-call"),
+          cxxMemberCallExpr(thisPointerType(CallTypeMatcher),
+                            callee(cxxMethodDecl(anyOf(
+                                hasOverloadedOperatorName("*"),
+                                matchers::matchesAnyListedName(ValueMethods)))))
+              .bind("member-call")),
+      hasType(qualType().bind("value-type")));
+
+  auto StdMoveCallMatcher =
+      callExpr(argumentCountIs(1), callee(functionDecl(hasName("::std::move"))),
+               hasArgument(0, ignoringImpCasts(OptionalDereferenceMatcher)));
+  Finder->addMatcher(
+      cxxConstructExpr(
+          argumentCountIs(1U),
+          hasDeclaration(cxxConstructorDecl(
+              ofClass(matchers::matchesAnyListedName(OptionalTypes)))),
+          hasType(ConstructTypeMatcher),
+          hasArgument(0U, ignoringImpCasts(anyOf(OptionalDereferenceMatcher,
+                                                 StdMoveCallMatcher))))
+          .bind("expr"),
+      this);
+}
+
+void OptionalValueConversionCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "OptionalTypes",
+                utils::options::serializeStringList(OptionalTypes));
+  Options.store(Opts, "ValueMethods",
+                utils::options::serializeStringList(ValueMethods));
+}
+
+void OptionalValueConversionCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedExpr = Result.Nodes.getNodeAs<Expr>("expr");
+  const auto *OptionalType = Result.Nodes.getNodeAs<QualType>("optional-type");
+  const auto *ValueType = Result.Nodes.getNodeAs<QualType>("value-type");
+
+  diag(MatchedExpr->getExprLoc(),
+       "conversion from %0 into %1 and back into %0, remove potentially "
+       "error-prone optional dereference")
+      << *OptionalType << ValueType->getUnqualifiedType();
+
+  if (const auto *OperatorExpr =
+          Result.Nodes.getNodeAs<CXXOperatorCallExpr>("op-call")) {
+    diag(OperatorExpr->getExprLoc(), "remove '*' to silence this warning",
+         DiagnosticIDs::Note)
+        << FixItHint::CreateRemoval(CharSourceRange::getTokenRange(
+               OperatorExpr->getBeginLoc(), OperatorExpr->getExprLoc()));
+    return;
+  }
+  if (const auto *CallExpr =
+          Result.Nodes.getNodeAs<CXXMemberCallExpr>("member-call")) {
+    const SourceLocation Begin =
+        utils::lexer::getPreviousToken(CallExpr->getExprLoc(),
+                                       *Result.SourceManager, getLangOpts())
+            .getLocation();
+    auto Diag =
+        diag(CallExpr->getExprLoc(),
+             "remove call to %0 to silence this warning", DiagnosticIDs::Note);
+    Diag << CallExpr->getMethodDecl()
+         << FixItHint::CreateRemoval(
+                CharSourceRange::getTokenRange(Begin, CallExpr->getEndLoc()));
+    if (const auto *Member =
+            llvm::dyn_cast<MemberExpr>(CallExpr->getCallee()->IgnoreImplicit());
+        Member && Member->isArrow())
+      Diag << FixItHint::CreateInsertion(CallExpr->getBeginLoc(), "*");
+    return;
+  }
+}
+
+} // namespace clang::tidy::bugprone

clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.h

@@ -0,0 +1,38 @@
+//===--- OptionalValueConversionCheck.h - clang-tidy ------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_OPTIONALVALUECONVERSIONCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_OPTIONALVALUECONVERSIONCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include <vector>
+
+namespace clang::tidy::bugprone {
+
+/// Detects potentially unintentional and redundant conversions where a value is
+/// extracted from an optional-like type and then used to create a new instance
+/// of the same optional-like type.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/optional-value-conversion.html
+class OptionalValueConversionCheck : public ClangTidyCheck {
+public:
+  OptionalValueConversionCheck(StringRef Name, ClangTidyContext *Context);
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  std::optional<TraversalKind> getCheckTraversalKind() const override;
+
+private:
+  std::vector<StringRef> OptionalTypes;
+  std::vector<StringRef> ValueMethods;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_OPTIONALVALUECONVERSIONCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -129,6 +129,13 @@ New checks
   Detects implicit conversions between pointers of different levels of
   indirection.
 
+- New :doc:`bugprone-optional-value-conversion
+  <clang-tidy/checks/bugprone/optional-value-conversion>` check.
+
+  Detects potentially unintentional and redundant conversions where a value is
+  extracted from an optional-like type and then used to create a new instance
+  of the same optional-like type.
+
 - New :doc:`modernize-use-constraints
   <clang-tidy/checks/modernize/use-constraints>` check.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst

@@ -0,0 +1,76 @@
+.. title:: clang-tidy - bugprone-optional-value-conversion
+
+bugprone-optional-value-conversion
+==================================
+
+Detects potentially unintentional and redundant conversions where a value is
+extracted from an optional-like type and then used to create a new instance of
+the same optional-like type.
+
+These conversions might be the result of developer oversight, leftovers from
+code refactoring, or other situations that could lead to unintended exceptions
+or cases where the resulting optional is always initialized, which might be
+unexpected behavior.
+
+To illustrate, consider the following problematic code snippet:
+
+.. code-block:: c++
+
+    #include <optional>
+
+    void print(std::optional<int>);
+
+    int main()
+    {
+      std::optional<int> opt;
+      // ...
+
+      // Unintentional conversion from std::optional<int> to int and back to
+      // std::optional<int>:
+      print(opt.value());
+
+      // ...
+    }
+
+A better approach would be to directly pass ``opt`` to the ``print`` function
+without extracting its value:
+
+.. code-block:: c++
+
+    #include <optional>
+
+    void print(std::optional<int>);
+
+    int main()
+    {
+      std::optional<int> opt;
+      // ...
+
+      // Proposed code: Directly pass the std::optional<int> to the print
+      // function.
+      print(opt);
+
+      // ...
+    }
+
+By passing ``opt`` directly to the print function, unnecessary conversions are
+avoided, and potential unintended behavior or exceptions are minimized.
+
+Value extraction using ``operator *`` is matched by default.
+The support for non-standard optional types such as ``boost::optional`` or
+``absl::optional`` may be limited.
+
+Options:
+--------
+
+.. option:: OptionalTypes
+
+    Semicolon-separated list of (fully qualified) optional type names or regular
+    expressions that match the optional types.
+    Default value is `::std::optional;::absl::optional;::boost::optional`.
+
+.. option:: ValueMethods
+
+    Semicolon-separated list of (fully qualified) method names or regular
+    expressions that match the methods.
+    Default value is `::value$;::get$`.

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -110,6 +110,7 @@ Clang-Tidy Checks
    `bugprone-no-escape <bugprone/no-escape.html>`_,
    `bugprone-non-zero-enum-to-bool-conversion <bugprone/non-zero-enum-to-bool-conversion.html>`_,
    `bugprone-not-null-terminated-result <bugprone/not-null-terminated-result.html>`_, "Yes"
+   `bugprone-optional-value-conversion <bugprone/optional-value-conversion.html>`_, "Yes"
    `bugprone-parent-virtual-call <bugprone/parent-virtual-call.html>`_, "Yes"
    `bugprone-posix-return <bugprone/posix-return.html>`_, "Yes"
    `bugprone-redundant-branch-condition <bugprone/redundant-branch-condition.html>`_, "Yes"