Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Duplicate diagnostics when running a task with the $go problemMatcher #2100

Closed
segevfiner opened this issue Nov 5, 2018 · 6 comments · Fixed by #2109
Closed

Duplicate diagnostics when running a task with the $go problemMatcher #2100

segevfiner opened this issue Nov 5, 2018 · 6 comments · Fixed by #2109

Comments

@segevfiner
Copy link
Contributor

segevfiner commented Nov 5, 2018

Steps to Reproduce

  1. Build any go package, when go.buildOnSave is enabled, with the following task definition:
{
    "label": "build",
    "type": "shell",
    "command": "go build -v",
    "problemMatcher": "$go",
    "group": {
        "kind": "build",
        "isDefault": true
    }
}
  1. Observe the duplicate errors.

image

Analysis

VS Code de-duplicates diagnostics based on their owner. For tasks this is set in the problemMatcher (The go problem matcher is defined here: vscode/src/vs/workbench/parts/tasks/common/problemMatcher.ts:1798-1807). While for extensions this is the name of the DiagnosticCollection (See vscode/src/vs/workbench/api/node/extHostDiagnostics.ts:258). In order for diagnostics generated by go.buildOnSave to be de-duplicated with diagnostics generated by tasks we need to use a single DiagnosticCollection, and name it go.

P.S. We should also set Diagnostic.source. It should be just go for build tasks, and probably the name of the linter, for linters. I will open a separate issue for this. EDIT: Created #2101

@segevfiner
Copy link
Contributor Author

I wonder if the right way to handle this is to define a DiagnosticCollection for each type of diagnostics source rather than the diagnostics severity, something like this:

buildDiagnosticCollection = vscode.languages.createDiagnosticCollection('go');
lintDiagnosticCollection = vscode.languages.createDiagnosticCollection('go-lint');
vetDiagnosticCollection = vscode.languages.createDiagnosticCollection('go-vet');
testDiagnosticCollection = vscode.languages.createDiagnosticCollection('go-test');

That way we only clear the collection when running the corresponding type of tool, and we can correctly set the owner of build errors to go to fix the duplication issue.

segevfiner added a commit to segevfiner/vscode-go that referenced this issue Nov 9, 2018
Also set setting diagnostic.source

Fixes microsoft#2100
segevfiner added a commit to segevfiner/vscode-go that referenced this issue Nov 9, 2018
Also set setting diagnostic.source

Fixes microsoft#2100, fixes microsoft#2101
@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Nov 11, 2018

If we had a task definition that runs the linter, then we would still have the duplicate problem situation, right?

Is there any situation where one would want both the build/lint/vet via tasks as well as build/lint/run via the save operation?

@segevfiner
Copy link
Contributor Author

If we had a task definition that runs the linter, then we would still have the duplicate problem situation, right?

Is there any situation where one would want both the build/lint/vet via tasks as well as build/lint/run via the save operation?

You wouldn't have the problem if you specify that the owner of the linter task's problem matcher is go-lint after this PR. (tasks.json schema.

The current go.buildOnSave builds the output into a temporary directory. If you want the final binary you will need to executego build separately. This makes sense, as overwriting the built binary on each save is quite unexpected, unless of course, the user configures such a thing explicitly.

This makes it likely for the user to want to configure a task that runs go build, at that point, it's also likely he will select the $go problem matcher as VS Code prompts for it, if not specified otherwise. This will lead to the duplicate diagnostics issue. Of course you can simply leave the task without a problem matcher as the automatic build on save will produce the same diagnostics, save for the Diagnostic.source property. But it stands to reason that the VS Code developers thought about this, and as such, introduced the de-duplication between diagnostics that share the same DiagnosticCollection.name and owner.

Another situation is a user defining a task that runs the linter on a larger scope than that configured for the automatic on save linting.

As such, it seems to me that that's the way DiagnosticCollection are meant to be used.

@ramya-rao-a
Copy link
Contributor

You wouldn't have the problem if you specify that the owner of the linter task's problem matcher is go-lint after this PR

This would require the user to be aware of this and know how to define a new problem matcher.

We can solve the second part of the problem by contributing the go-lint and go-vet problem matchers as part of this extension.

@segevfiner
Copy link
Contributor Author

This would require the user to be aware of this and know how to define a new problem matcher.

They kinda have to know how to do that to hit this issue in the first place. 😜 The owner thing is not that well explained in the docs though: Defining a problem matcher, and is only really explained in the tasks.json schema.

We can solve the second part of the problem by contributing the go-lint and go-vet problem matchers as part of this extension.

Yeah. We might have to contribute multiple problem matchers in case the linters have different patterns. Just with the same owner field (You can have multiple problem matchers with the same owner).

ramya-rao-a pushed a commit that referenced this issue Nov 13, 2018
* Split up the DiagnosticCollections per tool type

Also set setting diagnostic.source

Fixes #2100, fixes #2101

* Simplify handleDiagnosticErrors a bit

* Also dedupe build & lint warnings together

* Clear all diagnostics in runBuilds

* Small refactorings

* Revert test.only
@ramya-rao-a
Copy link
Contributor

The fix for this is now available in the latest version of the Go extension (0.8.0)
Thanks @segevfiner!

@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants