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

[Szelethus]

Summary

Asserting on invalid input isn't very nice, hence the patch to emit an error instead.

This is the first of many patches to overhaul the way we handle checker options, and I admit that it's a kind of "things that didn't fit anywhere else" patch.

[teemperor]

LGTM for the CloneChecker changes.

[NoQ]

Looks cool, thanks!!

include/clang/Basic/DiagnosticDriverKinds.td

306–307

I suggest hardcoding the word "value" in every message rather than putting it here, because it may be desirable to substitute it with something more specific or more accurate in every checker's specific circumstances.

Eg., "a non-negative integer" would be more precise than "a non-negative value".

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

352–355

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.,

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:

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.

[Szelethus]

include/clang/Basic/DiagnosticDriverKinds.td

306–307

Yup, makes total sense!

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

352–355

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.

[Szelethus]

• Added a convenience method to CheckerManager for reporting invalid user input
• Edited the error message to be more flexible

[NoQ]

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

352–355

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:

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.

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");

[Szelethus]

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

352–355

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?

[NoQ]

Would you mind me committing as is?

Np, please do, the patch is great regardless!

lib/StaticAnalyzer/Checkers/PaddingChecker.cpp

352–355

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");

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:

Mgr.initFieldWithOption(Checker, "AllowedPad",
                        &PaddingChecker::AllowedPad,
                        ...)

(sorry, couldn't help myself)