[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

28

Should language be checked for C++?

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

6

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

49

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

24

Use explicit type

25

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

28

Use explicit type

73

Unnecessary empty line

clang-tools-extra/docs/ReleaseNotes.rst

108

Keep alphabetical order.

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

13–14

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

15

Nit: Add space between // and NOLINT

39

Nit: use 2 chars indentation

40

Use traditional operators for consistency.

42

won't

42

Remove?

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

10

Indent with 2 chars instead of 4.

17

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

21

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

42

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

13–14

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++ -*-===//
+//
+// © 2023 Nokia
+// 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 <string>
+
+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;
+
+private:
+  const std::string EnumIgnoreRegexp;
+};
+
+} // 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 -----------------------===//
+//
+// © 2023 Nokia
+// 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 "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().isNullValue();
+                      });
+}
+
+} // namespace
+
+EnumToBoolConversionCheck::EnumToBoolConversionCheck(StringRef Name,
+                                                     ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      EnumIgnoreRegexp(Options.get("EnumIgnoreRegexp", "^$")) {}
+
+void EnumToBoolConversionCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "EnumIgnoreRegexp", EnumIgnoreRegexp);
+}
+
+void EnumToBoolConversionCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      castExpr(hasCastKind(CK_IntegralToBoolean),
+               unless(isExpansionInSystemHeader()), hasType(booleanType()),
+               hasSourceExpression(
+                   expr(hasType(qualType(hasCanonicalType(hasDeclaration(
+                            enumDecl(isCompleteAndHasNoZeroValue(),
+                                     unless(matchesName(EnumIgnoreRegexp)))
+                                .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.
+
+  Warns about `enum` to `bool` conversions where `enum` 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 `enum` is used only to hold values
+equal to its enumerators, then conversion to `bool` will always result in `true`
+value.
+
+Check will produce false-positives if `enum` will be used to store other values
+(used as bit-mask or zero-initialized on purpose).
+
+`//NOLINT` or casting first to underlying type before casting to `bool` can be
+used to deal with false-positives.
+
+Check won't produce warnings if definition of `enum` is not available.
+C++11 enum classes are supported.
+
+Example
+-------
+
+.. code-block:: c++
+
+    enum EStatus
+    {
+        OK = 1,
+        NOT_OK,
+        UNKNOWN
+    };
+
+    void process(EStatus status)
+    {
+        if (not status)
+        {
+            // log error <- this true-branch in theory wont be executed
+            return;
+        }
+
+        // proceed with "valid data"
+    }
+
+
+Options
+-------
+
+.. option:: EnumIgnoreRegexp
+
+    Default value: '^$'.
+    Regexp used to ignore usages of enum declarations that match regexp.

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.EnumIgnoreRegexp, value: '^::without::issue::IgnoredEnum$'}]}"
+
+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
+};
+
+bool testIgnored(IgnoredEnum value)
+{
+    return value;
+}
+
+}