src/util.ts: handle diagnostics errors for all visible files
Error messages from lint, vet, or go compiler often contain only the
start position's line number, and sometimes the column info, even though
vscode's DiagnosticCollection expects complete Ranges (both start/end
positions).
In handleDiagnosticErrors, the vscode extension implements a heuristic
to fill in the missing fields to construct complete Range entries.
For the file that is open and visible when the lint/vet/go build runs,
it finds the corredponding row, sets the start position to be the beggining
of the non-whitespace text part if the start column info is not available
from the tool's error message, and sets the end position to be the end of
the non-whitespace text part. For errors that belong to files that aren't
open or do not have focus, it sets the start column 0 and the end column 1
unconditionally.
This had a side-effect as described in golang/vscode-go#743 - when multiple
files with problems are open, the error location info for the file behind
isn't completed in a way it would've been handled if it was on the top.
The location info is not completely incorrect in the sense that that's
what the underlying tool provided, but visually it is not great.
This CL makes two changes
- Apply the handleDiagnosticError's heuristic to construct complete
Range information to all text documents 'known' to vscode. Here 'known'
text documents mean the document that is open and also vscode already
loaded the file contents in memory. That means, it's possible that
a file that is open but whose content is not loaded into memory may
still exhibit the problem described in golang/vscode-go#743. But I don't
find an efficient, clean way to handle the case.
- Apply the column info from the tools if available, instead of using position
0 unconditionally when the text document is not 'known' to vscode yet.
I expect in the future, gopls will handle vet/lint errors and provide
better range info than this, so this will become a more rare corner case.
Fixes golang/vscode-go#743
Change-Id: I5c953aa710a14a59d00ea7e94d4028ca05316362
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/259797
Trust: Hyang-Ah Hana Kim <hyangah@gmail.com>
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
diff --git a/src/util.ts b/src/util.ts
index 1c49bf8..279092c 100644
--- a/src/util.ts
+++ b/src/util.ts
@@ -791,7 +791,7 @@
atLeastSingleMatch = true;
const [, , file, , lineStr, , colStr, msg] = match;
const line = +lineStr;
- const col = +colStr;
+ const col = colStr ? +colStr : undefined;
// Building skips vendor folders,
// But vet and lint take in directories and not import paths, so no way to skip them
@@ -805,7 +805,7 @@
const filePath = path.resolve(cwd, file);
ret.push({ file: filePath, line, col, msg, severity });
- outputChannel.appendLine(`${filePath}:${line}: ${msg}`);
+ outputChannel.appendLine(`${filePath}:${line}:${col ?? ''} ${msg}`);
}
if (!atLeastSingleMatch && unexpectedOutput && vscode.window.activeTextEditor) {
outputChannel.appendLine(stderr);
@@ -836,21 +836,37 @@
diagnosticCollection.clear();
const diagnosticMap: Map<string, vscode.Diagnostic[]> = new Map();
+
+ const textDocumentMap: Map<string, vscode.TextDocument> = new Map();
+ if (document) {
+ textDocumentMap.set(document.uri.toString(), document);
+ }
+ // Also add other open .go files known to vscode for fast lookup.
+ vscode.workspace.textDocuments.forEach((t) => {
+ const fileName = t.uri.toString();
+ if (!fileName.endsWith('.go')) { return; }
+ textDocumentMap.set(fileName, t);
+ });
+
errors.forEach((error) => {
const canonicalFile = vscode.Uri.file(error.file).toString();
- let startColumn = 0;
- let endColumn = 1;
- if (document && document.uri.toString() === canonicalFile) {
+ let startColumn = error.col ? error.col - 1 : 0;
+ let endColumn = startColumn + 1;
+ // Some tools output only the line number or the start position.
+ // If the file content is available, adjust the diagnostic range so
+ // the squiggly underline for the error message is more visible.
+ const doc = textDocumentMap.get(canonicalFile);
+ if (doc) {
const tempRange = new vscode.Range(
error.line - 1,
0,
error.line - 1,
- document.lineAt(error.line - 1).range.end.character + 1
+ doc.lineAt(error.line - 1).range.end.character + 1 // end of the line
);
- const text = document.getText(tempRange);
+ const text = doc.getText(tempRange);
const [_, leading, trailing] = /^(\s*).*(\s*)$/.exec(text);
if (!error.col) {
- startColumn = leading.length;
+ startColumn = leading.length; // beginning of the non-white space.
} else {
startColumn = error.col - 1; // range is 0-indexed
}
@@ -873,20 +889,20 @@
if (diagnosticCollection === buildDiagnosticCollection) {
// If there are lint/vet warnings on current file, remove the ones co-inciding with the new build errors
- if (lintDiagnosticCollection.has(fileUri)) {
+ if (lintDiagnosticCollection && lintDiagnosticCollection.has(fileUri)) {
lintDiagnosticCollection.set(
fileUri,
deDupeDiagnostics(newDiagnostics, lintDiagnosticCollection.get(fileUri).slice())
);
}
- if (vetDiagnosticCollection.has(fileUri)) {
+ if (vetDiagnosticCollection && vetDiagnosticCollection.has(fileUri)) {
vetDiagnosticCollection.set(
fileUri,
deDupeDiagnostics(newDiagnostics, vetDiagnosticCollection.get(fileUri).slice())
);
}
- } else if (buildDiagnosticCollection.has(fileUri)) {
+ } 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);
}
diff --git a/test/integration/extension.test.ts b/test/integration/extension.test.ts
index 1b42970..385e554 100644
--- a/test/integration/extension.test.ts
+++ b/test/integration/extension.test.ts
@@ -35,6 +35,7 @@
getGoConfig,
getImportPath,
getToolsGopath,
+ handleDiagnosticErrors,
ICheckResult,
isVendorSupported
} from '../../src/util';
@@ -386,6 +387,29 @@
assert.equal(processutil.killProcessTree.callCount, 1, 'should have killed 1 lint job before launching the next');
});
+ test('Linting - lint errors with multiple open files', async () => {
+ // handleDiagnosticErrors may adjust the lint errors' ranges to make the error more visible.
+ // This adjustment applies only to the text documents known to vscode. This test checks
+ // the adjustment is made consistently across multiple open text documents.
+ const file1 = await vscode.workspace.openTextDocument(vscode.Uri.file(path.join(fixturePath, 'linterTest', 'linter_1.go')));
+ const file2 = await vscode.workspace.openTextDocument(vscode.Uri.file(path.join(fixturePath, 'linterTest', 'linter_2.go')));
+ const warnings = await goLint(file2.uri, Object.create(vscode.workspace.getConfiguration('go'), {
+ lintTool: { value: 'golint' },
+ lintFlags: { value: [] }
+ }), 'package');
+
+ const diagnosticCollection = vscode.languages.createDiagnosticCollection('linttest');
+ handleDiagnosticErrors(file2, warnings, diagnosticCollection);
+
+ // The first diagnostic message for each file should be about the use of MixedCaps in package name.
+ // Both files belong to the same package name, and we want them to be identical.
+ const file1Diagnostics = diagnosticCollection.get(file1.uri);
+ const file2Diagnostics = diagnosticCollection.get(file2.uri);
+ assert(file1Diagnostics.length > 0);
+ assert(file2Diagnostics.length > 0);
+ assert.deepStrictEqual(file1Diagnostics[0], file2Diagnostics[0]);
+ });
+
test('Error checking', async () => {
const config = Object.create(vscode.workspace.getConfiguration('go'), {
vetOnSave: { value: 'package' },
