gopls: deduplicate diagnostics from gopls / linters

[ramya-rao-a]

Below is an example where we have the same diagnostic being presented by both the language server and the linter

When LSP was not in the picture, such duplicates were removed in https://github.com/microsoft/vscode-go/blob/0.10.2/src/util.ts#L747 and https://github.com/microsoft/vscode-go/blob/0.10.2/src/util.ts#L756

We might have to do something similar to remove duplicates between LSP and the linting tool.

This issue is created from microsoft/vscode-go#2517 by @lggomez

[stamblerre]

My preference here would be that we leave this behavior as-is. golint will likely be deprecated soon (golang/go#38968) and staticcheck is already part of gopls, which leaves golangci-lint and revive.

The error shown in the image here is a build error, so I would hope that these linters don't return such errors. If there are duplicate errors, I'm not sure that it is worth deduping them.

[dominikh]

The error shown in the image here is a build error, so I would hope that these linters don't return such errors. If there are duplicate errors, I'm not sure that it is worth deduping them.

Linters do return build errors if being run on code with build errors.

[stamblerre]

Hm ok, I didn't realize. Then maybe this issue is specific to golint, and if that's deprecated we can deprecate it as a lintOnSave option.

[ramya-rao-a]

You can try changing the go.lintTool to other tools and see what the behavior then is

[ramya-rao-a]

But it has happened that go vet and a linter would provide the same diagnostics. If gopls does vetting, then the case of duplication would still be valid and also the need for de-duplicaton

[stamblerre]

Sure. Adding a bit of deduplication will not be difficult, as long as the error range and message are exactly the same.

[hyangah]

The existing deduplication code for go vet/lint/build errors:

vscode-go/src/util.ts

Lines 908 to 932 in cf03db6

	diagnosticMap.forEach((newDiagnostics, file) => {
	const fileUri = vscode.Uri.parse(file);
	if (diagnosticCollection === buildDiagnosticCollection) {
	// If there are lint/vet warnings on current file, remove the ones co-inciding with the new build errors
	if (lintDiagnosticCollection && lintDiagnosticCollection.has(fileUri)) {
	lintDiagnosticCollection.set(
	fileUri,
	deDupeDiagnostics(newDiagnostics, lintDiagnosticCollection.get(fileUri).slice())
	);
	}
	if (vetDiagnosticCollection && vetDiagnosticCollection.has(fileUri)) {
	vetDiagnosticCollection.set(
	fileUri,
	deDupeDiagnostics(newDiagnostics, vetDiagnosticCollection.get(fileUri).slice())
	);
	}
	} else if (buildDiagnosticCollection && buildDiagnosticCollection.has(fileUri)) {
	// If there are build errors on current file, ignore the new lint/vet warnings co-inciding with them
	newDiagnostics = deDupeDiagnostics(buildDiagnosticCollection.get(fileUri).slice(), newDiagnostics);
	}
	diagnosticCollection.set(fileUri, newDiagnostics);
	});
	}

BuildOnSave also provides diagnostics so will create duplicates.

We can intercept the diagnostics from Language Server using handleDiagnostics middleware. (e.g. https://go-review.googlesource.com/c/vscode-go/+/222338/1/src/goLanguageServer.ts)

cc @suzmue

[gopherbot]

Change https://golang.org/cl/279212 mentions this issue: src/goLint: switch the default lint tool to staticcheck

[gopherbot]

Change https://golang.org/cl/282557 mentions this issue: src/goLanguageServer.ts: deduplicate diagnostics from language client