[clang][analyzer] suppress `optin.core.EnumCastOutOfRange` for bit-wise operator of scoped enum

[zufuliu]

related to issue #48725.

enum class Flag {
    None = 0,
    A = 1, 
    B = 2,
};

constexpr Flag operator|(Flag a, Flag b) noexcept {
    return static_cast<Flag>(static_cast<int>(a) | static_cast<int>(b));
}

Flag getFlag() {
    return Flag::A | Flag::B;
}

https://godbolt.org/z/bhqcoPEPj

<source>:8:12: warning: The value '3' provided to the cast expression is not in the valid range of values for 'Flag' [clang-analyzer-optin.core.EnumCastOutOfRange]
    8 |     return static_cast<Flag>(static_cast<int>(a) | static_cast<int>(b));
      |            ^
<source>:1:12: note: enum declared here
    1 | enum class Flag {
      | ~~~~~~~~~~~^~~~~~
    2 |     None = 0,
      |     ~~~~~~~~~
    3 |     A = 1, 
      |     ~~~~~~
    4 |     B = 2,
      |     ~~~~~~
    5 | };
      | ~
<source>:12:12: note: Calling 'operator|'
   12 |     return Flag::A | Flag::B;
      |            ^~~~~~~~~~~~~~~~~
<source>:8:12: note: The value '3' provided to the cast expression is not in the valid range of values for 'Flag'
    8 |     return static_cast<Flag>(static_cast<int>(a) | static_cast<int>(b));
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: Zufu Liu (zufuliu)

related to issue #48725. ```c++ enum class Flag { None = 0, A = 1, B = 2, }; constexpr Flag operator|(Flag a, Flag b) noexcept { return static_cast<Flag>(static_cast<int>(a) | static_cast<int>(b)); } Flag getFlag() { return Flag::A | Flag::B; } ``` https://godbolt.org/z/bhqcoPEPj ``` <source>:8:12: warning: The value '3' provided to the cast expression is not in the valid range of values for 'Flag' [clang-analyzer-optin.core.EnumCastOutOfRange] 8 | return static_cast<Flag>(static_cast<int>(a) | static_cast<int>(b)); | ^ <source>:1:12: note: enum declared here 1 | enum class Flag { | ~~~~~~~~~~~^~~~~~ 2 | None = 0, | ~~~~~~~~~ 3 | A = 1, | ~~~~~~ 4 | B = 2, | ~~~~~~ 5 | }; | ~ <source>:12:12: note: Calling 'operator|' 12 | return Flag::A | Flag::B; | ^~~~~~~~~~~~~~~~~ <source>:8:12: note: The value '3' provided to the cast expression is not in the valid range of values for 'Flag' 8 | return static_cast<Flag>(static_cast<int>(a) | static_cast<int>(b)); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. ```

[whisperity]

This is, unfortunately, indeed not supported right now, but, as far as I know, this is a planned improvement. (An issue, however, would be that if the CSA checker is taking a configuration flag, as far as I know, there is no way to pass checker configurations of CSA checkers from Clang-Tidy's interface.)

[NagyDonat]

(Also note that this limitation of this "optin" checker is clearly documented: https://clang.llvm.org/docs/analyzer/checkers.html#optin-core-enumcastoutofrange-c-c . There are some plans to make this checker more general in the future, but until then it should not be enabled on projects that want to store flag combinations in enum variables.)

[zufuliu]

this limitation of this "optin" checker is clearly documented

Thanks for the link, however clang-analyzer-* does not enable the check in clang-tidy 17.

[NagyDonat]

This checker was improved by recent commits (authored by @gamesh411 and me), so in Clang 17 you can only find an earlier variant that's named alpha.cplusplus.EnumCastOutOfRange. (Note: this checker is not especially connected to C++, the "cplusplus" in the old name is just misleading. Also note that the old documentation was highly incomplete.)

This checker wasn't enabled by clang-analyzer-* in clang-tidy 17 because then it was an alpha checker (= has general quality issues, use at your own risk), while now it's an optin checker (= stable, but enforces a guideline or stylistic choice that is useful for some projects, but unwanted in others).

I'd say that the main issue is that the generic clang-analyzer-* flag, which should provide a "sane default", turns on the optin checkers.

[NagyDonat]

Unfortunately it seems that clang-analyzer-* is just a "dump" wildcard expression, not a reference to a "smart", curated list of generally useful checkers.

The alpha checkers are disabled by a hardcoded setting, which can be disabled by a hidden undocumented command-line flag for those users who like to see lots of false positives. This solution is appropriate for the alpha checkers but we cannot "borrow" it for the optin checkers, because there we hope that many users will opt in to some of them (which are compatible with their goals).

I don't know what would be the good UI that (1) makes the optin checkers easy to access (2) ensures that the rough "turn on everything" setting produces sane behavior.

[yvs2014]

the same false-positive for bit-wise flags in C code, for example using gtk/glib functions like

../gio/gunixmount.c:292:41: warning: The value '24' provided to the cast expression is not in the valid range of values for the enum [clang-analyzer-optin.core.EnumCastOutOfRange]
  292 |   subprocess = g_subprocess_newv (argv, G_SUBPROCESS_FLAGS_STDOUT_SILENCE | G_SUBPROCESS_FLAGS_STDERR_PIPE, &error);
      |                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../gio/gioenums.h:2036:9: note: enum declared here
 2036 | typedef enum {
      |         ^~~~~~
 2037 |   G_SUBPROCESS_FLAGS_NONE                  = 0,
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2038 |   G_SUBPROCESS_FLAGS_STDIN_PIPE            = (1u << 0),
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2039 |   G_SUBPROCESS_FLAGS_STDIN_INHERIT         = (1u << 1),
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2040 |   G_SUBPROCESS_FLAGS_STDOUT_PIPE           = (1u << 2),
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2041 |   G_SUBPROCESS_FLAGS_STDOUT_SILENCE        = (1u << 3),
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2042 |   G_SUBPROCESS_FLAGS_STDERR_PIPE           = (1u << 4),
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2043 |   G_SUBPROCESS_FLAGS_STDERR_SILENCE        = (1u << 5),
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2044 |   G_SUBPROCESS_FLAGS_STDERR_MERGE          = (1u << 6),
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2045 |   G_SUBPROCESS_FLAGS_INHERIT_FDS           = (1u << 7),
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2046 |   G_SUBPROCESS_FLAGS_SEARCH_PATH_FROM_ENVP = (1u << 8)
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 2047 | } GSubprocessFlags;
      | ~
../gio/gunixmount.c:286:7: note: Assuming the condition is false
  286 |   if (g_task_return_error_if_cancelled (task))
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../gio/gunixmount.c:286:3: note: Taking false branch
  286 |   if (g_task_return_error_if_cancelled (task))
      |   ^
../gio/gunixmount.c:292:41: note: The value '24' provided to the cast expression is not in the valid range of values for the enum
  292 |   subprocess = g_subprocess_newv (argv, G_SUBPROCESS_FLAGS_STDOUT_SILENCE | G_SUBPROCESS_FLAGS_STDERR_PIPE, &error);
      |                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

update: sorry, not spotted the previous related issue https://github.com/llvm/llvm-project/issues/48725 with C examples

[NagyDonat]

As I already explained, these issues are caused by the fact that the wildcard clang-analyzer-* turns on ALL static analyzer checkers, including the optin group which contains checkers that are not appropriate for all projects. (E.g. they enforce a design rule, which is followed on many projects, but may be too strict for some tastes.)

I think the right solution would be introducing a way to turn on a "sane default" set of clang analyzer checkers (which should be probably distinct from the wildcard clang-analyzer-*), but I'm primarily a clang static analyzer developer, and not familiar enough with the grouping/categories of tidy checks to decide this UI question.

@EugeneZelenko @whisperity @ anybody else more qualified than me What do you think about this question? How could we avoid this pitfall in the GUI?

[whisperity]

I always said that enabling CSA checkers through Tidy as a library has been a design blunder especially considering that at least for a long time (might still be a thing!) there were definitely no ways to configure CSA checker options through the Tidy invocation. (And we keep on having to relabel new issues to CSA instead of Tidy because users keep on believing that these are bugs from Tidy.)

As I already explained, these issues are caused by the fact that the wildcard clang-analyzer-* turns on ALL static analyzer checkers, including the optin group which contains checkers that are not appropriate for all projects.

Similarly to how --checks='*' in Tidy turns on ALL Tidy checks, including things like llvm- and llvmlibc- which are 99% nonsense for 99% of the projects. So a global "disallow list applicable even if using a superset wildcard" functionality is definitely missing.

There are some demarcations already in place, e.g., not enabling alpha checkers. And please check, but this SHOULD, at least in theory, keep clang-analyzer-alpha.* out of the checkers-to-fire list despite clang-analyzer-* being present. https://github.com/llvm/llvm-project/blob/fdf2b0a252c8aac9805b110a249817502d10e39f/clang-tools-extra/clang-tidy/ClangTidy.cpp#L481-L485

[NagyDonat]

There are some demarcations already in place, e.g., not enabling alpha checkers. And please check, but this SHOULD, at least in theory, keep clang-analyzer-alpha. out of the checkers-to-fire list despite clang-analyzer- being present.

Yes, I'm pretty sure that static analyzer alpha checkers are disabled by default, e.g. this issue had surfaced when EnumCastOutOfRange was moved to optin from alpha. (Also there are brutally buggy alpha checkers, so the users would complain loudly if those were enabled.)

[yvs2014]

the fact that the wildcard clang-analyzer-* turns on ALL static analyzer checkers, including the optin group which contains checkers that are not appropriate for all projects

Isn't it possible to disable specific checkers? Using in CLI mode like clang-tidy --checks=-clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling,-clang-analyzer-optin.core.EnumCastOutOfRange ... made it possible to get rid of many unhelpful clang-* complaints.

[NagyDonat]

Isn't it possible to disable specific checkers?

Yes, it's possible to disable the unwanted checkers after wildcard-enabling everything, and this might be a good workflow for some users.

I still think that it would be nice to have a user-friendly default that can be enabled "as is" and wouldn't need manual interventions each time a new optin checker appears. However, this is just a "nice to have" feature, not a bug that needs to be fixed.

[whisperity]

@NagyDonat I would say first what should happen is improving Tidy's documentation with a dedicated section for "this is what happens if you run CSA through Tidy" and there we can suggest using clang-analyzer-*,-clang-analyzer-optin.* for example. In parallel to that, we can also change the default behaviour to ignore optin.* checkers in addition to alpha.* checkers.

[NagyDonat]

That sounds like a good plan. I don't think that I'll have time for implementing these, but I can help with review if needed.