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


extending D's pragma like in C# for pragma(warning, disable, [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.

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.

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.

> extending D's pragma like in C# for pragma(warning, disable, [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.

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)

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?

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.

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.

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")
```

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.