[PATCH] D57850: [analyzer] Emit an error rather than assert on invalid checker option input

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.
Herald added a subscriber: Charusso.


================
Comment at: lib/StaticAnalyzer/Checkers/PaddingChecker.cpp:352-355
+  if (Checker->AllowedPad < 0)
+    Mgr.getDiagnostics().Report(diag::err_analyzer_checker_option_invalid_input)
+        << (llvm::Twine() + Checker->getTagDescription() + ":AllowedPad").str()
+        << "a non-negative";
----------------
NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > 
> > > I passively wish for a certain amount of de-duplication that wouldn't require every checker to obtain a diagnostics engine every time it tries to read an option. Eg.,
> > > ```lang=c++
> > >   auto *Checker = Mgr.registerChecker<PaddingChecker>();
> > >   Checker->AllowedPad = Mgr.getAnalyzerOptions()
> > >           .getCheckerIntegerOption(Checker, "AllowedPad", 24);
> > >   if (Checker->AllowedPad < 0)
> > >     Mgr.reportInvalidOptionValue(Checker, "AllowedPad", "a non-negative");
> > > ```
> > > 
> > > Or maybe even something like that:
> > > 
> > > ```lang=c++
> > >   auto *Checker = Mgr.registerChecker<PaddingChecker>();
> > >   Checker->AllowedPad = Mgr.getAnalyzerOptions()
> > >           .getCheckerIntegerOption(Checker, "AllowedPad", 24,
> > >                   [](int x) -> Option<std::string> {
> > >                       if (x < 0) {
> > >                         // Makes getCheckerIntegerOption() emit a diagnostic
> > >                         // and return the default value.
> > >                         return "a non-negative";
> > >                       }
> > >                       // Makes getCheckerIntegerOption() successfully return
> > >                       // the user-specified value.
> > >                       return None;
> > >                   });
> > > ```
> > > I.e., a validator lambda.
> > First one, sure. I'm a little unsure about the second: No other "options"-like classes have access to a `DiagnosticsEngine` in clang, as far as I'm aware, and I guess keeping `AnalyzerOptions` as simple is possible is preferred. Not only that, but a validator lambda seems an to be an overkill (though really-really cool) solution. Your first bit of code is far more readable IMO.
> Hmm, in the first example we'll also have to manually reset the option to the default value if it is invalid, which is also annoying - even if easy to understand, it's also easy to forget.
> 
> And with that and a bit of polish, the lambda approach isn't really much more verbose, and definitely involves less duplication:
> 
> ```lang=c++
> auto *Checker = Mgr.registerChecker<PaddingChecker>();
> Checker->AllowedPad = Mgr.getAnalyzerOptions()
>         .getCheckerIntegerOption(Checker, "AllowedPad", 24);
> if (Checker->AllowedPad < 0) {
>   Mgr.reportInvalidOptionValue(Checker, "AllowedPad", "a non-negative value");
>   Checker->AllowedPad = 24;
> }
> ```
> vs.
> ```lang=c++
> auto *Checker = Mgr.registerChecker<PaddingChecker>();
> Checker->AllowedPad = Mgr.getAnalyzerOptions()
>         .getCheckerIntegerOption(Checker, "AllowedPad", /*Default=*/ 24,
>                                  /*Validate=*/ [](int x) { return x >= 0; },
>                                  /*ValidMsg=*/ "a non-negative value");
> ```
Alright, so I've given this a lot of thought, here's where I'm standing on the issue:

* I would prefer not to add `DiagnosticsEngine` to `AnalyzerOptions`. In fact, I'd prefer not to add it even as a parameter to one of it's methods -- designwise, it should be a simple mapping of the command line parameters, not doing any complicated hackery.
* You got me convinced the validator lambda thing ;). However, a nice implementation of this (emphasis on //nice//) is most definitely a bigger undertaking.
* Once we're at the topic of "easy to forget", we could also verify compile-time whether checker options are actually used -- what I'm thinking here, is something like this:

```
auto *Checker = Mgr.registerChecker<PaddingChecker>();
Mgr.initFieldWithOption(Checker, "AllowedPad",
                        // Note that we should be able
                        // to know the default value.
                        Checker->AllowedPad,
                        // We could make this optional by defining a
                        // default validator...
                        /*Validate=*/ [](int x) { return x >= 0; },
                        // ...aaaand a default error message.
                        /*ValidMsg=*/ "a non-negative value");
```
`CheckerManager` later (once all checker registry functions finished) could validate, with the help of `CheckerRegistry`, whether
* All options for a given checker were queried for,
* The supplied checker options is valid, if not, restore them in compatibility mode, emit an error otherwise,
* No list is complete without a third item.

For now, I admit, I have little interest in this. Would you mind me committing as is?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57850/new/

https://reviews.llvm.org/D57850