[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Yeah, this looks good. How does this compare to `alpha.core.BoolConversion`, should we maybe merge them?



================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:84
+void EnumCastOutOfRangeChecker::reportWarning(CheckerContext &C) const {
+  if (auto N = C.generateNonFatalErrorNode(C.getState())) {
+    if (!EnumValueCastOutOfRange)
----------------
`C.getState()` is the default value (if you see how `generateNonFatalErrorNode()` calls `addTransition()` which in turns substitutes nullptr with `getState()`), so it can be omitted.


================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:88-89
+          new BuiltinBug(this, "Enum cast out of range",
+                         "The value provided to the cast expression is not in "
+                         "the valid range of values for the enum."));
+    C.emitReport(llvm::make_unique<BugReport>(
----------------
I believe that we'd need to explain the range we're seeing for the value in this case. Probably by asking the constraint manager to dump the range of possible values through some new API call (in case of `RangeConstraintManager` it should be straightforward).

Because otherwise it may be unobvious from the path what values are possible, when the `SVal` is a symbol.

Or, even better, we could highlight, with the help of a bug reporter visitor, the places where constraints were added to the symbol (then, again, it'd be better with ranges).

Ideally:
```
enum { Zero=0, One, Two, Three, Four } y;

void foo(int x) {
  if (x > 3) {
    // core note: Assuming 'x' is greater than 3
    // checker note: The assigned value is assumed to be within range [4, 2147483647]
    ...
  }
  z = x;
  // intermix of 'x' and 'z' is intentional:
  // checker notes should track symbol, not variable,
  // which is what makes them great.
  if (z < 7) {
    // core note: Assuming 'z' is less than 7
    // checker note: The assigned value is assumed to be within range [4, 6]
    if (x > 4) {
      // core note: Assuming 'x' is greater than 4
      // checker note: The assigned value is assumed to be within range [5, 6]
      y = z; // warning: The value provided to the cast expression is in range [5, 6], which is not in the valid range of values for the enum
    }
  }
}
```
(with a special case when the range consists of one element)


================
Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:119-122
+  // Check if any of the enum values possibly match.
+  bool PossibleValueMatch =
+      std::any_of(DeclValues.begin(), DeclValues.end(),
+                  ConstraintBasedEQEvaluator(C, *ValueToCastOptional));
----------------
I suspect that enum values often cover a whole segment, and in this case a pair of assumes for the sides of the segment (or, even better, a single `assumeInclusiveRange`) would be much faster.


https://reviews.llvm.org/D33672