Page MenuHomePhabricator

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

Authored by pfultz2 on Jul 12 2022, 8:59 AM.

Details

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
}

Diff Detail

Event Timeline

pfultz2 created this revision.Jul 12 2022, 8:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2022, 8:59 AM
pfultz2 requested review of this revision.Jul 12 2022, 8:59 AM
artem.tamazov requested changes to this revision.Jul 25 2022, 9:16 AM
artem.tamazov added a subscriber: artem.tamazov.

A typo in the doc, but otherwise LGTM.

clang-tools-extra/docs/clang-tidy/checks/bugprone/enum-to-int.rst
8
This revision now requires changes to proceed.Jul 25 2022, 9:16 AM
pfultz2 updated this revision to Diff 451284.Aug 9 2022, 3:25 PM

Fix typo and merge from main.

pfultz2 marked an inline comment as done.Aug 9 2022, 3:26 PM
This revision is now accepted and ready to land.Aug 9 2022, 5:27 PM
pfultz2 updated this revision to Diff 451666.Aug 10 2022, 3:56 PM
pfultz2 retitled this revision from Add new clang-tidy check to find implicit conversions from enum to integer. to [clang-tidy] Add new clang-tidy check to find implicit conversions from enum to integer..Aug 10 2022, 5:29 PM

Anymore feedback? Can this be merged now?

njames93 added a subscriber: 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

This revision now requires review to proceed.Aug 11 2022, 3:16 PM
njames93 added inline comments.Aug 11 2022, 3:18 PM
clang-tools-extra/clang-tidy/bugprone/EnumToIntCheck.cpp
32

diagnostics in clang aren't formatted as full sentences.

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.

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

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

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 updated this revision to Diff 453514.Aug 17 2022, 8:02 PM

Update review comments and added fixit hints.

pfultz2 marked 3 inline comments as done.Aug 17 2022, 8:03 PM
pfultz2 updated this revision to Diff 453515.Aug 17 2022, 8:05 PM
pfultz2 added reviewers: njames93, artem.tamazov.
pfultz2 updated this revision to Diff 454632.Aug 22 2022, 3:28 PM

Updated to diagnose return statements as well.

Anymore feedback?

So can this be merged now?

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

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

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

carlosgalvezp added inline comments.Tue, Jan 10, 11:42 PM
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 added inline comments.Wed, Jan 11, 5:18 PM
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 updated this revision to Diff 488440.Wed, Jan 11, 5:42 PM

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

pfultz2 marked 5 inline comments as done.Wed, Jan 11, 5:43 PM
pfultz2 updated this revision to Diff 488445.Wed, Jan 11, 5:48 PM

Merge from main.

carlosgalvezp added inline comments.Wed, Jan 11, 11:30 PM
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?