Allow skipping checks with @("nolint(...)") and @nolint("...")

[ricardaxel]

Related #935

Here is the basic idea I had in mind to locally disable checks : one can attach a user defined attribute to any declaration (function declaration, variable declaration, ..). If this happens, the given check is disabled for the given declaration.

Example :

@("NOLINT(useless_initializer)")
int a = 0; // No warning emited

One could have done that with comment (like clang-tidy does) but I think UDAs are great since they are directly included in AST. They have a limitation though : according to libdparse D grammar, attributes can only be paired with 'declaration2'. So this would be a little hard to disable checks at other levels than declaration. For example, delete_check is applied on a deleteExpression.

Looking forward your reviews, I will try yo extend this proof of concept to all warnings in next few days if you're are okay with this direction.

[WebFreak001]

can you also add a check to the ModuleDeclaration that checks for the UDAs and then just returns or continues with recursion? Also people might use whitespace within NOLINT() and maybe we just want to lowercase it instead of uppercasing, since non-string UDA equivalents would be all lower-case or camel-case

[WebFreak001]

you have mixed spaces and tabs everywhere in this file, try to stay consistent with the existing style (only tabs in this case)

[ricardaxel]

Do you use any formatter ? I can't find any config, this would be easier to maintain style conventions.

[ricardaxel]

Also people might use whitespace within NOLINT() and maybe we just want to lowercase it instead of uppercasing, since non-string UDA equivalents would be all lower-case or camel-case

No problem, I'll draft a second version with regex (case insensitive and ignoring spaces)

EDIT: Done Here --> 39757e1 and 06b5ff5

I was thinking about some forms that could be accepted :

- @("nolint(useless_initializer)") // skip useless_initializer check
- @("nolint(useless_assert_check, useless_assert_check)") // skip useless_initializer and useless_assert_check checks
- @("nolint") // skip ALL checks
- @("nolint(useless_*)") // skip all check that begin with 'useless'

[ricardaxel]

can you also add a check to the ModuleDeclaration that checks for the UDAs and then just returns or continues with recursion?

Not sure to understand well. Do you mean something like that ?

override void visit(const ModuleDeclaration mod)
{
    if(hasNoLintUda(mod))
       return;
    else
      accept(this)
}

[WebFreak001]

re module declaration: yes like that

also I think all your suggestions for UDA match format are nice and would be useful

[ricardaxel]

Soo I think this is a first mergeable version.

I think I'll keep other improvements for futures PRs, especially :

• adding support and unittest for all checks one by one
• adding support for @("nolint"), @("nolint(useless_*)"), ...

Just a question : what do you expect when a malformed UDA is given ? For example @nolint("useles_initializer") (it contains a typo). I guess raising an error is pretty extrem, and dump a warning could be confusing with real D-Scanner warning, no ? For now, such cases are silently ignored

[WebFreak001]

we could probably add a dscanner checker to check for misconfigured dscanner UDAs later

[WebFreak001]

we need to make the nolint check check for error message IDs (in most linters this is the enum KEY member)

If you want to keep the optimization that it doesn't traverse into code, in the base analyzer class you probably want to add a abstract string[] getAvailableErrorKeys(); to be able to check all of them where you currently only check for the check name, however there would need to be another check in the actual addErrorMessage code that will check before emitting the warning for real.

[ricardaxel]

we need to make the nolint check check for error message IDs

I totally missed that one analyzer could handle multiple checks. I made some refacto to handle this.
The optimization to avoid the traversal is gone for now, and will probably be delayed for a future PR.

ah the idea was here that the nolint would apply to the entire file

I also completely misunderstood that, this is fixed :)

Hope I didn't miss anything else

[AndrejMitrovic]

Great to see this feature in and thanks so much for working on this!

But I can't seem to silence this error for example:

// test.d
@("nolint(dscanner.exception_check)")
void main() {
    try {
    } catch (Error) {
    }
}

$ dscanner lint ./test.d

.\test.d(4:14): Warning: Catching Error or Throwable is almost always a bad idea. (exception_check)
    } catch (Error) {
             ^^^^^

Using master of dscanner.

Also it would be nice to have this feature documented in --help, I only found out about it by accident.

And does #935 need closing?

[ricardaxel]

Great to see this feature in and thanks so much for working on this!

But I can't seem to silence this error for example:

// test.d
@("nolint(dscanner.exception_check)")
void main() {
    try {
    } catch (Error) {
    }
}

To silence an error, one has to use its ID (and not its name !), which is in this case dscanner.suspicious.catch_em_all, as we can see here. This is pretty confusing since the name and the key ID can be different, and this is the name which is used in the dscanner.ini configuration file...

Also it would be nice to have this feature documented in --help, I only found out about it by accident.

And does #935 need closing?

The 'nolint' feature is pretty experimental, it has not been widely tested and would require some more polish (see previous messages of this PR + PR #941. I personally think that one should also work on adding coherence between names and IDs. So there is still some work to do). I think this is the reason for the lack of documentation.