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 2020
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 2020
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 2020
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 2020
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 2020
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 2020
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 2020
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 2020
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 2020