PiotrZSL updated this revision to Diff 512559.
PiotrZSL marked 3 inline comments as done.
PiotrZSL added a comment.
Add fixes, add suport for std:move, fixed operator*() calls, added more tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147357/new/
https://reviews.llvm.org/D147357
Files:
clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp
clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp
@@ -0,0 +1,111 @@
+// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-optional-value-conversion %t -- --fix-notes
+
+namespace std {
+ template<typename T>
+ struct optional
+ {
+ constexpr optional() noexcept;
+ constexpr optional(T&&) noexcept;
+ constexpr optional(const T&) noexcept;
+ template<typename U>
+ constexpr optional(U&&) noexcept;
+ const T &operator*() const;
+ T &operator*();
+ const T &value() const;
+ T &value();
+ T value_or(T) const;
+ };
+
+ template <class T>
+ T&& move(T &x) {
+ return static_cast<T&&>(x);
+ }
+}
+
+void takeOptionalValue(std::optional<int>);
+void takeOptionalRef(const std::optional<int>&);
+void takeOptionalRRef(std::optional<int>&&);
+void takeOtherOptional(std::optional<long>);
+
+void incorrect(std::optional<int> param)
+{
+ std::optional<int>* ptr = ¶m;
+ takeOptionalValue(**ptr);
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalValue(*ptr);
+ takeOptionalValue(*param);
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalValue(param);
+ takeOptionalValue(param.value());
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalValue(param);
+ takeOptionalValue(param.operator*());
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalValue(param);
+ takeOptionalRef(*param);
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalRef(param);
+ takeOptionalRef(param.value());
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalRef(param);
+ takeOptionalRef(param.operator*());
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalRef(param);
+ std::optional<int> p = *param;
+ // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: std::optional<int> p = param;
+
+ takeOptionalValue(std::move(**ptr));
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalValue(std::move(*ptr));
+ takeOptionalValue(std::move(*param));
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalValue(std::move(param));
+ takeOptionalValue(std::move(param.value()));
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalValue(std::move(param));
+ takeOptionalValue(std::move(param.operator*()));
+ // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalValue(std::move(param));
+ takeOptionalRef(std::move(*param));
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalRef(std::move(param));
+ takeOptionalRef(std::move(param.value()));
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalRef(std::move(param));
+ takeOptionalRef(std::move(param.operator*()));
+ // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalRef(std::move(param));
+ takeOptionalRRef(std::move(*param));
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalRRef(std::move(param));
+ takeOptionalRRef(std::move(param.value()));
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalRRef(std::move(param));
+ takeOptionalRRef(std::move(param.operator*()));
+ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: takeOptionalRRef(std::move(param));
+ std::optional<int> p2 = std::move(*param);
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: conversion from 'std::optional<int>' into 'int' and back into 'std::optional<int>', remove potentially error-prone optional dereference [bugprone-optional-value-conversion]
+ // CHECK-FIXES: std::optional<int> p2 = std::move(param);
+}
+
+void correct(std::optional<int> param)
+{
+ takeOtherOptional(*param);
+ takeOtherOptional(param.value());
+ takeOtherOptional(param.value_or(5U));
+ takeOtherOptional(param.operator*());
+
+ std::optional<long> p = *param;
+ takeOptionalValue(param.value_or(5U));
+ takeOptionalRef(param.value_or(5U));
+/*
+ std::optional<int>* ptr = ¶m;
+ takeOptionalValue(ptr->value());
+ takeOptionalValue(ptr->operator*());
+ takeOptionalRef(ptr->value());
+ takeOptionalRef(ptr->operator*());
+ takeOptionalRRef(std::move(ptr->value()));
+ takeOptionalRRef(std::move(ptr->operator*()));*/
+}
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -105,6 +105,7 @@
`bugprone-multiple-statement-macro <bugprone/multiple-statement-macro.html>`_,
`bugprone-no-escape <bugprone/no-escape.html>`_,
`bugprone-not-null-terminated-result <bugprone/not-null-terminated-result.html>`_, "Yes"
+ `bugprone-optional-value-conversion <bugprone/optional-value-conversion.html>`_,
`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"
Index: clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst
@@ -0,0 +1,48 @@
+.. 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.
+
+.. 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());
+
+ // ...
+ }
+
+Value extraction using ``operator *`` is matched by default.
+
+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$`.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,13 @@
New checks
^^^^^^^^^^
+- 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:`bugprone-unsafe-functions
<clang-tidy/checks/bugprone/unsafe-functions>` check.
Index: clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.h
===================================================================
--- /dev/null
+++ 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
Index: clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp
@@ -0,0 +1,120 @@
+//===--- 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"),
+ cxxMemberCallExpr(thisPointerType(CallTypeMatcher),
+ callee(cxxMethodDecl(anyOf(
+ hasOverloadedOperatorName("*"),
+ matchers::matchesAnyListedName(ValueMethods)))))
+ .bind("fun")),
+ 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")) {
+ 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>("fun")) {
+ const SourceLocation Begin =
+ utils::lexer::getPreviousToken(CallExpr->getExprLoc(),
+ *Result.SourceManager, getLangOpts())
+ .getLocation();
+ diag(CallExpr->getExprLoc(), "remove call to %0 to silence this warning",
+ DiagnosticIDs::Note)
+ << CallExpr->getMethodDecl()
+ << FixItHint::CreateRemoval(
+ CharSourceRange::getTokenRange(Begin, CallExpr->getEndLoc()));
+ return;
+ }
+}
+
+} // namespace clang::tidy::bugprone
Index: clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -34,6 +34,7 @@
MultipleStatementMacroCheck.cpp
NoEscapeCheck.cpp
NotNullTerminatedResultCheck.cpp
+ OptionalValueConversionCheck.cpp
ParentVirtualCallCheck.cpp
PosixReturnCheck.cpp
RedundantBranchConditionCheck.cpp
Index: clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -39,6 +39,7 @@
#include "MultipleStatementMacroCheck.h"
#include "NoEscapeCheck.h"
#include "NotNullTerminatedResultCheck.h"
+#include "OptionalValueConversionCheck.h"
#include "ParentVirtualCallCheck.h"
#include "PosixReturnCheck.h"
#include "RedundantBranchConditionCheck.h"
@@ -134,6 +135,8 @@
"bugprone-move-forwarding-reference");
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>(
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
