Page MenuHomePhabricator

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

Authored by Szelethus on Feb 6 2019, 2:43 PM.

Details

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.

Diff Detail

Repository
rC Clang

Event Timeline

Szelethus created this revision.Feb 6 2019, 2:43 PM
Herald added a project: Restricted Project. · View Herald Transcript
teemperor resigned from this revision.Feb 6 2019, 2:51 PM

LGTM for the CloneChecker changes.

NoQ accepted this revision.Feb 8 2019, 6:19 PM

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.

This revision is now accepted and ready to land.Feb 8 2019, 6:19 PM
Szelethus marked 2 inline comments as done.Feb 11 2019, 2:24 PM
Szelethus added inline comments.
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 updated this revision to Diff 186346.Feb 11 2019, 3:00 PM
  • Added a convenience method to CheckerManager for reporting invalid user input
  • Edited the error message to be more flexible
NoQ added inline comments.Feb 15 2019, 6:17 PM
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 marked an inline comment as done.Mar 6 2019, 7:31 AM
Szelethus added inline comments.
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 added a comment.Mar 7 2019, 6:17 PM

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)

This revision was automatically updated to reflect the committed changes.