[wip] ignore warnings with @suppress(key) comments

[CodeMyst]

very much work in progress, its very inefficient and error prone, but its a good working concept.

you can ignore warnings on specific lines by adding a suppressing comment to the end of the line, example:

void main()
{
    import std.stdio : writeln;

    int a = 5; // @suppress(dscanner.suspicious.unmodified)

    writeln(a);
}

some issues with my implementation:

• whether the suppress message is in a comment isn't checked
• for every warning im reading the file and finding the line of the warning, ideally this would be done once per file only

[WebFreak001]

ideally this should also support // @suppress(dscanner.suspicious) and // @suppress(dscanner), I also support that in serve-d and it's quite convenient

You probably want to do this using tokens instead of re-reading the file as this won't work with stdin input. (eg. basically all IDEs)

[CodeMyst]

can you guide me towards where i can get those tokens from?

[WebFreak001]

as far as I can currently see, they aren't accessible without hacks yet. However because they are stored with the nodes anyway it should be pretty safe to pass the tokens where the visitor is created.

[CodeMyst]

but then i'd need to pass the tokens to every analyzer and that'd be tedious to do, but doable i guess

[CodeMyst]

hmmm... looks like the tokens variable in run.d doesn't even contain comments 🤔

[CodeMyst]

even in the current repo, the way to get the // [warn] comments is without tokens and just pure string lookups. but those are read from a file and not stdin 🤔

D-Scanner/src/dscanner/analysis/helpers.d

Line 87 in 0ec1e95

if (codeLine.indexOf("// [warn]:") == -1)

[CodeMyst]

so i tried getting the comment from the tokens and i think i managed to do it, i can get the token.trailingComment for a token that's on the line with the warning. the issue is that the trailingComment won't contain a comment like // @suppress(dscanner.suspicious.unmodified), only if it's a triple slash comment (///)

would it be an issue if we just went with that? but then people who use serve-d or dls would have issues since for those you need a double slash comment afaik

[CodeMyst]

the addition of comments to the tokens is done here:

https://github.com/dlang-community/libdparse/blob/4b85ef36783cd47d7bcebc1b556b10b5b9c4fc5a/src/dparse/lexer.d#L415

but non doc comments are ignored

and i think using doc comments for suppressing warnings is a bad idea.

maybe add another field for storing the trailing non doc comment for a token?

[WebFreak001]

Those people are using an unofficial extension which isn't specified anywhere, so it's the fault of the developers. So try to ignore any such existing arguments when working around a problem.

However documentation comments aren't really good style to use, as they would then show up in documentation when for example ignoring the "undocumented variable" issue.

Maybe we could pick an approach similar to Java where we define UDAs @supress("") that can apply to code blocks, method definitions and single statements. For the parse tree this would be trivial to go through. The definition of the UDAs wouldn't matter as long as they are defined with a given name, so build tools and IDEs can define temporary files or include some dub package that defines the UDAs for the user. I haven't given this idea too much thought yet, but I think it's an interesting concept to think about.

[WebFreak001]

I will start a thread on the D forum asking for what the community thinks.

[CodeMyst]

Hmm I guess that would be a good way to go about it. I'm against IDE's to define it since it should be usable without one (like CI for example). And having it in a dub package would kind of be meh since dscanner doesn't require the source to be built, but would work.

Good idea on the forum thingy, once you create it you should link it here.

[WebFreak001]

the thread is on https://forum.dlang.org/post/[email protected] now

[WebFreak001]

SHOO has commented an approach which looks pretty good imo and generally matches the responses so far: https://forum.dlang.org/post/[email protected]

I would just go for a simpler syntax for the disable reasoning without quotes (we are in a comment and don't need complex string syntax after all)

So the suggestion:

// @dscanner suppress(suspicious.unmodified, some reasons)
...
// @dscanner unsuppress(suspicious.unmodified)

and

statement; // @dscanner suppress(suspicious.unmodified, some reasons)

If there is some code on the line before the suppress comment, it would be a single line suppression.

Do you think that's a good way to go?

[CodeMyst]

I like that way, and it should be simple to implement (if I figure out a way to get the comments, I guess I would have to store the code for the whole time instead of just disregarding after parsing the module like it does right now).

I will get started working on this tomorrow.

[Hackerpilot]

The lexer throws away all comments for the purpose of making the parser implementation sane. I didn't really have this kind of use case (keeping, and also skipping comments) in mind when making the lexer.

If you look at the dfmt code (the "format" function in src/dfmt/formatter.d) you can see that it actually lexes the code twice, and makes two sets of tokens. One set of tokens is sent to a parser so that the formatter has a bit of parse information available, and the actual formatting works on the other tokens array that include comments.

We may have to do something similar here, or possibly re-work the way that the lexing is done. Maybe get all the tokens and put all of the comment tokens into a container, putting all the "real" tokens into a normal array for the parser. We may need to use/reorganize the logic in the dparse library that attaches doc comments to tokens to get this to work well.

[CodeMyst]

welp, this now got too far away from my coding abilities, hopefully you can figure this stuff out.

this turned from a simple feature to a rewrite of how lexing works lol, but its for the better since i would just implement some janky way with no clean code at all

also i guess a new issue/pr should be made for the keeping comments thing and the discussion continued there

[WebFreak001]

if we go to improve the parse API we could go the approach Microsoft goes with Roslyn:

you have documents which contain the source code text, a syntax tree and all defined symbols (like dsymbol)

The syntax tree contains all information about the text and can be converted back into text without any loss of information. Every syntax tree is made up of nodes (child syntax trees), tokens and trivia (whitespace, comments)

Example:

See https://docs.microsoft.com/en-us/dotnet/csharp/roslyn-sdk/syntax-visualizer?tabs=csharp

[Hackerpilot]

if we go to improve the parse API we could go the approach Microsoft goes with Roslyn:

I was just suggesting making a change to the lexing API. One that would allow us to get tokens for parsing as well as a collection of comments, and doing this in only one pass instead of the two passes that dfmt does.

[WebFreak001]

I was just suggesting making a change to the lexing API. One that would allow us to get tokens for parsing as well as a collection of comments

I think that's an approach with limited usability. It would fit in here perfectly, but other tools working with both the AST and also comments might see only limited use without putting in major work to process both at the same time.

It is a very compatible solution with the current code, but I'm not quite in agreement if that's the path we should follow ultimately. Some tools might want information for example about whitespaces in the future to do formatting. Reversing the AST tree into source code (without loss of information) is also something which is currently not possible but is definitely something with major applications. (like dfix)

[Hackerpilot]

What you're talking about is a good idea for a new major version of dparse, and I think that we should make changes to the AST and parser code to allow that use case. However, what I'm talking about is a new/enhanced version of the getTokensForParser function that will allow D-Scanner to access comments and for dfmt to lex the code only once instead of twice. These immediate goals can be accomplished with some fairly minimal effort.

[CodeMyst]

Would adding fields for leading and trailing non doc comments to tokens be enough for that? It should be doable to do it in one pass, currently there is some code for non doc comments but in the switch they are just ignored here:

https://github.com/dlang-community/libdparse/blob/1c6c21d2851ee2fb5c2f15b04b86e6eea0c0da78/src/dparse/lexer.d#L475-L477

[CodeMyst]

bump?

[WebFreak001]

I think having comments (and whitespace) attached as token prefix/suffixes would be pretty good

[WebFreak001]

@CodeMyst made a PR to implement what you need (and more) now: dlang-community/libdparse#387

[WebFreak001]

@CodeMyst started trying the new API yet? If you need it I can tag a new beta release with the comments/whitespace changes for you to test but you can also locally check it out or use ~master in dub (+ dub upgrade)

[CodeMyst]

Sorry for the inactivity, haven't tried it yet. I'll go back to working on this in the next couple of days.

[CodeMyst]

how should i get the latest changes for libdparse? can i just pull the submodule?

[CodeMyst]

so, i pulled the libdparse submodule, but since the current d-scanner doesn't use the latest version of libdparse there are some errors:

src\dscanner\analysis\auto_function.d(81): Error: cannot implicitly convert expression p.primary of type const(TokenStructure!(ubyte, "import dparse.lexer:TokenTriviaFields; mixin TokenTriviaFields;")) to immutable(TokenStructure!(ubyte, "import dparse.lexer:TokenTriviaFields; mixin TokenTriviaFields;"))
src\dscanner\analysis\mismatched_args.d(136): Error: cannot implicitly convert expression iot.identifier of type const(TokenStructure!(ubyte, "import dparse.lexer:TokenTriviaFields; mixin TokenTriviaFields;")) to immutable(TokenStructure!(ubyte, "import dparse.lexer:TokenTriviaFields; mixin TokenTriviaFields;"))
src\dscanner\analysis\useless_assert.d(47): Error: cannot implicitly convert expression unary.primaryExpression.primary of type TokenStructure!(ubyte, "import dparse.lexer:TokenTriviaFields; mixin TokenTriviaFields;") to immutable(TokenStructure!(ubyte, "import dparse.lexer:TokenTriviaFields; mixin TokenTriviaFields;"))

[Hackerpilot]

You can try again now that dlang-community/libdparse#393 is merged.

[CodeMyst]

thanks! ill try it again

[dgellow]

For reference, once this PR is merged that should fix #240.

[WebFreak001]

before this is merged when this feature is finished: if the libdparse API works well, we will have to update libdparse on dub. (at least make a beta release mid development too)

[rikkimax]

How's it going @CodeMyst @WebFreak001?