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 @@ -20,6 +20,7 @@ #include "DanglingHandleCheck.h" #include "DynamicStaticInitializersCheck.h" #include "EasilySwappableParametersCheck.h" +#include "EnumToBoolConversionCheck.h" #include "ExceptionEscapeCheck.h" #include "FoldInitTypeCheck.h" #include "ForwardDeclarationNamespaceCheck.h" @@ -102,6 +103,8 @@ "bugprone-dynamic-static-initializers"); CheckFactories.registerCheck( "bugprone-easily-swappable-parameters"); + CheckFactories.registerCheck( + "bugprone-enum-to-bool-conversion"); CheckFactories.registerCheck( "bugprone-exception-escape"); CheckFactories.registerCheck("bugprone-fold-init-type"); 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 @@ -15,6 +15,7 @@ DanglingHandleCheck.cpp DynamicStaticInitializersCheck.cpp EasilySwappableParametersCheck.cpp + EnumToBoolConversionCheck.cpp ExceptionEscapeCheck.cpp FoldInitTypeCheck.cpp ForwardDeclarationNamespaceCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/EnumToBoolConversionCheck.h b/clang-tools-extra/clang-tidy/bugprone/EnumToBoolConversionCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/EnumToBoolConversionCheck.h @@ -0,0 +1,36 @@ +//===--- EnumToBoolConversionCheck.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_ENUMTOBOOLCONVERSIONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOBOOLCONVERSIONCHECK_H + +#include "../ClangTidyCheck.h" +#include + +namespace clang::tidy::bugprone { + +/// Warns about `enum` to `bool` conversions where `enum` doesn't have a +/// zero-value enumerator. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/enum-to-bool-conversion.html +class EnumToBoolConversionCheck : public ClangTidyCheck { +public: + EnumToBoolConversionCheck(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; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override; + +private: + const std::vector EnumIgnoreList; +}; + +} // namespace clang::tidy::bugprone + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOBOOLCONVERSIONCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/EnumToBoolConversionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/EnumToBoolConversionCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/EnumToBoolConversionCheck.cpp @@ -0,0 +1,76 @@ +//===--- EnumToBoolConversionCheck.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 "EnumToBoolConversionCheck.h" +#include "../utils/Matchers.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +namespace { + +AST_MATCHER(EnumDecl, isCompleteAndHasNoZeroValue) { + const auto *Definition = Node.getDefinition(); + return Definition and Node.isComplete() and + std::none_of( + Definition->enumerator_begin(), Definition->enumerator_end(), + [](const auto *Value) { return Value->getInitVal().isZero(); }); +} + +} // namespace + +EnumToBoolConversionCheck::EnumToBoolConversionCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + EnumIgnoreList( + utils::options::parseStringList(Options.get("EnumIgnoreList", ""))) {} + +void EnumToBoolConversionCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "EnumIgnoreList", + utils::options::serializeStringList(EnumIgnoreList)); +} + +bool EnumToBoolConversionCheck::isLanguageVersionSupported( + const LangOptions &LangOpts) const { + return LangOpts.CPlusPlus; +} + +void EnumToBoolConversionCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + castExpr(hasCastKind(CK_IntegralToBoolean), + unless(isExpansionInSystemHeader()), hasType(booleanType()), + hasSourceExpression( + expr(hasType(qualType(hasCanonicalType(hasDeclaration( + enumDecl(isCompleteAndHasNoZeroValue(), + unless(matchers::matchesAnyListedName( + EnumIgnoreList))) + .bind("enum"))))), + unless(declRefExpr(to(enumConstantDecl()))))), + unless(hasAncestor(staticAssertDecl()))) + .bind("cast"), + this); +} + +void EnumToBoolConversionCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Cast = Result.Nodes.getNodeAs("cast"); + const auto *Enum = Result.Nodes.getNodeAs("enum"); + + diag(Cast->getExprLoc(), "conversion of %0 into 'bool' will always return " + "'true', enum doesn't have a zero-value enumerator") + << Enum; + + diag(Enum->getLocation(), "enum is defined here", DiagnosticIDs::Note); +} + +} // 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 @@ -109,6 +109,12 @@ This check relies heavily on, but is not exclusive to, the functions from the *Annex K. "Bounds-checking interfaces"* of C11. +- New :doc:`bugprone-enum-to-bool-conversion + ` check. + + Detect implicit and explicit casts of ``enum`` type into ``bool`` where ``enum`` type + doesn't have a zero-value enumerator. + - New :doc:`cppcoreguidelines-avoid-capture-default-when-capturing-this ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst @@ -0,0 +1,58 @@ +.. title:: clang-tidy - bugprone-enum-to-bool-conversion + +bugprone-enum-to-bool-conversion +================================ + +Detect implicit and explicit casts of ``enum`` type into ``bool`` where ``enum`` +type doesn't have a zero-value enumerator. If the ``enum`` is used only to hold +values equal to its enumerators, then conversion to ``bool`` will always result +in ``true`` value. This can lead to unnecessary code that reduces readability +and maintainability and can result in bugs. + +The check produces false positives if the ``enum`` is used to store other values +(used as a bit-mask or zero-initialized on purpose). To deal with false-positives, +``//NOLINT`` or casting first to the underlying type before casting to ``bool`` +can be used. + +It is important to note that this check will not generate warnings if the +definition of the enumeration type is not available. +Additionally, C++11 enumeration classes are supported by this check. + +Overall, this check serves to improve code quality and readability by identifying +and flagging instances where implicit or explicit casts from enumeration types to +boolean could cause potential issues. + +Example +------- + +.. code-block:: c++ + + enum EStatus + { + OK = 1, + NOT_OK, + UNKNOWN + }; + + void process(EStatus status) + { + if (not status) + { + // this true-branch in theory wont be executed + return; + } + + // proceed with "valid data" + } + + +Options +------- + +.. option:: EnumIgnoreList + + Option is used to ignore certain enum types when checking for + implicit/explicit casts to bool. It accepts a semicolon-separated list of + (fully qualified) enum type names or regular expressions that match the enum + type names. + The default value is an empty string, which means no enums will be ignored. 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 @@ -86,6 +86,7 @@ `bugprone-dangling-handle `_, `bugprone-dynamic-static-initializers `_, `bugprone-easily-swappable-parameters `_, + `bugprone-enum-to-bool-conversion `_, `bugprone-exception-escape `_, `bugprone-fold-init-type `_, `bugprone-forward-declaration-namespace `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion-cpp11.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion-cpp11.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion-cpp11.cpp @@ -0,0 +1,32 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-enum-to-bool-conversion %t + +namespace with::issue +{ + +enum class EStatus : char +{ + SUCCESS = 1, + FAILURE = 2, + INVALID_PARAM = 3, + UNKNOWN = 4 +}; + +bool testEnumConversion(EStatus value) +{ +// CHECK-MESSAGES: :[[@LINE+1]]:12: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-enum-to-bool-conversion] + return static_cast(value); +} + +enum EResult : short +{ + OK = 1, + NOT_OK +}; + +bool testEnumConversion(const EResult& value) +{ +// CHECK-MESSAGES: :[[@LINE+1]]:12: warning: conversion of 'EResult' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-enum-to-bool-conversion] + return value; +} + +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion.cpp @@ -0,0 +1,130 @@ +// RUN: %check_clang_tidy -std=c++98-or-later %s bugprone-enum-to-bool-conversion %t -- \ +// RUN: -config="{CheckOptions: [{key: bugprone-enum-to-bool-conversion.EnumIgnoreList, value: '::without::issue::IgnoredEnum;IgnoredSecondEnum'}]}" + +namespace with::issue +{ + +typedef enum EStatus +{ + SUCCESS = 1, + FAILURE = 2, + INVALID_PARAM = 3, + UNKNOWN = 4 +} Status; + +bool testEnumConversion(EStatus value) +{ +// CHECK-MESSAGES: :[[@LINE+1]]:12: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-enum-to-bool-conversion] + return value; +} + +bool testTypedefConversion(Status value) +{ +// CHECK-MESSAGES: :[[@LINE+1]]:12: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-enum-to-bool-conversion] + return value; +} + +bool testExplicitConversion(EStatus value) +{ +// CHECK-MESSAGES: :[[@LINE+1]]:30: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-enum-to-bool-conversion] + return static_cast(value); +} + +bool testInIfConversion(EStatus value) +{ +// CHECK-MESSAGES: :[[@LINE+1]]:9: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-enum-to-bool-conversion] + if (value) + { + return false; + } + + return true; +} + +bool testWithNegation(EStatus value) +{ +// CHECK-MESSAGES: :[[@LINE+1]]:16: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-enum-to-bool-conversion] + return not value; +} + +} + +namespace without::issue +{ + +enum StatusWithZero +{ + UNK = 0, + OK = 1, + NOT_OK = 2 +}; + +bool testEnumConversion(StatusWithZero value) +{ + return value; +} + +enum WithDefault +{ + Value0, + Value1 +}; + +bool testEnumConversion(WithDefault value) +{ + return value; +} + +enum WithNegative : int +{ + Nen2 = -2, + Nen1, + Nen0 +}; + +bool testEnumConversion(WithNegative value) +{ + return value; +} + +enum EStatus +{ + SUCCESS = 1, + FAILURE, + INVALID_PARAM, + UNKNOWN +}; + +bool explicitCompare(EStatus value) +{ + return value == SUCCESS; +} + +bool testEnumeratorCompare() +{ + return SUCCESS; +} + +enum IgnoredEnum +{ + IGNORED_VALUE_1 = 1, + IGNORED_VALUE_2 +}; + +enum IgnoredSecondEnum +{ + IGNORED_SECOND_VALUE_1 = 1, + IGNORED_SECOND_VALUE_2 +}; + +bool testIgnored(IgnoredEnum value) +{ + return value; +} + +bool testIgnored(IgnoredSecondEnum value) +{ + return value; +} + +}