[analyzer] Move alpha checker EnumCastOutOfRange to optin #67157

NagyDonat · 2023-09-22T15:46:30Z

The checker EnumCastOutOfRange verifies the (helpful, but not standard-mandated) design rule that integer to enum casts should not produce values that don't have a corresponding enumerator. As it was improved and cleaned up by recent changes, this commit renames it from alpha.cplusplus.EnumCastOutOfRange to optin.core.EnumCastOutOfRange to reflect that it's no longer alpha quality.

As this checker handles a basic language feature (which is also present in plain C), I moved it to a "core" subpackage within "optin".

In addition to the renaming, this commit cleans up the documentation in checkers.rst and adds the new example code to a test file to ensure that it's indeed producing the behavior claimend in the documentation.

llvmbot · 2023-09-22T15:47:38Z

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Changes

The checker EnumCastOutOfRange verifies the (helpful, but not standard-mandated) design rule that integer to enum casts should not produce values that don't have a corresponding enumerator. As it was improved and cleaned up by recent changes, this commit renames it from alpha.cplusplus.EnumCastOutOfRange to optin.core.EnumCastOutOfRange to reflect that it's no longer alpha quality.

As this checker handles a basic language feature (which is also present in plain C), I moved it to a "core" subpackage within "optin".

In addition to the renaming, this commit cleans up the documentation in checkers.rst and adds the new example code to a test file to ensure that it's indeed producing the behavior claimend in the documentation.

Full diff: https://github.com/llvm/llvm-project/pull/67157.diff

6 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/docs/analyzer/checkers.rst (+22-17)
(modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+13-4)
(modified) clang/test/Analysis/enum-cast-out-of-range.c (+1-1)
(modified) clang/test/Analysis/enum-cast-out-of-range.cpp (+12-1)
(modified) clang/www/analyzer/alpha_checks.html (-19)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 477a40630f11097..522272a81c088f1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -491,6 +491,9 @@ Static Analyzer
   Read the PR for the details.
   (`#66086 <https://github.com/llvm/llvm-project/pull/66086>`_)
 
+- Move checker ``alpha.cplusplus.EnumCastOutOfRange`` out of the ``alpha``
+  package to ``optin.core.EnumCastOutOfRange``.
+
 .. _release-notes-sanitizers:
 
 Sanitizers
diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst
index dbd6d7787823530..552f7676a328371 100644
--- a/clang/docs/analyzer/checkers.rst
+++ b/clang/docs/analyzer/checkers.rst
@@ -535,6 +535,28 @@ optin
 
 Checkers for portability, performance or coding style specific rules.
 
+.. _optin-core-EnumCastOutOfRange:
+
+optin.core.EnumCastOutOfRange (C, C++)
+""""""""""""""""""""""""""""""""""""""
+Check for integer to enumeration casts that would produce a value with no
+corresponding enumerator. This is not necessarily undefined behavior, but can
+lead to nasty surprises, so projects may decide to use a coding standard that
+disallows these "unusual" conversions.
+
+Note that no warnings are produced when the enum type (e.g. `std::byte`) has no
+enumerators at all.
+
+.. code-block:: cpp
+
+ enum WidgetKind { A=1, B, C, X=99 };
+
+ void foo() {
+   WidgetKind c = static_cast<WidgetKind>(3);  // OK
+   WidgetKind x = static_cast<WidgetKind>(99); // OK
+   WidgetKind d = static_cast<WidgetKind>(4);  // warn
+ }
+
 .. _optin-cplusplus-UninitializedObject:
 
 optin.cplusplus.UninitializedObject (C++)
@@ -1853,23 +1875,6 @@ Reports destructions of polymorphic objects with a non-virtual destructor in the
              //       destructor
  }
 
-.. _alpha-cplusplus-EnumCastOutOfRange:
-
-alpha.cplusplus.EnumCastOutOfRange (C++)
-""""""""""""""""""""""""""""""""""""""""
-Check for integer to enumeration casts that could result in undefined values.
-
-.. code-block:: cpp
-
- enum TestEnum {
-   A = 0
- };
-
- void foo() {
-   TestEnum t = static_cast(-1);
-       // warn: the value provided to the cast expression is not in
-       //       the valid range of values for the enum
-
 .. _alpha-cplusplus-InvalidatedIterator:
 
 alpha.cplusplus.InvalidatedIterator (C++)
diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
index 65c1595eb6245dd..1c859c39cf44d91 100644
--- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -36,6 +36,7 @@ def CoreAlpha : Package<"core">, ParentPackage<Alpha>;
 // Note: OptIn is *not* intended for checkers that are too noisy to be on by
 // default. Such checkers belong in the alpha package.
 def OptIn : Package<"optin">;
+def CoreOptIn : Package<"core">, ParentPackage<OptIn>;
 
 // In the Portability package reside checkers for finding code that relies on
 // implementation-defined behavior. Such checks are wanted for cross-platform
@@ -433,6 +434,18 @@ def UndefinedNewArraySizeChecker : Checker<"NewArraySize">,
 
 } // end "core.uninitialized"
 
+//===----------------------------------------------------------------------===//
+// Optin checkers for core language features
+//===----------------------------------------------------------------------===//
+
+let ParentPackage = CoreOptIn in {
+
+def EnumCastOutOfRangeChecker : Checker<"EnumCastOutOfRange">,
+  HelpText<"Check integer to enumeration casts for out of range values">,
+  Documentation<HasDocumentation>;
+
+} // end "optin.core"
+
 //===----------------------------------------------------------------------===//
 // Unix API checkers.
 //===----------------------------------------------------------------------===//
@@ -763,10 +776,6 @@ def DeleteWithNonVirtualDtorChecker : Checker<"DeleteWithNonVirtualDtor">,
            "destructor in their base class">,
   Documentation<HasDocumentation>;
 
-def EnumCastOutOfRangeChecker : Checker<"EnumCastOutOfRange">,
-  HelpText<"Check integer to enumeration casts for out of range values">,
-  Documentation<HasDocumentation>;
-
 def IteratorModeling : Checker<"IteratorModeling">,
   HelpText<"Models iterators of C++ containers">,
   Dependencies<[ContainerModeling]>,
diff --git a/clang/test/Analysis/enum-cast-out-of-range.c b/clang/test/Analysis/enum-cast-out-of-range.c
index 3282cba653d7125..328b8b28ca70003 100644
--- a/clang/test/Analysis/enum-cast-out-of-range.c
+++ b/clang/test/Analysis/enum-cast-out-of-range.c
@@ -1,5 +1,5 @@
 // RUN: %clang_analyze_cc1 \
-// RUN:   -analyzer-checker=core,alpha.cplusplus.EnumCastOutOfRange \
+// RUN:   -analyzer-checker=core,optin.core.EnumCastOutOfRange \
 // RUN:   -verify %s
 
 enum En_t {
diff --git a/clang/test/Analysis/enum-cast-out-of-range.cpp b/clang/test/Analysis/enum-cast-out-of-range.cpp
index abc1431e5be140f..33790f726c5ff8a 100644
--- a/clang/test/Analysis/enum-cast-out-of-range.cpp
+++ b/clang/test/Analysis/enum-cast-out-of-range.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_analyze_cc1 \
-// RUN:   -analyzer-checker=core,alpha.cplusplus.EnumCastOutOfRange \
+// RUN:   -analyzer-checker=core,optin.core.EnumCastOutOfRange \
 // RUN:   -std=c++11 -verify %s
 
 enum unscoped_unspecified_t {
@@ -215,3 +215,14 @@ void empty_enums_init_with_zero_should_not_warn() {
 
   ignore_unused(eu, ef, efu);
 }
+
+//Test the example from checkers.rst:
+enum WidgetKind { A=1, B, C, X=99 };
+
+void foo() {
+  WidgetKind c = static_cast<WidgetKind>(3);  // OK
+  WidgetKind x = static_cast<WidgetKind>(99); // OK
+  WidgetKind d = static_cast<WidgetKind>(4);  // expected-warning {{The value provided to the cast expression is not in the valid range of values for the enum}}
+
+  ignore_unused(c, x, d);
+}
diff --git a/clang/www/analyzer/alpha_checks.html b/clang/www/analyzer/alpha_checks.html
index 181ce1b9de591b3..a00e330a4e99579 100644
--- a/clang/www/analyzer/alpha_checks.html
+++ b/clang/www/analyzer/alpha_checks.html
@@ -350,25 +350,6 @@ <h3 id="cplusplus_alpha_checkers">C++ Alpha Checkers</h3>
 }
 </pre></div></div></td></tr>
 
-<tr><td><a id="alpha.cplusplus.EnumCastOutOfRange"><div class="namedescr expandable"><span class="name">
-alpha.cplusplus.EnumCastOutOfRange</span><span class="lang">
-(C++)</span><div class="descr">
-  Check for integer to enumeration casts that could result in undefined values.
-</div></div></a></td>
-  <td><div class="exampleContainer expandable">
-    <div class="example"><pre>
-enum TestEnum {
-  A = 0
-};
-
-void foo() {
-  TestEnum t = static_cast<TestEnum>(-1);
-      // warn: the value provided to the cast expression is not in
-               the valid range of values for the enum
-}
-</pre></div></div></td></tr>
-
-
 <tr><td><a id="alpha.cplusplus.InvalidatedIterator"><div class="namedescr expandable"><span class="name">
 alpha.cplusplus.InvalidatedIterator</span><span class="lang">
 (C++)</span><div class="descr">

gamesh411 · 2023-09-28T08:20:02Z

I have run this checker on OS projects, and evaluated the results.
The quality of reports varies, but all in all, IMHO, this checker is OK to be in the optin package.

OpenSource Project name	New Reports	Reports Lost	Evaluation of reports
memcached_1.6.8	New reports	Lost reports	no reports
tmux_2.6	New reports	Lost reports	no reports
curl_curl-7_66_0	New reports	Lost reports	18 reports, all seem valid, even if some of them are just plain ugly macro expansion-wrapped madness
twin_v0.8.1	New reports	Lost reports	2 reports, 1 is flag type usage, so this project would want NOT want to opt into this checker
vim_v8.2.1920	New reports	Lost reports	29 reports, valid, but not really useful for this project
openssl_openssl-3.0.0	New reports	Lost reports	no reports
sqlite_version-3.33.0	New reports	Lost reports	no reports
ffmpeg_n4.3.1	New reports	Lost reports	39 reports, not really useful or understandable
postgres_REL_13_0	New reports	Lost reports	16 reports, they valid from the coding style enforcing POV
tinyxml2_8.0.0	New reports	Lost reports	no reports
libwebm_libwebm-1.0.0.27	New reports	Lost reports	no reports
xerces_v3.2.3	New reports	Lost reports	2 reports, not enough context in the bugpath to meaningfully evaluate correctness
bitcoin_v0.20.1	New reports	Lost reports	3 reports, flag style usage, not relevant
protobuf_v3.13.0	New reports	Lost reports	6 reports at least 2 of them seem valid issues if the code is not properly handling 0 valued enums (I guess this is not the case, but still, these are meaningful findings for developers)

NagyDonat · 2023-09-28T13:20:18Z

Before merging this PR, the diagnostics of the EnumCast checker should be updated to mention the name of the enum type in question. @gamesh411 could you create a PR that implements this improvement (as we discussed)?

haoNoQ · 2023-09-28T21:44:18Z

Before merging this PR, the diagnostics of the EnumCast checker should be updated to mention the name of the enum type in question. @gamesh411 could you create a PR that implements this improvement (as we discussed)?

Yes, it should call out the enum, call out the value, and then track the value (with trackExpressionValue()) in order to explain where it's coming from and why we think it's out of range.

Xazax-hun · 2023-10-09T23:27:21Z

How well does this check work with bitwise enums?

People often write code like:

enum AnimalFlags
{
    HasClaws   = 1,
    CanFly     = 2,
    EatsFish   = 4,
    Endangered = 8
};

AnimalFlags operator|(AnimalFlags a, AnimalFlags b)
{
    return static_cast<AnimalFlags>(static_cast<int>(a) | static_cast<int>(b));
}

auto flags = HasClaws | CanFly;

Would this warning emit false positives for these patterns? If yes, can/should we suppress them?

NagyDonat · 2023-10-18T16:10:37Z

@Xazax-hun This checker doesn't accept the use of bitwise enums (like the one in your example), so projects that use bitwise enums should not enable this checker. (Perhaps this limitation should be mentioned in the docs? @gamesh411)

It would be possible to write a more complex checker that recognizes and accepts enums that are used as flags; but I'm not sure that it's a good investment of developer-hours and I think that the current simple&stupid checker is already useful for some projects.

The checker EnumCastOutOfRange verifies the (helpful, but not standard-mandated) design rule that integer to enum casts should not produce values that don't have a corresponding enumerator. As it was improved and cleaned up by recent changes, this commit renames it from `alpha.cplusplus.EnumCastOutOfRange` to `optin.core.EnumCastOutOfRange` to reflect that it's no longer alpha quality. As this checker handles a basic language feature (which is also present in plain C), I moved it to a "core" subpackage within "optin". In addition to the renaming, this commit cleans up the documentation in `checkers.rst` and adds the new example code to a test file to ensure that it's indeed producing the behavior claimend in the documentation.

NagyDonat · 2023-12-05T16:06:21Z

(Force pushed the branch because I wanted to rebase it onto a recent main and fix the merge conflicts. Is there a better workflow than this?)

steakhal · 2023-12-05T16:40:20Z

(Force pushed the branch because I wanted to rebase it onto a recent main and fix the merge conflicts. Is there a better workflow than this?)

I think git also offers conflict resolution for git merge origin/main. It should put the resolution into the merge commit.
That being said, I usually just do the rebase/resolve approach and force-push; only after all conversations are resolved and I don't expect anyone reviewing at the time. But yea, a merge commit should be the way for GitHub AFAIK.

NagyDonat · 2023-12-05T17:55:32Z

Before merging this PR, the diagnostics of the EnumCast checker should be updated to mention the name of the enum type in question. @gamesh411 could you create a PR that implements this improvement (as we discussed)?

Yes, it should call out the enum, call out the value, and then track the value (with trackExpressionValue()) in order to explain where it's coming from and why we think it's out of range.

These suggestions were mostly implemented in #68191, I created PR #74503 to implement the last missing part ("call out the value").

NagyDonat · 2023-12-11T13:43:43Z

PR #74503 was merged, so I think all the review comments are addressed. @haoNoQ @Xazax-hun What do you think about merging this?

Xazax-hun · 2023-12-12T04:27:21Z

I am a bit concerned about the bad user experience with bitwise enums, but I think that is probably OK since this is an optin check. That being said, I think:

It would be easy to address this, there is already a clang tidy check for bitwise enums, the heuristics to recognize them could be reused from there and used to suppress bogus warnings.
As long as this limitation is documented, I am happy to get this merged as is.

NagyDonat · 2023-12-12T12:24:47Z

The limitation is emphasized in the documentation, and I also added a TODO comment that mentions it in the source code.

I'll merge this change as it is now (because I don't want to leave it open for even more time), but @whisperity is interested in this "bitflag enum" case and he'll address these issues when he'll have time for it (in January?).

Xazax-hun · 2023-12-12T15:03:41Z

Sounds good!

nico · 2023-12-12T16:57:01Z

This breaks check-clang everywhere, e.g.:

https://lab.llvm.org/buildbot/#/builders/139/builds/55404
http://45.33.8.238/linux/125647/step_7.txt

Please take a look and revert for now if it takes a while to fix. (Also, please run tests before committing.)

NagyDonat · 2023-12-12T17:13:53Z

The issue was really trivial to fix, I created a PR for it: #75216.

Sorry for breaking check-clang, I did ran the tests before pressing merge on the github GUI, but I forgot to consider that the GUI button rebases the commit onto the current main tip before merging it, and while the testcase was passing locally, it started to fail when it was rebased onto a version that contained #74503 (which was also added by me by the way, but I forgot to consider its effect on this commit).

* main: (5908 commits) [readtapi] Cleanup printing command line options (llvm#75106) [flang] Fix compilation error due to variable no being used (llvm#75210) [C API] Add getters and setters for fast-math flags on relevant instructions (llvm#75123) [libc++][CI] Tests the no RTTI configuration. (llvm#65518) [RemoveDIs] Fold variable into assert, it's only used once. NFC [RemoveDI] Handle DPValues in SROA (llvm#74089) [AArch64][GlobalISel] Test Pre-Commit for Look into array's element [mlir][tensor] Fix bug in `tensor.extract(tensor.from_elements)` folder (llvm#75109) [analyzer] Move alpha checker EnumCastOutOfRange to optin (llvm#67157) [RemoveDIs] Handle DPValues in replaceDbgDeclare (llvm#73507) [SHT_LLVM_BB_ADDR_MAP] Implements PGOAnalysisMap in Object and ObjectYAML with tests. [X86][GlobalISel] Add instruction selection for G_SELECT (llvm#70753) [AMDGPU] Remove unused function splitScalar64BitAddSub [LLVM][DWARF] Add compilation directory and dwo name to TU in dwo section (llvm#74909) [clang][Interp] Implement __builtin_ffs (llvm#72988) [RemoveDIs] Update ConvertDebugDeclareToDebugValue after llvm#72276 (llvm#73508) [libc][NFC] Reuse `FloatProperties` constant instead of creating new ones (llvm#75187) [RemoveDIs] Fix removeRedundantDdbgInstrs utils for dbg.declares (llvm#74102) Reapply "[RemoveDIs][NFC] Find DPValues using findDbgDeclares (llvm#73500)" [GitHub] Remove author association print from new-prs workflow ...

NagyDonat requested review from gamesh411 and dkrupp September 22, 2023 15:46

llvmbot added clang Clang issues not falling into any other category clang:static analyzer clang:dataflow Clang Dataflow Analysis framework - https://clang.llvm.org/docs/DataFlowAnalysisIntro.html labels Sep 22, 2023

NagyDonat requested review from haoNoQ and steakhal September 22, 2023 15:47

gamesh411 approved these changes Sep 28, 2023

View reviewed changes

NagyDonat force-pushed the dealpha_enum_cast branch from 9fd8def to 5c42d3e Compare December 5, 2023 16:05

Describe the limitation that the checker doesn't accept bitwise enums

e293c3e

Add a TODO to mitigate the limitations

fa2f9d5

Xazax-hun approved these changes Dec 12, 2023

View reviewed changes

NagyDonat merged commit c873f77 into llvm:main Dec 12, 2023

NagyDonat deleted the dealpha_enum_cast branch December 13, 2023 10:29

whisperity mentioned this pull request Mar 27, 2024

[config] Label Clang 18.0-19.trunk SA & Tidy checkers Ericsson/codechecker#4193

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[analyzer] Move alpha checker EnumCastOutOfRange to optin #67157

[analyzer] Move alpha checker EnumCastOutOfRange to optin #67157

Uh oh!

NagyDonat commented Sep 22, 2023

Uh oh!

llvmbot commented Sep 22, 2023 •

edited

Loading

Uh oh!

gamesh411 commented Sep 28, 2023 •

edited

Loading

Uh oh!

NagyDonat commented Sep 28, 2023

Uh oh!

haoNoQ commented Sep 28, 2023

Uh oh!

Xazax-hun commented Oct 9, 2023

Uh oh!

NagyDonat commented Oct 18, 2023

Uh oh!

NagyDonat commented Dec 5, 2023

Uh oh!

steakhal commented Dec 5, 2023

Uh oh!

NagyDonat commented Dec 5, 2023 •

edited

Loading

Uh oh!

NagyDonat commented Dec 11, 2023

Uh oh!

Xazax-hun commented Dec 12, 2023

Uh oh!

NagyDonat commented Dec 12, 2023

Uh oh!

Xazax-hun commented Dec 12, 2023

Uh oh!

nico commented Dec 12, 2023

Uh oh!

NagyDonat commented Dec 12, 2023 •

edited

Loading

Uh oh!

Uh oh!

[analyzer] Move alpha checker EnumCastOutOfRange to optin #67157

[analyzer] Move alpha checker EnumCastOutOfRange to optin #67157

Uh oh!

Conversation

NagyDonat commented Sep 22, 2023

Uh oh!

llvmbot commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gamesh411 commented Sep 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NagyDonat commented Sep 28, 2023

Uh oh!

haoNoQ commented Sep 28, 2023

Uh oh!

Xazax-hun commented Oct 9, 2023

Uh oh!

NagyDonat commented Oct 18, 2023

Uh oh!

NagyDonat commented Dec 5, 2023

Uh oh!

steakhal commented Dec 5, 2023

Uh oh!

NagyDonat commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NagyDonat commented Dec 11, 2023

Uh oh!

Xazax-hun commented Dec 12, 2023

Uh oh!

NagyDonat commented Dec 12, 2023

Uh oh!

Xazax-hun commented Dec 12, 2023

Uh oh!

nico commented Dec 12, 2023

Uh oh!

NagyDonat commented Dec 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Sep 22, 2023 •

edited

Loading

gamesh411 commented Sep 28, 2023 •

edited

Loading

NagyDonat commented Dec 5, 2023 •

edited

Loading

NagyDonat commented Dec 12, 2023 •

edited

Loading