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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> Would you mind me committing as is?

Np, please do, the patch is great regardless!



================
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";
----------------
Szelethus wrote:
> 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?
> 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:
> ```lang=c++
> 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");
> ```
For **ULTIMATE** reliability and reusability, i'd also use a pointer-to-member argument instead of a pass-by-reference out-parameter, so that make sure that the value is indeed stored in the checker:
```lang=c++
Mgr.initFieldWithOption(Checker, "AllowedPad",
                        &PaddingChecker::AllowedPad,
                        ...)
```
(sorry, couldn't help myself)


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

https://reviews.llvm.org/D57850