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 @@ -38,6 +38,7 @@ #include "MoveForwardingReferenceCheck.h" #include "MultipleStatementMacroCheck.h" #include "NoEscapeCheck.h" +#include "NonZeroEnumToBoolConversionCheck.h" #include "NotNullTerminatedResultCheck.h" #include "ParentVirtualCallCheck.h" #include "PosixReturnCheck.h" @@ -139,6 +140,8 @@ CheckFactories.registerCheck( "bugprone-narrowing-conversions"); CheckFactories.registerCheck("bugprone-no-escape"); + CheckFactories.registerCheck( + "bugprone-non-zero-enum-to-bool-conversion"); CheckFactories.registerCheck( "bugprone-not-null-terminated-result"); 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 @@ -33,17 +33,18 @@ MoveForwardingReferenceCheck.cpp MultipleStatementMacroCheck.cpp NoEscapeCheck.cpp + NonZeroEnumToBoolConversionCheck.cpp NotNullTerminatedResultCheck.cpp ParentVirtualCallCheck.cpp PosixReturnCheck.cpp RedundantBranchConditionCheck.cpp ReservedIdentifierCheck.cpp SharedPtrArrayMismatchCheck.cpp - SmartPtrArrayMismatchCheck.cpp SignalHandlerCheck.cpp SignedCharMisuseCheck.cpp SizeofContainerCheck.cpp SizeofExpressionCheck.cpp + SmartPtrArrayMismatchCheck.cpp SpuriouslyWakeUpFunctionsCheck.cpp StandaloneEmptyCheck.cpp StringConstructorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/NonZeroEnumToBoolConversionCheck.h b/clang-tools-extra/clang-tidy/bugprone/NonZeroEnumToBoolConversionCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/NonZeroEnumToBoolConversionCheck.h @@ -0,0 +1,36 @@ +//===--- NonZeroNonZeroEnumToBoolConversionCheck.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_NONZEROENUMTOBOOLCONVERSIONCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_NONZEROENUMTOBOOLCONVERSIONCHECK_H + +#include "../ClangTidyCheck.h" +#include + +namespace clang::tidy::bugprone { + +/// Detect implicit and explicit casts of `enum` type into `bool` where +/// `enum` type doesn't have a zero-value enumerator. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/non-zero-enum-to-bool-conversion.html +class NonZeroEnumToBoolConversionCheck : public ClangTidyCheck { +public: + NonZeroEnumToBoolConversionCheck(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_NONZEROENUMTOBOOLCONVERSIONCHECK_H diff --git a/clang-tools-extra/clang-tidy/bugprone/NonZeroEnumToBoolConversionCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/NonZeroEnumToBoolConversionCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/NonZeroEnumToBoolConversionCheck.cpp @@ -0,0 +1,78 @@ +//===--- NonZeroEnumToBoolConversionCheck.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 "NonZeroEnumToBoolConversionCheck.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 EnumDecl *Definition = Node.getDefinition(); + return Definition && Node.isComplete() && + std::none_of(Definition->enumerator_begin(), + Definition->enumerator_end(), + [](const EnumConstantDecl *Value) { + return Value->getInitVal().isZero(); + }); +} + +} // namespace + +NonZeroEnumToBoolConversionCheck::NonZeroEnumToBoolConversionCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + EnumIgnoreList( + utils::options::parseStringList(Options.get("EnumIgnoreList", ""))) {} + +void NonZeroEnumToBoolConversionCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "EnumIgnoreList", + utils::options::serializeStringList(EnumIgnoreList)); +} + +bool NonZeroEnumToBoolConversionCheck::isLanguageVersionSupported( + const LangOptions &LangOpts) const { + return LangOpts.CPlusPlus; +} + +void NonZeroEnumToBoolConversionCheck::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 NonZeroEnumToBoolConversionCheck::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 @@ -106,6 +106,12 @@ New checks ^^^^^^^^^^ +- New :doc:`bugprone-non-zero-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:`bugprone-unsafe-functions ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/non-zero-enum-to-bool-conversion.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/non-zero-enum-to-bool-conversion.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/non-zero-enum-to-bool-conversion.rst @@ -0,0 +1,53 @@ +.. title:: clang-tidy - bugprone-non-zero-enum-to-bool-conversion + +bugprone-non-zero-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. + +May produce false positives if the ``enum`` is used to store other values +(used as a bit-mask or zero-initialized on purpose). To deal with them, +``// 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 (!status) { + // this true-branch won't 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 @@ -104,6 +104,7 @@ `bugprone-move-forwarding-reference `_, "Yes" `bugprone-multiple-statement-macro `_, `bugprone-no-escape `_, + `bugprone-non-zero-enum-to-bool-conversion `_, `bugprone-not-null-terminated-result `_, "Yes" `bugprone-parent-virtual-call `_, "Yes" `bugprone-posix-return `_, "Yes" diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/non-zero-enum-to-bool-conversion-cpp11.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/non-zero-enum-to-bool-conversion-cpp11.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/non-zero-enum-to-bool-conversion-cpp11.cpp @@ -0,0 +1,110 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-non-zero-enum-to-bool-conversion %t + +namespace with::issue { + +enum class EStatusC : char { + SUCCESS = 1, + FAILURE = 2, + INVALID_PARAM = 3, + UNKNOWN = 4 +}; + +bool testEnumConversion(EStatusC value) { + // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: conversion of 'EStatusC' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-enum-to-bool-conversion] + return static_cast(value); +} + +enum class EStatusS : short { + SUCCESS = 1, + FAILURE = 2, + INVALID_PARAM = 3, + UNKNOWN = 4 +}; + +bool testEnumConversion(EStatusS value) { + // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: conversion of 'EStatusS' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-enum-to-bool-conversion] + return static_cast(value); +} + + +enum class EStatusI : int { + SUCCESS = 1, + FAILURE = 2, + INVALID_PARAM = 3, + UNKNOWN = 4 +}; + +bool testEnumConversion(EStatusI value) { + // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: conversion of 'EStatusI' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-enum-to-bool-conversion] + return static_cast(value); +} + +enum class EStatus { + SUCCESS = 1, + FAILURE = 2, + INVALID_PARAM = 3, + UNKNOWN = 4 +}; + +bool testEnumConversion(EStatus value) { + // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-enum-to-bool-conversion] + return static_cast(value); +} + +namespace enum_int { + +enum EResult : int { + OK = 1, + NOT_OK +}; + +bool testEnumConversion(const EResult& value) { + // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: conversion of 'EResult' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-enum-to-bool-conversion] + return value; +} + +} + +namespace enum_short { + +enum EResult : short { + OK = 1, + NOT_OK +}; + +bool testEnumConversion(const EResult& value) { + // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: conversion of 'EResult' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-enum-to-bool-conversion] + return value; +} + +} + +namespace enum_char { + +enum EResult : char { + OK = 1, + NOT_OK +}; + +bool testEnumConversion(const EResult& value) { + // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: conversion of 'EResult' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-enum-to-bool-conversion] + return value; +} + +} + +namespace enum_default { + +enum EResult { + OK = 1, + NOT_OK +}; + +bool testEnumConversion(const EResult& value) { + // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: conversion of 'EResult' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-enum-to-bool-conversion] + return value; +} + +} + +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/non-zero-enum-to-bool-conversion.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/non-zero-enum-to-bool-conversion.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/non-zero-enum-to-bool-conversion.cpp @@ -0,0 +1,107 @@ +// RUN: %check_clang_tidy -std=c++98-or-later %s bugprone-non-zero-enum-to-bool-conversion %t -- \ +// RUN: -config="{CheckOptions: [{key: bugprone-non-zero-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]]:10: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-enum-to-bool-conversion] + return value; +} + +bool testTypedefConversion(Status value) { + // CHECK-MESSAGES: :[[@LINE+1]]:10: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-enum-to-bool-conversion] + return value; +} + +bool testExplicitConversion(EStatus value) { + // CHECK-MESSAGES: :[[@LINE+1]]:28: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-enum-to-bool-conversion] + return static_cast(value); +} + +bool testInIfConversion(EStatus value) { + // CHECK-MESSAGES: :[[@LINE+1]]:7: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-enum-to-bool-conversion] + if (value) { + return false; + } + return true; +} + +bool testWithNegation(EStatus value) { + // CHECK-MESSAGES: :[[@LINE+1]]:14: warning: conversion of 'EStatus' into 'bool' will always return 'true', enum doesn't have a zero-value enumerator [bugprone-non-zero-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; +} + +}