Skip to content

[cmd] Emit errors instead of hiding flags #4465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 19, 2025

Conversation

Szelethus
Copy link
Contributor

One of the great annoyances with any tool, but in particular with CodeChecker is that we have a habit of hiding, suppressing stuff instead of just telling the user whats up. Such is the case with --stats, --ctu, etc. These flags can only be used if the clang CodeChecker found supports statistics generation or CTU analysis. However, if you try to invoke CodeChecker with an incompatible flag, you get something like this:

$ CodeChecker analyze compile_commands.json  --stats -o output
usage: CodeChecker [-h] {analyze} ...
CodeChecker: error: unrecognized arguments: --stats

That is nuts! Not only do we emit this message, it doesn't even show up in --help. How is a user supposed to know about z3 analyses, if we never show it in the first place? If we did, a user might get a hold of a clang that supports it, there is literally zero reason to just outright hide it.

With this patch, all flags that depend on an analyzer tool having a specific capability are displayed by default, and a helpful error message is shown if its not available. Here is an example:

$ CodeChecker analyze compile_commands.json  --stats -o output
[ERROR 2025-02-20 15:04] - Statistics options can only be enabled if clang has statistics checkers available!
[INFO 2025-02-20 15:04] - CodeChecker has not found Clang Static Analyzer checkers statisticsCollector.ReturnValueCheck and statisticsCollector.SpecialReturnValue
[INFO 2025-02-20 15:04] - hint: Maybe CodeChecker found the wrong analyzer binary?

The change is as trivial as it looks, the only thing I changed is that some statistics related option's default value now depends on whether clang is stats capable (similarly to how its always been done for ctu flags), and since the z3 tests checked z3 compliance from CodeChecker's own error message, that needed adjustment as well. In the Z3 tests, the command that runs CodeChecker anayze were moved below the part where the compile_commands.json is copied.

@Szelethus Szelethus added enhancement 🌟 CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands usability 👍 Usability-related features labels Feb 20, 2025
@Szelethus Szelethus added this to the release 6.26.0 milestone Feb 20, 2025
@Szelethus
Copy link
Contributor Author

As a sidenote: I tried creating a custom argparse action that prematurely exits as the argument is parsed, but that package is so hard to work with I figured it takes more effot than its worth. In particular, I couldn't revert to "store_true" action type after my own checks were done, I couldn't even iterate through the subarguments of an argument group, etc. Hence the not-so-clever patch.

@Szelethus Szelethus requested review from dkrupp and cservakt February 20, 2025 14:14
Copy link
Contributor

@bruntib bruntib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this improvement!

Copy link
Contributor

@bruntib bruntib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wanted to request for some changes :(
Also, please check the failing tests, too.

@Szelethus Szelethus force-pushed the errors_instead_of_hiding_flags branch from 267ad50 to 3986f46 Compare March 11, 2025 12:59
@Szelethus
Copy link
Contributor Author

I fixed the web tests, though I did remove an exception throw from the web/test/libtest library. Interestingly, call_command is slighty different in web/test/libtest/ than in analyzer/test/libtest/, most notably, that OSError exception is already absent in the analyzer version. I was kind of sctraching my head why it was needed in web, so I just got rid of it for now.

@Szelethus Szelethus requested a review from bruntib March 11, 2025 13:46
@bruntib bruntib merged commit e938190 into Ericsson:master Mar 19, 2025
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands enhancement 🌟 usability 👍 Usability-related features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants