[clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer.

[pfultz2]

Summary

This check diagnoses instances where an enum is implicitly converted to an
integer. In C++11, enums can be defined as enum class which will prevent
such implicit conversion, however, enum provides no such guarantees to
prevent bugs. There can be many reasons why enum cannot be replaced with
enum class such as compatibility with C or legacy libraries.

This check will diagnose similar implicit conversions when using enum to
find the same class of bugs. Currently it will only warn on function or
constructor calls as such conversions are not clear to the user, but this
could be expanded in the future.

void foo(int i);
void f() {
    foo(e1); // e1 is implictly converted to an int
}

[artem.tamazov]

A typo in the doc, but otherwise LGTM.

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

9

[pfultz2]

Fix typo and merge from main.

[artem.tamazov]

LGTM

[pfultz2]

Anymore feedback? Can this be merged now?

[njames93]

What is the motivation for requiring these to be the arguments of call or construct expressions?

Would be nice to add a note with an embedded fixit to silence this warning.
Something like this, though you may need to use the Lexer to get the correct location to insert the ).

auto Note = diag(MatchedExpr->getBeginLoc(), "insert an explicit cast to silence this warning", DiagnosticIDs::Note);
if (Result.Context->getLangOpts().CPlusPlus11)
  Note << FixItHint::CreateInsertion(MatchedExpr->getBeginLoc(), "static_cast<int>(")
           << FixItHint::CreateInsertion(MatchedExpr->getEndLoc().getLocWithOffset(1), ")");
else
  Note << FixItHint::CreateInsertion(MatchedExpr->getBeginLoc(), "(int)");

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

29

Remove this.

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

19

FIXME

[njames93]

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

32

diagnostics in clang aren't formatted as full sentences.

[pfultz2]

What is the motivation for requiring these to be the arguments of call or construct expressions?

It is to just to try and limit the possible false positives at first. Usually a function call it is not clear locally that it will be converted to an integer but perhaps its common for users to assign an enum it an integer?

I could extend it to support everything except explicit casts if you think that would be better.

[pfultz2]

And for context, here is an actual bug this check would help find:

https://github.com/ROCmSoftwarePlatform/MIOpen/pull/1578#discussion_r889038610

[artem.tamazov]

In D129570#3717972, @pfultz2 wrote:

I could extend it to support everything except explicit casts if you think that would be better.

If asked, I would vote for extending this warning.

[pfultz2]

Update review comments and added fixit hints.

[pfultz2]

Updated to diagnose return statements as well.

[pfultz2]

Anymore feedback?

[pfultz2]

So can this be merged now?

[pfultz2]

All review comments have been addressed. I assume this can be merged now.

[pfultz2]

@njames93 I fixed the review comments, can this be merged?

[pfultz2]

I would like to see this merged in. Is there anyone available to review or approve?

[carlosgalvezp]

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

32

Check that this pointer is not nullptr, before dereferencing it. Typically we do it like this in all other checks:

if (const auto* MatchedExpr = Result.Nodes.getNodeAs<Expr>("x")) {
  ...
}

If you don't like the extra nesting you can return early.

if (!MatchedExpr)
  return;

35

I think this text is redundant given the fix-it note. Also, I find "silence this warning" a bit confusing, since in order to do that one would use NOLINT. By adding the cast one is not just "silencing the warning", one is fixing the code to be explicit.

46

I don't think we can assume the type of the enum is int, see for example:

enum Foo : unsigned int
{
    a,
    b
};


void f(unsigned int);

int main()
{
    f(a);
}

Even if there is no explicit underlying type, isn't the underlying type implementation-defined?

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

2

Please add a test using an enum with fixed underlying type that is not "int".

4

Please add a test to verify the C-style cast fix.

4–5

Please use a consistent style

enum A {
  e1,
  e2,
};

[pfultz2]

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

46

Since its an explicit cast then we should probably use the type that the function accepts(since thats what it will be eventually converted to) rather than the type of the underling enum type.

[pfultz2]

Improve null checking, use the correct type in the fixit, and updated the tests.

[pfultz2]

Merge from main.

[carlosgalvezp]

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

46

I'm not sure that's correct either, and it could lead to narrowing conversion warnings being ignored, see example:

https://godbolt.org/z/PfPxEYPrj

The correct thing to do would be to cast to the underlying type of the enum. A user could do it like this, although it's quite unreadable:

static_cast<std::underlying_type_t<MyEnum>>(enum_value)

But since in this context we have a lot more information from the AST, maybe we can automatically figure out the underlying type of the enum without having to call std::underlying_type?

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 "EnumToIntCheck.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<EnumToIntCheck>("bugprone-enum-to-int");
     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
+  EnumToIntCheck.cpp
   ExceptionEscapeCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   ForwardingReferenceOverloadCheck.cpp
   ImplicitWideningOfMultiplicationResultCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundingsCheck.cpp
   InfiniteLoopCheck.cpp

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

+//===--- EnumToIntCheck.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_ENUMTOINTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOINTCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Diagnoses instances of an enum implicitly converted to an integer
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/enum-to-int.html
+class EnumToIntCheck : public ClangTidyCheck {
+public:
+  EnumToIntCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_ENUMTOINTCHECK_H

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

+//===--- EnumToIntCheck.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 "EnumToIntCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void EnumToIntCheck::registerMatchers(MatchFinder *Finder) {
+  auto ImplicitEnumToInt = implicitCastExpr(
+      hasCastKind(CK_IntegralCast),
+      hasSourceExpression(expr(hasType(enumType()))),
+      anyOf(hasParent(callExpr()), hasParent(cxxConstructExpr())));
+  Finder->addMatcher(ImplicitEnumToInt.bind("x"), this);
+}
+
+void EnumToIntCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedExpr = Result.Nodes.getNodeAs<Expr>("x");
+  diag(MatchedExpr->getBeginLoc(), "enum is implictly converted to an integral")
+      << MatchedExpr->getSourceRange();
+  auto Note = diag(MatchedExpr->getBeginLoc(),
+                   "insert an explicit cast to silence this warning",
+                   DiagnosticIDs::Note);
+  if (Result.Context->getLangOpts().CPlusPlus11) {
+    SourceManager &SM = *Result.SourceManager;
+    Note << FixItHint::CreateInsertion(MatchedExpr->getBeginLoc(),
+                                       "static_cast<int>(");
+    Note << FixItHint::CreateInsertion(
+        Lexer::getLocForEndOfToken(MatchedExpr->getEndLoc(), 0, SM,
+                                   getLangOpts()),
+        ")");
+  } else {
+    Note << FixItHint::CreateInsertion(MatchedExpr->getBeginLoc(), "(int)");
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 New checks
 ^^^^^^^^^^
 
 - New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
   <clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members>` check.
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
+- New :doc:`bugprone-enum-to-int
+  <clang-tidy/checks/bugprone/enum-to-int>` check.
+
+  Finds implicit conversion of enum to an integral type.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc54-cpp
   <clang-tidy/checks/cert/msc54-cpp>` to
   :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` was added.
 

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

+.. title:: clang-tidy - bugprone-enum-to-int
+
+bugprone-enum-to-int
+====================
+
+This check diagnoses instances where an enum is implicitly converted to an
+integer. In C++11, enums can be defined as ``enum class`` which will prevent
+such implicit conversion, however, ``enum`` provides no such guarantees to
+prevent bugs. There can be many reasons why ``enum`` cannot be replaced with
+``enum class`` such as compatibility with C or legacy libraries.
+
+This check will diagnose similiar implicit conversions whne using ``enum`` to
+find the same class of bugs. Currently it will only warn on function or
+constructor calls as such conversions are not clear to the usr, but this
+could be expanded in the future.
+
+Examples:
+
+.. code-block:: c++
+
+    void foo(int i);
+    void f() {
+        foo(e1); // e1 is implictly converted to an int
+    }

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-int <bugprone/enum-to-int.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-int.cpp

+// RUN: %check_clang_tidy -std=c++11-or-later --fix-notes %s bugprone-enum-to-int %t
+
+enum A { e1,
+         e2 };
+
+struct bar {
+  bar(int);
+};
+void foo(int i);
+void f1() {
+  foo(e1);
+  // CHECK-NOTES: :[[@LINE-1]]:7: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+  // CHECK-NOTES: :[[@LINE-2]]:7: note: insert an explicit cast to silence this warning
+  // CHECK-NOTES: static_cast<int>( )
+  // CHECK-FIXES: foo(static_cast<int>(e1));
+}
+void f2() {
+  foo(static_cast<int>(e2));
+}
+void f3() {
+  int i = e1;
+  foo(i);
+}
+void f4() {
+  bar a(e1);
+  // CHECK-NOTES: :[[@LINE-1]]:9: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+  // CHECK-NOTES: :[[@LINE-2]]:9: note: insert an explicit cast to silence this warning
+  // CHECK-NOTES: static_cast<int>( )
+  // CHECK-FIXES: bar a(static_cast<int>(e1));
+}
+void f5() {
+  auto a = bar{e1};
+  // CHECK-NOTES: :[[@LINE-1]]:16: warning: enum is implictly converted to an integral [bugprone-enum-to-int]
+  // CHECK-NOTES: :[[@LINE-2]]:16: note: insert an explicit cast to silence this warning
+  // CHECK-NOTES: static_cast<int>( )
+  // CHECK-FIXES: auto a = bar{static_cast<int>(e1)};
+}