[clang-tidy] Add bugprone-non-zero-enum-to-bool-conversion check

[PiotrZSL]

Summary

Detect implicit and explicit conversions of enum to bool,
when enum doesn't have a enumerator with value equal to 0.
In theory such conversion should always return TRUE.

[PiotrZSL]

Removed "Offers fixes" from list.rst

[Eugene.Zelenko]

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

29

Should language be checked for C++?

clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst

7

Please synchronize first statement with statement in Release Notes. Please use double back-ticks for language constructs.

50

Please use single back-ticks for option values. Default value is usually placed after option description.

[PiotrZSL]

Correct copyrights and commit author

[PiotrZSL]

Ready for review

[PiotrZSL]

Corrected issues mentioned in review

[PiotrZSL]

Fixed

[PiotrZSL]

Documentation to be updated, option need to be changed to list

[PiotrZSL]

Ping, Rebase, Changed Option from Regexp to List, Improved documentation, Removed usage of deprecated API

[carlosgalvezp]

Such a great check, thanks! I have very minor comments, the most debatable one about the name of the check (but no strong opinion).

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

107

I think the name is a bit generic, do we foresee risk of conflict with other similar checks in the future? I wonder if we should narrow it down to something like bugprone-non-zero-enum-to-bool-conversion. WDYT?

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

23

Use explicit type

24

The convention in the LLVM repo is to use traditional operators, please keep consistency.

27

Use explicit type

72

Unnecessary empty line

clang-tools-extra/docs/ReleaseNotes.rst

112

Keep alphabetical order.

clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst

12–13

I can't think of a case where this could happen, might be worth adding an example below.

14

Nit: Add space between // and NOLINT

38

Nit: use 2 chars indentation

39

Use traditional operators for consistency.

41

won't

41

Remove?

clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion-cpp11.cpp

9

Indent with 2 chars instead of 4.

16

Nit: Add 2 chars indentation to align with the code.

20

Maybe test also "int", since it's the most common default? Also test one enum class without explicit underlying type.

[PiotrZSL]

Thank you for review, I will correct comments in this week.

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

107

I'm fine with name change....

clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst

41

enums are ints, you can assign them any value by using static casts or when reading data from external message, so thats why in theory, but in this example you right, there is no "in theory"

[PiotrZSL]

TODO: Fix review comments, rename check.

[PiotrZSL]

clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst

12–13

I thinkg that in STL we got some enums (flags) used as bitmasks... Just wanted to mention that there could be some known false-positives.

[PiotrZSL]

Review fixes + check rename

[carlosgalvezp]

LGTM, thanks! Would be good to fix the last nit before landing.

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

17 ↗

(On Diff #511904)

Nit remove excessive whitespace

[PiotrZSL]

Fix check description in source file (white spaces, sync it with release notes)

[PiotrZSL]

Rebase due to broken baseline

[PiotrZSL]

Rebase

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

 #include "AssignmentInIfConditionCheck.h"
 #include "BadSignalToKillThreadCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "BranchCloneCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
 #include "DynamicStaticInitializersCheck.h"
 #include "EasilySwappableParametersCheck.h"
+#include "EnumToBoolConversionCheck.h"
 #include "ExceptionEscapeCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
 #include "ImplicitWideningOfMultiplicationResultCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundingsCheck.h"
 #include "InfiniteLoopCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<CopyConstructorInitCheck>(
         "bugprone-copy-constructor-init");
     CheckFactories.registerCheck<DanglingHandleCheck>(
         "bugprone-dangling-handle");
     CheckFactories.registerCheck<DynamicStaticInitializersCheck>(
         "bugprone-dynamic-static-initializers");
     CheckFactories.registerCheck<EasilySwappableParametersCheck>(
         "bugprone-easily-swappable-parameters");
+    CheckFactories.registerCheck<EnumToBoolConversionCheck>(
+        "bugprone-enum-to-bool-conversion");
     CheckFactories.registerCheck<ExceptionEscapeCheck>(
         "bugprone-exception-escape");
     CheckFactories.registerCheck<FoldInitTypeCheck>("bugprone-fold-init-type");
     CheckFactories.registerCheck<ForwardDeclarationNamespaceCheck>(
         "bugprone-forward-declaration-namespace");
     CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
         "bugprone-forwarding-reference-overload");
     CheckFactories.registerCheck<ImplicitWideningOfMultiplicationResultCheck>(

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

add_clang_library(clangTidyBugproneModule
   BadSignalToKillThreadCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   BranchCloneCheck.cpp
   BugproneTidyModule.cpp
   CopyConstructorInitCheck.cpp
   DanglingHandleCheck.cpp
   DynamicStaticInitializersCheck.cpp
   EasilySwappableParametersCheck.cpp
+  EnumToBoolConversionCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundingsCheck.cpp
   InfiniteLoopCheck.cpp

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

+//===--- 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 <vector>
+
+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<StringRef> EnumIgnoreList;
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOBOOLCONVERSIONCHECK_H

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

+//===--- 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 <algorithm>
+
+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<CastExpr>("cast");
+  const auto *Enum = Result.Nodes.getNodeAs<EnumDecl>("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

clang-tools-extra/docs/ReleaseNotes.rst

 - New :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` check.
 
   Checks for functions that have safer, more secure replacements available, or
   are considered deprecated due to design flaws.
   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
+  <clang-tidy/checks/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
   <clang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this>` check.
 
   Warns when lambda specify a capture default and capture ``this``.
 
 - New :doc: `llvmlibc-inline-function-decl
   <clang-tidy/checks/llvmlibc/inline-function-decl>` check.
 

clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-bool-conversion.rst

+.. 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.

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

.. csv-table::
    `bugprone-assignment-in-if-condition <bugprone/assignment-in-if-condition.html>`_,
    `bugprone-bad-signal-to-kill-thread <bugprone/bad-signal-to-kill-thread.html>`_,
    `bugprone-bool-pointer-implicit-conversion <bugprone/bool-pointer-implicit-conversion.html>`_, "Yes"
    `bugprone-branch-clone <bugprone/branch-clone.html>`_,
    `bugprone-copy-constructor-init <bugprone/copy-constructor-init.html>`_, "Yes"
    `bugprone-dangling-handle <bugprone/dangling-handle.html>`_,
    `bugprone-dynamic-static-initializers <bugprone/dynamic-static-initializers.html>`_,
    `bugprone-easily-swappable-parameters <bugprone/easily-swappable-parameters.html>`_,
+   `bugprone-enum-to-bool-conversion <bugprone/enum-to-bool-conversion.html>`_,
    `bugprone-exception-escape <bugprone/exception-escape.html>`_,
    `bugprone-fold-init-type <bugprone/fold-init-type.html>`_,
    `bugprone-forward-declaration-namespace <bugprone/forward-declaration-namespace.html>`_,
    `bugprone-forwarding-reference-overload <bugprone/forwarding-reference-overload.html>`_,
    `bugprone-implicit-widening-of-multiplication-result <bugprone/implicit-widening-of-multiplication-result.html>`_, "Yes"
    `bugprone-inaccurate-erase <bugprone/inaccurate-erase.html>`_, "Yes"
    `bugprone-incorrect-roundings <bugprone/incorrect-roundings.html>`_,
    `bugprone-infinite-loop <bugprone/infinite-loop.html>`_,

clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion-cpp11.cpp

+// 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<bool>(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;
+}
+
+}

clang-tools-extra/test/clang-tidy/checkers/bugprone/enum-to-bool-conversion.cpp

+// 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<bool>(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;
+}
+
+}