www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Disabling warnings in D-Scanner: looking for suggestions

reply WebFreak001 <d.forum webfreak.org> writes:
Currently in D-Scanner it is only possible to disable warnings 
for the entire test suite, but not on a per-line basis. So we 
would like to ask for some suggestions what would be good 
constructs to achieve this. (See discussion on 
https://github.com/dlang-community/D-Scanner/pull/799 )

Some suggestions:

disabling warnings per-line using `// 
 suppress(dscanner.suspicious.unmodified)`
pros:
- relatively easy to implement (requires a full copy of source 
code in memory)
- supported by dls and serve-d already
cons:
- blows up line length
- not very flexible to ignore more than one type
- hard to verify (for example misspelling suppress)


more fully featured comments to disable
pros:
- very flexible
- could reuse syntax from well-known linting tools
cons:
- potentially very hard to implement parsing
- potentially reimplementing scoping to figure out where blocks 
start and end
- even harder to verify misspelling


using a Java-like  suppress UDA in the source code to mark blocks 
and statements
pros:
- easy to ignore warnings for whole classes
- very easy to implement when having an AST
- doesn't break when adding line breaks to a multi-line variable 
definition
- auto completion included, can easily extend UDA definitions & 
parameters with documentation and new warning codes
cons:
- easy to ignore warnings for whole classes (bad practice)
- hard to mark only certain lines (like template parameters, 
singular if checks, etc)
- D grammar doesn't allow UDA blocks inside statements
- would need some generated UDA definition file or additional dub 
package to depend on to compile successfully



[warning codes]) and pragma(warning, restore, [warning codes])
pros:
- good syntax support (supported in most syntax parts)
- can enable/disable anywhere without requiring extra nesting in 
{} blocks
- easy to understand exact effect range
- could offer integration with disabling built-in dmd warnings 
(like dead code)
cons:
- would need to extend D's spec (and would only start working 
with newer D versions) as currently unknown pragmas, even vendor 
defined, are defined to error
- would need more precise specification for warning codes
-> would take a lot of time to implement


What kind of suppression system would you prefer to see in 
D-Scanner, what would you prefer not to see? Open for suggestions.
Mar 23
next sibling parent kinke <noone nowhere.com> writes:
On Monday, 23 March 2020 at 13:15:08 UTC, WebFreak001 wrote:
 more fully featured comments to disable
 pros:
 - very flexible
 - could reuse syntax from well-known linting tools
Definitely this.
Mar 23
prev sibling next sibling parent reply Dennis <dkorpel gmail.com> writes:
I actively try to reduce D-scanner warnings in my projects 
appearing in the VS Code 'problems' tab (using your super-handy 
code-d extension, thanks for developing that!), but I never put 
comments in to suppress warnings.

They clutter up the code and I don't like them.

I know code-d supports the `// stfu` comment, which is almost as 
simple and unobtrusive as it gets, but even then I don't like the 
idea of someone else reading my code and wondering where those 
random profanities come from. Explicit  suppress comments are 
clearer in that regard, but they are long and tie your code to 
dscanner. You might need an additional set of suppression 
comments once you add a different linting tool, and it only gets 
worse.

That's why I only resolve warnings by modifying the offending 
code:

 calling foo without side effects discards return value of type 
 string, prepend a cast(void) if intentional
This particular warning is generated by dub, but dscanner gives a similar warning for unused parameters, though code-d only grays unused parameters instead of adding a problem. In any case, casting by void is great since it is short and clear, and it transfers to any linting tool. It even transfers to C/C++ in this case. Other examples how I remove warnings are:
 Local imports should specify the symbols being imported to 
 avoid hiding local symbols
Specify the symbols or move the import to global scope.
 Undocumented public declaration
Add a documentation comment to the declaration (even just an empty /// ) or make it private / package.
 Line longer than 120 characters
Split up the line.
 Variable name does not match style guidelines
Change the capitalization. You can make an alias to the symbol with the original capitalization since aliases don't have a style rule. There are a few warnings that can't easily be solved however, like the 'dscanner.suspicious.unmodified' you mentioned. ``` int x; // dscanner says: 'could be declared const' increment(x); // but this function takes arguments by ref! x += 0; // so should I just do some bogus 'modification'? ``` Ideally, I would like to the language to support `increment(ref x)` on the call site (similar to what you suggest: https://forum.dlang.org/thread/dhiwpttqeaxctdzczxlb forum.dlang.org). Though we can't count on that. My most hated warning is:
 Avoid subtracting from '.length' as it may be unsigned.
Sometimes you can rewrite e.g. (x < a.length-1) to (x + 1 < a.length), but other times there's simply no way around this, and I need to access element `a.length-1`, and Dscanner can't see it's in side an `if (a.length > 0)` block. Regarding your suggestions:
 more fully featured comments to disable
I don't know what 'fully featured' means. Can you elaborate this one?
 using a Java-like  suppress UDA in the source code to mark 
 blocks and statements
 ...
 - easy to ignore warnings for whole classes (bad practice)
I'm not sure I would call surpressing warnings at large scale is bad practice per se. When I open e.g. bindbc-opengl, dscanner gives 1000 problems of 'undocumented public declarations', while of course there aren't 1000 individual cases where documentaiton was forgotten. It is just raw bindings with no documentation, that is only worth mentioning once. Also a huge large array literal might spawn a lot of warnings that every line is too long, really cluttering up the problems tab.

 [warning codes]) and pragma(warning, restore, [warning codes])
In that case, I feel the warnings should be defined by the language and not the linting tool.
 What kind of suppression system would you prefer to see in 
 D-Scanner, what would you prefer not to see? Open for 
 suggestions.
As I said, ideally you never have to suppress warnings. I know sometimes it's hard to get around it, but I don't know a good solution for that unfortunately.
Mar 24
parent reply WebFreak001 <d.forum webfreak.org> writes:
a few comments about your linter fixes:

On Tuesday, 24 March 2020 at 21:03:22 UTC, Dennis wrote:
 I know code-d supports the `// stfu` comment, which is almost 
 as simple and unobtrusive as it gets, but even then I don't 
 like the idea of someone else reading my code and wondering 
 where those random profanities come from.
well this is actually just supposed to be an easter egg :)
 Line longer than 120 characters
Split up the line.
sometimes this isn't possible very well (like long embedded strings)
 Variable name does not match style guidelines
Change the capitalization. You can make an alias to the symbol with the original capitalization since aliases don't have a style rule.
often this happens when you explicitly make something not match style guidelines (UDA struct, ABI/mangling compatibility for some binding, etc)
 There are a few warnings that can't easily be solved however, 
 like the 'dscanner.suspicious.unmodified' you mentioned.
 ```
 int x; // dscanner says: 'could be declared const'
 increment(x); // but this function takes arguments by ref!
 x += 0; // so should I just do some bogus 'modification'?
 ```

 Ideally, I would like to the language to support `increment(ref 
 x)` on the call site (similar to what you suggest:
 https://forum.dlang.org/thread/dhiwpttqeaxctdzczxlb forum.dlang.org).
 Though we can't count on that.
Yep repeatedly falling into this one, really sucks.
 My most hated warning is:

 Avoid subtracting from '.length' as it may be unsigned.
Sometimes you can rewrite e.g. (x < a.length-1) to (x + 1 < a.length), but other times there's simply no way around this, and I need to access element `a.length-1`, and Dscanner can't see it's in side an `if (a.length > 0)` block.
here is a "fix": a.length + -1
 Regarding your suggestions:

 more fully featured comments to disable
I don't know what 'fully featured' means. Can you elaborate this one?
for example PC-Lint includes some syntax like //lint e715 // disable "'argument' not referenced " for entire module //lint +e715 // enable "'argument' not referenced" again for module //lint --e(715) // disable 'argument' not referenced once however they have extremely complicated additional syntax too, but it's hard to look up this stuff. We would need to specify all the possibilities what the dscanner lint ignores could do then (like if it should be possible to add some special comment before a block to disable a warning for the entire block body)
Mar 24
parent reply Dennis <dkorpel gmail.com> writes:
On Tuesday, 24 March 2020 at 21:58:43 UTC, WebFreak001 wrote:
 sometimes this isn't possible very well (like long embedded 
 strings)
You can split those string literals up and concatenate with ~, though you do lose the convenience of a single embedded string literal.
 often this happens when you explicitly make something not match 
 style guidelines (UDA struct, ABI/mangling compatibility for 
 some binding, etc)
Yes, that's when I run into this warning too.
 here is a "fix": a.length + -1
That's... actually not that bad, thanks! It doesn't really express "I know this is unsigned, but it can't underflow" but if it pleases D-Scanner, I might take it. :p
 for example PC-Lint includes some syntax like
 //lint e715    // disable "'argument' not referenced " for 
 entire module
 //lint +e715    // enable "'argument' not referenced" again for 
 module
 //lint --e(715)    // disable 'argument' not referenced once

 however they have extremely complicated additional syntax too, 
 but it's hard to look up this stuff.
If you sell it like that, it's an obvious no for me. Upon encountering 'lint e715' it's not only unknown what kind of warning the code 'e715' refers to, but 'lint' doesn't tell me what reference I should use either. Of course D-Scanner can do something elaborate better, but I think suppressing warnings should be super simple. Maybe even something as simple as `// stfu`, but then something like `// warning: text explaining the warning` that binds to a declaration / statement using the same rules as Ddoc. D-Scanner only looks for the 'warning:' part, and from context or the explanation text it becomes clear what the raised warning would be. You could mass-disable bindings in a module like this: ``` // warning: bindings do not adhere to D-style and are undocumented module my.windows.bindings; ``` This scheme has a risk that a comment like this:
 warning: does not match style guide because it's a UDA
Would also suppress a valid 'missing documentation for public symbol' warning. And disabling all warnings for an entire module is blunt; some users _will_ request a way to have granular control over it. I don't think you can escape that. But I won't be one of those users, so I don't have much to say there. I hope some other people respond to this thread, it's pretty quiet so far. Maybe few users actually use D scanner?
Mar 24
parent reply CodeMyst <codemyst outlook.com> writes:
On Wednesday, 25 March 2020 at 00:26:01 UTC, Dennis wrote:
 On Tuesday, 24 March 2020 at 21:58:43 UTC, WebFreak001 wrote:
 sometimes this isn't possible very well (like long embedded 
 strings)
You can split those string literals up and concatenate with ~, though you do lose the convenience of a single embedded string literal.
 often this happens when you explicitly make something not 
 match style guidelines (UDA struct, ABI/mangling compatibility 
 for some binding, etc)
Yes, that's when I run into this warning too.
 here is a "fix": a.length + -1
That's... actually not that bad, thanks! It doesn't really express "I know this is unsigned, but it can't underflow" but if it pleases D-Scanner, I might take it. :p
 for example PC-Lint includes some syntax like
 //lint e715    // disable "'argument' not referenced " for 
 entire module
 //lint +e715    // enable "'argument' not referenced" again 
 for module
 //lint --e(715)    // disable 'argument' not referenced once

 however they have extremely complicated additional syntax too, 
 but it's hard to look up this stuff.
If you sell it like that, it's an obvious no for me. Upon encountering 'lint e715' it's not only unknown what kind of warning the code 'e715' refers to, but 'lint' doesn't tell me what reference I should use either. Of course D-Scanner can do something elaborate better, but I think suppressing warnings should be super simple. Maybe even something as simple as `// stfu`, but then something like `// warning: text explaining the warning` that binds to a declaration / statement using the same rules as Ddoc. D-Scanner only looks for the 'warning:' part, and from context or the explanation text it becomes clear what the raised warning would be. You could mass-disable bindings in a module like this: ``` // warning: bindings do not adhere to D-style and are undocumented module my.windows.bindings; ``` This scheme has a risk that a comment like this:
 warning: does not match style guide because it's a UDA
Would also suppress a valid 'missing documentation for public symbol' warning. And disabling all warnings for an entire module is blunt; some users _will_ request a way to have granular control over it. I don't think you can escape that. But I won't be one of those users, so I don't have much to say there. I hope some other people respond to this thread, it's pretty quiet so far. Maybe few users actually use D scanner?
I'm mostly in favour of simple `// suppress` comments. You talked about it cluttering up code and making it ugly which is true, but this shouldn't even be used that often since there are few cases where you will want to suppress warnings. And about the use of multiple linters, is there even another linter for D available? And the warning comments with an explanation have an issue like you mentioned that they will mute every warning on a line, even the ones you would like to have, with the simpler suppress comments you could have: ``` // warning is suppressed here because of blahblah int a = 5; // suppress(dscanner.suspicious.unmodified) foo(a); ``` It does look a bit ugly, but you're doing something against standards so it should catch your eye I guess... And muting warnings for the whole module could be done by adding a `// suppress(dscanner)` on the module declaration.
Mar 25
parent Dennis <dkorpel gmail.com> writes:
On Wednesday, 25 March 2020 at 11:50:14 UTC, CodeMyst wrote:
 And about the use of multiple linters, is there even another 
 linter for D available?
dmd can give warnings about unreachable code even in generic template code. I don't think you can suppress them, but if a way to suppress them is added, it won't be a 'dscanner' comment, it will be another mechanism. That aside, it's true that currently D-Scanner is the only dedicated D linter. My concern comes from some over-linted TypeScript/Java projects I've worked on in the past, where the automatic code-formatter would battle it out with the style checker, or Eclipse would want it one way while PMD / Findbugs want it another way. Maybe this won't become a problem for D, but I'm simply wary of a design based on the idea that D-scanner is the one and only linter for D code.
Mar 25
prev sibling parent reply SHOO <zan77137 nifty.com> writes:
On Monday, 23 March 2020 at 13:15:08 UTC, WebFreak001 wrote:
 What kind of suppression system would you prefer to see in 
 D-Scanner, what would you prefer not to see? Open for 
 suggestions.
I am working in C using MISRA-C static analysis. According to the findings made there, the suppression of warnings by comments is definitely necessary. Too many warnings can discourage developers and bury important warnings. In MISRA-C, daring to ignore warnings is called "deviation". And to deviate, a deviation report is required. Even in the D-Scanner lint, a statement of reasons seems to be necessary for suppression. In addition, there are three code units that require suppression - File unit suppression: For example, binding would fall under this category. ``` // dscanner suppress(suspicious.unmodified, "some reasons") foo(); ``` - Block unit suppression: Wrapping multiple warnings in a single function can be useful in consolidating the areas that need attention. ``` void foo() { // dscanner suppress(suspicious.unmodified, "some reasons") hoge(); fuga(); piyo(); // dscanner unsuppress(suspicious.unmodified) } ``` - Line unit suppression: This is useful when suppressing a single warning. (but I don't like this, because lines tend to be longer and less readable; I prefer block unit suppression over line unit suppression, even if it is for a single line.) ``` foo(); // dscanner-suppress(suspicious.unmodified, "some reasons") ```
Mar 28
parent WebFreak001 <d.forum webfreak.org> writes:
On Saturday, 28 March 2020 at 19:40:49 UTC, SHOO wrote:
 On Monday, 23 March 2020 at 13:15:08 UTC, WebFreak001 wrote:
 [...]
I am working in C using MISRA-C static analysis. According to the findings made there, the suppression of warnings by comments is definitely necessary. Too many warnings can discourage developers and bury important warnings. In MISRA-C, daring to ignore warnings is called "deviation". And to deviate, a deviation report is required. Even in the D-Scanner lint, a statement of reasons seems to be necessary for suppression. In addition, there are three code units that require suppression - File unit suppression: For example, binding would fall under this category. ``` // dscanner suppress(suspicious.unmodified, "some reasons") foo(); ``` - Block unit suppression: Wrapping multiple warnings in a single function can be useful in consolidating the areas that need attention. ``` void foo() { // dscanner suppress(suspicious.unmodified, "some reasons") hoge(); fuga(); piyo(); // dscanner unsuppress(suspicious.unmodified) } ``` - Line unit suppression: This is useful when suppressing a single warning. (but I don't like this, because lines tend to be longer and less readable; I prefer block unit suppression over line unit suppression, even if it is for a single line.) ``` foo(); // dscanner-suppress(suspicious.unmodified, "some reasons") ```
I think this is a good suggestion. Overall I'm also for the comments starting/ending multi-line blocks + single line suppression. I'm also strongly for descriptions why to ignore warnings (we also follow the MISRA-C guidelines) I suggested your syntax (but slightly simpler, without quotes because we are in a comment block) in the PR page. But overall I think that's a good and simple syntax we can standardize on. With the syntax it's currently only possible to suppress/unsuppress single warnings per line, but I think that's an alright limitation.
Mar 30