DScanner: Enable a check blacklist for a gradual transition

[wilzbach]

Continuing the discussion at #5495 (comment)
The idea is

• to allow for better crowd-source collaboration on such tasks like improving the docs (properly_documented_public_functions) or providing public examples (has_public_example) as anyone can do this one module at a time
• to prevent regressions in the already checked modules (e.g. a new function in module X will be verified against the enabled checks even if there aren't globally enabled)

So instead of the all-or-nothing state for a Dscanner check before, a gradual, distributed transition can happen. While DScanner has a lot of great and useful checks, of course, a bit of care needs to be taken, so that they don't stand in the way of collaborators.

Hence, for convenience I copy/pasted my summary of currently "actionable" checks:

Actionable

• asm_style_check: only std.math
• could_be_immutable_check: @JackStouffer and @bbasile are big fans of this
• exception_check: Pokemon exception catching style (silent sinkhole for bugs), only a few occurrences
• function_attribute_check: duplicate attributes @safe @safe void foo() {}
• has_public_example: high-level vision, "Make sure every function in Phobos has an example" (this is still in the DScanner queue)
• label_var_same_name_check: very confusing to read, only a few occurrences
• long_line_check: fixed in Fix line_length checker for multiLine literals dlang-community/D-Scanner#465
• mismatched_args_check: prevents myFunction(counter, obj) for void myFunction(A)(A obj, int counter)
• object_const_check: checks whether opEquals, opCmp and toHash are const
• opequals_tohash_check checks for aggregates which have opEquals defined, but not toHash
• properly_documented_public_functions: high-level vision ("Make sure every function in Phobos has Params: and Returns: sections"), call to action)
• redundant_attributes_check: see Remove redundant access specifier from Phobos #5477
• undocumented_declaration_check: prevents accidentally leaked symbols, unfortunately a lot has happend in the past. I tried to fix std.algorithm once: std.algorithms: document public methods #4312
• unused_label_check: this check doesn't support mixins yet (unused_label: Detect unknown labels  dlang-community/D-Scanner#467)
• unused_variable_check: mostly artifacts from debug sessions

To be discussed

• auto_ref_assignment_check: only two occurrences, false positives?
• imports_sortedness: I still need to modify the Dscanner check to ignore selective sortedness ...
• length_subtraction_check: these may be unsigned and could lead to integer underflow
• logical_precedence_check: if (a && b || c) // bad, if (a && (b || c)) // good
• number_style_check: big numbers should use an underscore
• style_check: many, many symbols aren't named according to the DStyle
• useless_initializer: seems to be a quite common pattern in Phobos
• vcall_in_ctor: only four occurrences

Boring details

• the blacklist was generated
• The phobos branch at DScanner isn't vanilla anymore, but I am trying to get the bug fixes and features to upstream/master
• For simplicity (and maybe the future) I commented the discussed checks, but left the blacklist in (I used "+disabled" instead of setting the check to disabled, to have all (partially) disabled checks at one place)
• Before someone asks, inifiled doesn't support multilines (yet).

[CyberShadow]

One concern is that bulk code additions (e.g. adding a binding module as those in etc, or moving in some code from another project such as Druntime or tools) is going to be painful, as the submitter will need to keep running Dscanner to get a list of rules to which they will need to add their module.

It would be a bit nicer if the rules file was inverted: instead of a list of modules per rule, have a list of rules per module; then, such additions could just blacklist the entire module, and allow style fixes or finer-grained blacklists be done in follow-up PRs.

Otherwise this looks good to me. Would be nice to improve in how style failures are communicated to users to reduce submitter frustration (a comment from dlang-bot telling the user how to run the style check locally is a start; inline comments on failing lines would be better; inline comments with suggestions on how to fix them, or automatic pull requests to their branch, would be best).

[wilzbach]

One concern is that bulk code additions (e.g. adding a binding module as those in etc, or moving in some code from another project such as Druntime or tools) is going to be painful, as the submitter will need to keep running Dscanner to get a list of rules to which they will need to add their module.

Hehe, ideally new submissions should adhere the existing checks ;-)

It would be a bit nicer if the rules file was inverted: instead of a list of modules per rule, have a list of rules per module; then, such additions could just blacklist the entire module, and allow style fixes or finer-grained blacklists be done in follow-up PRs.

They can always do sth. like make -f posix.mak | cut -d':' -f2 | sort | uniq to get a list of all occurring checks ...
Apart from that I am not sure whether this will be a real problem, as imho (1) we should move modules away from Phobos not into, (2) criteria for std.experimental got so harsh that passing the static code analysis checker is a minor issue.

Otherwise this looks good to me. Would be nice to improve in how style failures are communicated to users to reduce submitter frustration (a comment from dlang-bot telling the user how to run the style check locally is a start; inline comments on failing lines would be better; inline comments with suggestions on how to fix them, or automatic pull requests to their branch, would be best).

This is on @MartinNowak's and my TODO list, e.g.

• Show warnings in comment (for now: regression PR not against master) dlang-bot#57
• Point out common mistakes dlang-bot#8

Of course help is welcome.

[CyberShadow]

Fine by me; going to leave this one open for a while more in case others have any opinions/arguments to present.

[CyberShadow]

Alrighty then