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

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.

Reviewed By: carlosgalvezp

Differential Revision: https://reviews.llvm.org/D144036

Author: PiotrZSL

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 @@ class BugproneModule : public ClangTidyModule {
     CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
         "bugprone-narrowing-conversions");
     CheckFactories.registerCheck<NoEscapeCheck>("bugprone-no-escape");
+    CheckFactories.registerCheck<NonZeroEnumToBoolConversionCheck>(
+        "bugprone-non-zero-enum-to-bool-conversion");
     CheckFactories.registerCheck<NotNullTerminatedResultCheck>(
         "bugprone-not-null-terminated-result");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(

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

@@ -33,17 +33,18 @@ add_clang_library(clangTidyBugproneModule
   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

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

clang-tools-extra/docs/ReleaseNotes.rst

@@ -106,6 +106,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`bugprone-non-zero-enum-to-bool-conversion
+  <clang-tidy/checks/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
   <clang-tidy/checks/bugprone/unsafe-functions>` check.
 

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.

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

@@ -104,6 +104,7 @@ Clang-Tidy Checks
    `bugprone-move-forwarding-reference <bugprone/move-forwarding-reference.html>`_, "Yes"
    `bugprone-multiple-statement-macro <bugprone/multiple-statement-macro.html>`_,
    `bugprone-no-escape <bugprone/no-escape.html>`_,
+   `bugprone-non-zero-enum-to-bool-conversion <bugprone/non-zero-enum-to-bool-conversion.html>`_,
    `bugprone-not-null-terminated-result <bugprone/not-null-terminated-result.html>`_, "Yes"
    `bugprone-parent-virtual-call <bugprone/parent-virtual-call.html>`_, "Yes"
    `bugprone-posix-return <bugprone/posix-return.html>`_, "Yes"

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