[clang][analyzer] Fix empty enum handling in EnumCastOutOfRange checker

[gamesh411]

Summary

The alpha.cplusplus.EnumCastOutOfRange checker previously gave many
false positives because a warning was given if the initializer value
did not appear in the enumerator list.
The strict handling caused std::byte to always give a warning, as it
is implemented as an enum class without any declarators.

[shafik]

I did not look at this in detail but I don't think this approach is correct. I fixed this for constant evaluation the other day and you can find the details here: D130058

The test suite I wrote should be sufficient for you to validate your approach against.

[donat.nagy]

I think the old "report when the value stored in an enum type is not equal to one of the enumerators" behavior would be useful as an opt-in checker that could be adopted by some projects; while the new "relaxed" version is too bland to be really useful. I'd suggest keeping the old behavior in the general case, eliminating the "obvious" false positives like std::byte (don't report types that have no enumerators) and moving this checker towards the optin group.

[steakhal]

In D153954#4480103, @donat.nagy wrote:

I think the old "report when the value stored in an enum type is not equal to one of the enumerators" behavior would be useful as an opt-in checker that could be adopted by some projects; while the new "relaxed" version is too bland to be really useful. I'd suggest keeping the old behavior in the general case, eliminating the "obvious" false positives like std::byte (don't report types that have no enumerators) and moving this checker towards the optin group.

I would agree that after ignoring the obvious cases (like no enumerators), a checker like this could be useful as an "optin" checker, however, I'd not recommend moving the actual implementation anywhere.
You can create "virtual" checkers like it's done in many cases, eg. MismatchedDeallocatorChecker.

FYI I haven't checked the actual implementation thoroughly, because I had higher-level remarks inline.

clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp

49

I can see llvm::APSInt used a few places. Consider using namespace llvm;

71–75

Why do you fold the "Pred" ExplodedNode?
I'd say, you probably didn't want to use addTransition here.
Why don't you assume the subsequent assumptions and transition only once?

Anyway, I think it's better to have the addTransition closer to the outer scope (where the error reporting is done), so that we can easily see how many ways we branch off, etc.

clang/test/Analysis/enum-cast-out-of-range.cpp

200

void sink(...); would have achieved the same and I'd say it's less complex.
sink(unscoped, scoped);

[donat.nagy]

In D153954#4480296, @steakhal wrote:

[...] I'd not recommend moving the actual implementation anywhere.

I agree, I mostly spoke figuratively, suggesting that this checker could end up in the optin group when it's eventually brought out of alpha.

clang/test/Analysis/enum-cast-out-of-range.cpp

200

Or just (void)unscoped; (void)scoped; if we're bikeshedding this test...

[gamesh411]

The checker now retains the original detection logic, but only whitelists empty
enums.

As a future step the checker is moved into the optin package.

[gamesh411]

In D153954#4456713, @shafik wrote:

I did not look at this in detail but I don't think this approach is correct. I fixed this for constant evaluation the other day and you can find the details here: D130058

The test suite I wrote should be sufficient for you to validate your approach against.

@shafik, @donat.nagy Thanks for looking at this patch.
I have checked the linked improvements in Sema, and this initial modification would only lead to a ClangSA implementation of the same error-detection logic.
Due to symbolic execution, the set of potentially detectable errors is bigger, but this is maybe not the right place for this checker.

My goal is to improve the checker and eventually bring it out of alpha.

I see one main problem with this checker:

It does not support enums with fixed underlying type, namely std::byte, which is implemented like this:

enum class byte: unsigned char {};

As std::byte has no enumerators, the checker would say any otherwise allowed non-default initialization of a std::byte instance is bad, i.e.:

std::byte b {42}; // the checker gives a warning for this

Aside from this, in the optin package, there is a place for the current state of the checker (namely that only the value mentioned in the enumerator list are OK).
I will create a separate patch to move it out of alpha and into optin package.

clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp

49

Again, good point, I just abandoned the direction where many APSInt mentions are introduced, and the original only has 2 mentions of APSInt.

71–75

Thanks! This is a very good point; I just changed the general direction of the checker, and this implementation is no longer touched by the change.

[donat.nagy]

LGTM, with a little bikeshedding ;-)

clang/test/Analysis/enum-cast-out-of-range.cpp

203

Consider using "specified" and "unspecified" instead of "fixed" and "unfixed", because the rest of the test file uses them and in my opinion "unfixed" looks strange in this context. (I know that e.g. https://en.cppreference.com/w/cpp/language/enum speaks about "fixed" underlying context, but it doesn't use "unfixed".)

[steakhal]

LGTM

clang/test/Analysis/enum-cast-out-of-range.cpp

203

How about calling it "plain" or "common"?

212–214

[donat.nagy]

clang/test/Analysis/enum-cast-out-of-range.cpp

203

"plain" is also a good alternative for "unfixed" (although it would still be inconsistent with the earlier cases); but "common" is confusing, because in addition to the "plain, regular, normal" meaning it can also mean "mutual; shared by more than one <something>".

[gamesh411]

minor review fixups

clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp

public:
   ConstraintBasedEQEvaluator(CheckerContext &C,
                              const DefinedOrUnknownSVal CompareValue)
       : CompareValue(CompareValue), PS(C.getState()), SVB(C.getSValBuilder()) {}
 
   bool operator()(const llvm::APSInt &EnumDeclInitValue) {
     DefinedOrUnknownSVal EnumDeclValue = SVB.makeIntVal(EnumDeclInitValue);
     DefinedOrUnknownSVal ElemEqualsValueToCast =
         SVB.evalEQ(PS, EnumDeclValue, CompareValue);
 
     return static_cast<bool>(PS->assume(ElemEqualsValueToCast, true));
   }
 };
 
 // This checker checks CastExpr statements.
 // If the value provided to the cast is one of the values the enumeration can
 // represent, the said value matches the enumeration. If the checker can
 // establish the impossibility of matching it gives a warning.
 // Being conservative, it does not warn if there is slight possibility the
 // value can be matching.
 class EnumCastOutOfRangeChecker : public Checker<check::PreStmt<CastExpr>> {
   mutable std::unique_ptr<BuiltinBug> EnumValueCastOutOfRange;
   void reportWarning(CheckerContext &C) const;
 
 public:
   void checkPreStmt(const CastExpr *CE, CheckerContext &C) const;
 };
 
 using EnumValueVector = llvm::SmallVector<llvm::APSInt, 6>;
 
 // Collects all of the values an enum can represent (as SVals).
 EnumValueVector getDeclValuesForEnum(const EnumDecl *ED) {
   EnumValueVector DeclValues(
       std::distance(ED->enumerator_begin(), ED->enumerator_end()));
   llvm::transform(ED->enumerators(), DeclValues.begin(),
                  [](const EnumConstantDecl *D) { return D->getInitVal(); });
   return DeclValues;
 }
 } // namespace
 
 void EnumCastOutOfRangeChecker::reportWarning(CheckerContext &C) const {
   if (const ExplodedNode *N = C.generateNonFatalErrorNode()) {
     if (!EnumValueCastOutOfRange)
       EnumValueCastOutOfRange.reset(
void EnumCastOutOfRangeChecker::checkPreStmt(const CastExpr *CE,
 
   // If the cast is an enum, get its declaration.
   // If the isEnumeralType() returned true, then the declaration must exist
   // even if it is a stub declaration. It is up to the getDeclValuesForEnum()
   // function to handle this.
   const EnumDecl *ED = T->castAs<EnumType>()->getDecl();
 
   EnumValueVector DeclValues = getDeclValuesForEnum(ED);
+
+  // If the declarator list is empty, bail out.
+  // Every initialization an enum with a fixed underlying type but without any
+  // enumerators would produce a warning if we were to continue at this point.
+  // The most notable example is std::byte in the C++17 standard library.
+  if (DeclValues.size() == 0)
+    return;
+
   // Check if any of the enum values possibly match.
   bool PossibleValueMatch = llvm::any_of(
       DeclValues, ConstraintBasedEQEvaluator(C, *ValueToCast));
 
   // If there is no value that can possibly match any of the enum values, then
   // warn.
   if (!PossibleValueMatch)
     reportWarning(C);

clang/test/Analysis/enum-cast-out-of-range.cpp

void rangeConstrained7(int input) {
   if (input >= 3 && input <= 3)
     auto value = static_cast<scoped_specified_t>(input); // expected-warning {{The value provided to the cast expression is not in the valid range of values for the enum}}
 }
 
 void enumBitFieldAssignment() {
   S s;
   s.E = static_cast<unscoped_unspecified_t>(4); // OK.
   s.E = static_cast<unscoped_unspecified_t>(5); // expected-warning {{The value provided to the cast expression is not in the valid range of values for the enum}}
 }
+
+
+enum class empty_unspecified {};
+
+enum class empty_specified: char {};
+
+enum class empty_specified_unsigned: unsigned char {};
+
+void ignore_unused(...);
+
+void empty_enums_init_with_zero_should_not_warn() {
+  auto eu = static_cast<empty_unspecified>(0); //should always be OK to zero initialize any enum
+  auto ef = static_cast<empty_specified>(0);
+  auto efu = static_cast<empty_specified_unsigned>(0);
+
+  ignore_unused(eu, ef, efu);
+}