diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/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( "bugprone-multiple-statement-macro"); + CheckFactories.registerCheck( + "bugprone-optional-value-conversion"); CheckFactories.registerCheck( "bugprone-redundant-branch-condition"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/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 diff --git a/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.h b/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.h new file mode 100644 --- /dev/null +++ b/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 + +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 getCheckTraversalKind() const override; + +private: + std::vector OptionalTypes; + std::vector ValueMethods; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_OPTIONALVALUECONVERSIONCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/OptionalValueConversionCheck.cpp @@ -0,0 +1,93 @@ +//===--- 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/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, + 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 +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))), + cxxMemberCallExpr(thisPointerType(CallTypeMatcher), + callee(cxxMethodDecl( + matchers::matchesAnyListedName(ValueMethods)))) + + ), + hasType(qualType().bind("value-type"))); + + Finder->addMatcher( + cxxConstructExpr( + argumentCountIs(1U), + hasDeclaration(cxxConstructorDecl( + ofClass(matchers::matchesAnyListedName(OptionalTypes)))), + hasType(ConstructTypeMatcher), + hasArgument(0U, ignoringImpCasts(OptionalDereferenceMatcher))) + .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"); + const auto *OptionalType = Result.Nodes.getNodeAs("optional-type"); + const auto *ValueType = Result.Nodes.getNodeAs("value-type"); + + diag(MatchedExpr->getExprLoc(), + "conversion from %0 into %1 and back into %0, remove potentially " + "error-prone optional dereference") + << *OptionalType << ValueType->getUnqualifiedType(); +} + +} // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -101,6 +101,13 @@ New checks ^^^^^^^^^^ +- New :doc:`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 ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/optional-value-conversion.rst @@ -0,0 +1,46 @@ +.. 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 + + void print(std::optional); + + int main() + { + std::optional opt; + // ... + + // Unintentional conversion from std::optional to int and back to + // std::optional: + print(opt.value()); + + // ... + } + +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$`. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -105,6 +105,7 @@ `bugprone-multiple-statement-macro `_, `bugprone-no-escape `_, `bugprone-not-null-terminated-result `_, "Yes" + `bugprone-optional-value-conversion `_, `bugprone-parent-virtual-call `_, "Yes" `bugprone-posix-return `_, "Yes" `bugprone-redundant-branch-condition `_, "Yes" diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/optional-value-conversion.cpp @@ -0,0 +1,52 @@ +// RUN: %check_clang_tidy -std=c++17-or-later %s bugprone-optional-value-conversion %t + +namespace std { + template + struct optional + { + constexpr optional() noexcept; + constexpr optional(T&&) noexcept; + constexpr optional(const T&) noexcept; + template + constexpr optional(U&&) noexcept; + const T &operator*() const; + const T &value() const; + T value_or(T) const; + }; +} + +void takeOptionalValue(std::optional); +void takeOptionalRef(const std::optional&); +void takeOtherOptional(std::optional); + +void incorrect(std::optional param) +{ + takeOptionalValue(*param); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: conversion from 'std::optional' into 'int' and back into 'std::optional', remove potentially error-prone optional dereference [bugprone-optional-value-conversion] + + takeOptionalValue(param.value()); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: conversion from 'std::optional' into 'int' and back into 'std::optional', remove potentially error-prone optional dereference [bugprone-optional-value-conversion] + + takeOptionalRef(*param); + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: conversion from 'std::optional' into 'int' and back into 'std::optional', remove potentially error-prone optional dereference [bugprone-optional-value-conversion] + + takeOptionalRef(param.value()); + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: conversion from 'std::optional' into 'int' and back into 'std::optional', remove potentially error-prone optional dereference [bugprone-optional-value-conversion] + std::optional p = *param; + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: conversion from 'std::optional' into 'int' and back into 'std::optional', remove potentially error-prone optional dereference [bugprone-optional-value-conversion] +} + +void correct(std::optional param) +{ + takeOtherOptional(*param); + + takeOtherOptional(param.value()); + + takeOtherOptional(param.value_or(5U)); + + std::optional p = *param; + + takeOptionalValue(param.value_or(5U)); + + takeOptionalRef(param.value_or(5U)); +}