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

Commit

Permalink
Split up the DiagnosticCollections per tool type (#2109)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@segevfiner @ramya-rao-a
segevfiner authored and ramya-rao-a committed Nov 13, 2018
1 parent 633d1f2 commit 03dc46a
Show file tree
Hide file tree
Showing 8 changed files with 79 additions and 67 deletions.
5 changes: 3 additions & 2 deletions src/goBuild.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { getTestFlags } from './testUtils';
import { getCurrentGoWorkspaceFromGOPATH } from './goPath';
import { diagnosticsStatusBarItem } from './goStatus';
import { isModSupported } from './goModules';
import { buildDiagnosticCollection } from './goMain';
/**
* Builds current package or workspace.
*/
Expand All @@ -34,7 +35,7 @@ export function buildCode(buildWorkspace?: boolean) {
isModSupported(documentUri).then(isMod => {
goBuild(documentUri, isMod, goConfig, buildWorkspace)
.then(errors => {
handleDiagnosticErrors(editor ? editor.document : null, errors, vscode.DiagnosticSeverity.Error);
handleDiagnosticErrors(editor ? editor.document : null, errors, buildDiagnosticCollection);
diagnosticsStatusBarItem.hide();
})
.catch(err => {
Expand Down Expand Up @@ -132,4 +133,4 @@ export function goBuild(fileUri: vscode.Uri, isMod: boolean, goConfig: vscode.Wo

let epoch = 0;
let tokenSource: vscode.CancellationTokenSource;
let running = false;
let running = false;
22 changes: 15 additions & 7 deletions src/goCheck.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { goLint } from './goLint';
import { goVet } from './goVet';
import { goBuild } from './goBuild';
import { isModSupported } from './goModules';
import { buildDiagnosticCollection, lintDiagnosticCollection, vetDiagnosticCollection } from './goMain';

let statusBarItem = vscode.window.createStatusBarItem(vscode.StatusBarAlignment.Left);
statusBarItem.command = 'go.test.showOutput';
Expand Down Expand Up @@ -46,7 +47,12 @@ export function notifyIfGeneratedFile(e: vscode.TextDocumentChangeEvent) {
}
}

export function check(fileUri: vscode.Uri, goConfig: vscode.WorkspaceConfiguration): Promise<ICheckResult[]> {
interface IToolCheckResults {
diagnosticCollection: vscode.DiagnosticCollection;
errors: ICheckResult[];
}

export function check(fileUri: vscode.Uri, goConfig: vscode.WorkspaceConfiguration): Promise<IToolCheckResults[]> {
diagnosticsStatusBarItem.hide();
outputChannel.clear();
let runningToolsPromises = [];
Expand Down Expand Up @@ -89,7 +95,9 @@ export function check(fileUri: vscode.Uri, goConfig: vscode.WorkspaceConfigurati
};

if (!!goConfig['buildOnSave'] && goConfig['buildOnSave'] !== 'off') {
runningToolsPromises.push(isModSupported(fileUri).then(isMod => goBuild(fileUri, isMod, goConfig, goConfig['buildOnSave'] === 'workspace')));
runningToolsPromises.push(isModSupported(fileUri)
.then(isMod => goBuild(fileUri, isMod, goConfig, goConfig['buildOnSave'] === 'workspace'))
.then(errors => ({ diagnosticCollection: buildDiagnosticCollection, errors })));
}

if (!!goConfig['testOnSave']) {
Expand All @@ -108,11 +116,13 @@ export function check(fileUri: vscode.Uri, goConfig: vscode.WorkspaceConfigurati
}

if (!!goConfig['lintOnSave'] && goConfig['lintOnSave'] !== 'off') {
runningToolsPromises.push(goLint(fileUri, goConfig, (goConfig['lintOnSave'])));
runningToolsPromises.push(goLint(fileUri, goConfig, goConfig['lintOnSave'])
.then(errors => ({diagnosticCollection: lintDiagnosticCollection, errors: errors})));
}

if (!!goConfig['vetOnSave'] && goConfig['vetOnSave'] !== 'off') {
runningToolsPromises.push(goVet(fileUri, goConfig, goConfig['vetOnSave'] === 'workspace'));
runningToolsPromises.push(goVet(fileUri, goConfig, goConfig['vetOnSave'] === 'workspace')
.then(errors => ({diagnosticCollection: vetDiagnosticCollection, errors: errors})));
}

if (!!goConfig['coverOnSave']) {
Expand All @@ -125,7 +135,5 @@ export function check(fileUri: vscode.Uri, goConfig: vscode.WorkspaceConfigurati
});
}

return Promise.all(runningToolsPromises).then(function (resultSets) {
return [].concat.apply([], resultSets);
});
return Promise.all(runningToolsPromises);
}
3 changes: 2 additions & 1 deletion src/goLint.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import vscode = require('vscode');
import { getToolsEnvVars, resolvePath, runTool, ICheckResult, handleDiagnosticErrors, getWorkspaceFolderPath, getToolsGopath } from './util';
import { outputChannel } from './goStatus';
import { diagnosticsStatusBarItem } from './goStatus';
import { lintDiagnosticCollection } from './goMain';
/**
* Runs linter on the current file, package or workspace.
*/
Expand All @@ -26,7 +27,7 @@ export function lintCode(scope?: string) {

goLint(documentUri, goConfig, scope)
.then(warnings => {
handleDiagnosticErrors(editor ? editor.document : null, warnings, vscode.DiagnosticSeverity.Warning);
handleDiagnosticErrors(editor ? editor.document : null, warnings, lintDiagnosticCollection);
diagnosticsStatusBarItem.hide();
})
.catch(err => {
Expand Down
7 changes: 4 additions & 3 deletions src/goLiveErrors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { getBinPath, getToolsEnvVars } from './util';
import cp = require('child_process');
import path = require('path');
import { promptForMissingTool } from './goInstallTools';
import { errorDiagnosticCollection } from './goMain';
import { buildDiagnosticCollection } from './goMain';

// Interface for settings configuration for adding and removing tags
interface GoLiveErrorsConfig {
Expand Down Expand Up @@ -69,7 +69,7 @@ function processFile(e: vscode.TextDocumentChangeEvent) {
return;
}

errorDiagnosticCollection.clear();
buildDiagnosticCollection.clear();

if (err) {
// we want to take the error path here because the command we are calling
Expand All @@ -86,6 +86,7 @@ function processFile(e: vscode.TextDocumentChangeEvent) {
file = vscode.Uri.file(file).toString();
let range = new vscode.Range(+line - 1, +column, +line - 1, +column);
let diagnostic = new vscode.Diagnostic(range, message, vscode.DiagnosticSeverity.Error);
diagnostic.source = 'go';

let diagnostics = diagnosticMap.get(file);
if (!diagnostics) {
Expand All @@ -96,7 +97,7 @@ function processFile(e: vscode.TextDocumentChangeEvent) {
});

diagnosticMap.forEach((diagnostics, file) => {
errorDiagnosticCollection.set(vscode.Uri.parse(file), diagnostics);
buildDiagnosticCollection.set(vscode.Uri.parse(file), diagnostics);
});
}
});
Expand Down
26 changes: 16 additions & 10 deletions src/goMain.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,9 @@ import { installCurrentPackage } from './goInstall';
import { updateWorkspaceModCache } from './goModules';
import { setGlobalState } from './stateUtils';

export let errorDiagnosticCollection: vscode.DiagnosticCollection;
export let warningDiagnosticCollection: vscode.DiagnosticCollection;
export let buildDiagnosticCollection: vscode.DiagnosticCollection;
export let lintDiagnosticCollection: vscode.DiagnosticCollection;
export let vetDiagnosticCollection: vscode.DiagnosticCollection;

export function activate(ctx: vscode.ExtensionContext): void {
let useLangServer = vscode.workspace.getConfiguration('go')['useLanguageServer'];
Expand Down Expand Up @@ -181,10 +182,12 @@ export function activate(ctx: vscode.ExtensionContext): void {
ctx.subscriptions.push(vscode.languages.registerCodeLensProvider(GO_MODE, referencesCodeLensProvider));
ctx.subscriptions.push(vscode.debug.registerDebugConfigurationProvider('go', new GoDebugConfigurationProvider()));

errorDiagnosticCollection = vscode.languages.createDiagnosticCollection('go-error');
ctx.subscriptions.push(errorDiagnosticCollection);
warningDiagnosticCollection = vscode.languages.createDiagnosticCollection('go-warning');
ctx.subscriptions.push(warningDiagnosticCollection);
buildDiagnosticCollection = vscode.languages.createDiagnosticCollection('go');
ctx.subscriptions.push(buildDiagnosticCollection);
lintDiagnosticCollection = vscode.languages.createDiagnosticCollection('go-lint');
ctx.subscriptions.push(lintDiagnosticCollection);
vetDiagnosticCollection = vscode.languages.createDiagnosticCollection('go-vet');
ctx.subscriptions.push(vetDiagnosticCollection);
vscode.workspace.onDidChangeTextDocument(removeCodeCoverageOnFileChange, null, ctx.subscriptions);
vscode.workspace.onDidChangeTextDocument(removeTestStatus, null, ctx.subscriptions);
vscode.window.onDidChangeActiveTextEditor(showHideStatus, null, ctx.subscriptions);
Expand Down Expand Up @@ -443,11 +446,14 @@ function runBuilds(document: vscode.TextDocument, goConfig: vscode.WorkspaceConf
return;
}

errorDiagnosticCollection.clear();
warningDiagnosticCollection.clear();
buildDiagnosticCollection.clear();
lintDiagnosticCollection.clear();
vetDiagnosticCollection.clear();
check(document.uri, goConfig)
.then((errors) => {
handleDiagnosticErrors(document, errors);
.then(results => {
results.forEach(result => {
handleDiagnosticErrors(document, result.errors, result.diagnosticCollection);
});
})
.catch(err => {
vscode.window.showInformationMessage('Error: ' + err);
Expand Down
3 changes: 2 additions & 1 deletion src/goVet.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import vscode = require('vscode');
import { getToolsEnvVars, runTool, ICheckResult, handleDiagnosticErrors, getWorkspaceFolderPath, getGoVersion, SemVersion } from './util';
import { outputChannel } from './goStatus';
import { diagnosticsStatusBarItem } from './goStatus';
import { vetDiagnosticCollection } from './goMain';

/**
* Runs go vet in the current package or workspace.
Expand All @@ -27,7 +28,7 @@ export function vetCode(vetWorkspace?: boolean) {

goVet(documentUri, goConfig, vetWorkspace)
.then(warnings => {
handleDiagnosticErrors(editor ? editor.document : null, warnings, vscode.DiagnosticSeverity.Warning);
handleDiagnosticErrors(editor ? editor.document : null, warnings, vetDiagnosticCollection);
diagnosticsStatusBarItem.hide();
})
.catch(err => {
Expand Down
62 changes: 25 additions & 37 deletions src/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ import TelemetryReporter from 'vscode-extension-telemetry';
import fs = require('fs');
import os = require('os');
import { outputChannel } from './goStatus';
import { errorDiagnosticCollection, warningDiagnosticCollection } from './goMain';
import { NearestNeighborDict, Node } from './avlTree';
import { getCurrentPackage } from './goModules';
import { buildDiagnosticCollection, lintDiagnosticCollection, vetDiagnosticCollection } from './goMain';

const extensionId: string = 'ms-vscode.Go';
const extensionVersion: string = vscode.extensions.getExtension(extensionId).packageJSON.version;
Expand Down Expand Up @@ -703,16 +703,11 @@ export function runTool(args: string[], cwd: string, severity: string, useStdErr
});
}

export function handleDiagnosticErrors(document: vscode.TextDocument, errors: ICheckResult[], diagnosticSeverity?: vscode.DiagnosticSeverity) {
export function handleDiagnosticErrors(document: vscode.TextDocument, errors: ICheckResult[], diagnosticCollection: vscode.DiagnosticCollection) {

if (diagnosticSeverity === undefined || diagnosticSeverity === vscode.DiagnosticSeverity.Error) {
errorDiagnosticCollection.clear();
}
if (diagnosticSeverity === undefined || diagnosticSeverity === vscode.DiagnosticSeverity.Warning) {
warningDiagnosticCollection.clear();
}
diagnosticCollection.clear();

let diagnosticMap: Map<string, Map<vscode.DiagnosticSeverity, vscode.Diagnostic[]>> = new Map();
let diagnosticMap: Map<string, vscode.Diagnostic[]> = new Map();
errors.forEach(error => {
let canonicalFile = vscode.Uri.file(error.file).toString();
let startColumn = 0;
Expand All @@ -731,46 +726,39 @@ export function handleDiagnosticErrors(document: vscode.TextDocument, errors: IC
let range = new vscode.Range(error.line - 1, startColumn, error.line - 1, endColumn);
let severity = mapSeverityToVSCodeSeverity(error.severity);
let diagnostic = new vscode.Diagnostic(range, error.msg, severity);
diagnostic.source = diagnosticCollection.name;
let diagnostics = diagnosticMap.get(canonicalFile);
if (!diagnostics) {
diagnostics = new Map<vscode.DiagnosticSeverity, vscode.Diagnostic[]>();
}
if (!diagnostics[severity]) {
diagnostics[severity] = [];
diagnostics = [];
}
diagnostics[severity].push(diagnostic);
diagnostics.push(diagnostic);
diagnosticMap.set(canonicalFile, diagnostics);
});

diagnosticMap.forEach((diagMap, file) => {
diagnosticMap.forEach((newDiagnostics, file) => {
const fileUri = vscode.Uri.parse(file);
if (diagnosticSeverity === undefined || diagnosticSeverity === vscode.DiagnosticSeverity.Error) {
const newErrors = diagMap[vscode.DiagnosticSeverity.Error];
let existingWarnings = warningDiagnosticCollection.get(fileUri);
errorDiagnosticCollection.set(fileUri, newErrors);

// If there are warnings on current file, remove the ones co-inciding with the new errors
if (newErrors && existingWarnings) {
const errorLines = newErrors.map(x => x.range.start.line);
existingWarnings = existingWarnings.filter(x => errorLines.indexOf(x.range.start.line) === -1);
warningDiagnosticCollection.set(fileUri, existingWarnings);
}
}
if (diagnosticSeverity === undefined || diagnosticSeverity === vscode.DiagnosticSeverity.Warning) {
const existingErrors = errorDiagnosticCollection.get(fileUri);
let newWarnings = diagMap[vscode.DiagnosticSeverity.Warning];

// If there are errors on current file, ignore the new warnings co-inciding with them
if (existingErrors && newWarnings) {
const errorLines = existingErrors.map(x => x.range.start.line);
newWarnings = newWarnings.filter(x => errorLines.indexOf(x.range.start.line) === -1);

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)) {
lintDiagnosticCollection.set(fileUri, deDupeDiagnostics(newDiagnostics, lintDiagnosticCollection.get(fileUri)));
}

warningDiagnosticCollection.set(fileUri, newWarnings);
if (vetDiagnosticCollection.has(fileUri)) {
vetDiagnosticCollection.set(fileUri, deDupeDiagnostics(newDiagnostics, vetDiagnosticCollection.get(fileUri)));
}
} else if (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), newDiagnostics);
}
diagnosticCollection.set(fileUri, newDiagnostics);
});
}

function deDupeDiagnostics(buildDiagnostics: vscode.Diagnostic[], otherDiagnostics: vscode.Diagnostic[]): vscode.Diagnostic[] {
const buildDiagnosticsLines = buildDiagnostics.map(x => x.range.start.line);
return otherDiagnostics.filter(x => buildDiagnosticsLines.indexOf(x.range.start.line) === -1);
}

function mapSeverityToVSCodeSeverity(sev: string): vscode.DiagnosticSeverity {
switch (sev) {
Expand Down Expand Up @@ -954,4 +942,4 @@ export function runGodoc(packagePath: string, symbol: string, token: vscode.Canc
}
});
});
}
}
18 changes: 12 additions & 6 deletions test/go.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,8 @@ It returns the number of bytes written and any write error encountered.
'vetFlags': { value: ['-all'] },
'lintOnSave': { value: 'package' },
'lintTool': { value: 'golint' },
'lintFlags': { value: [] }
'lintFlags': { value: [] },
'buildOnSave': { value: 'package' },
});
let expected = [
{ line: 7, severity: 'warning', msg: 'exported function Print2 should have comment or be unexported' },
Expand All @@ -289,7 +290,8 @@ It returns the number of bytes written and any write error encountered.
return Promise.resolve();
}
return check(vscode.Uri.file(path.join(fixturePath, 'errorsTest', 'errors.go')), config).then(diagnostics => {
let sortedDiagnostics = diagnostics.sort((a, b) => a.line - b.line);
const allDiagnostics = [].concat.apply([], diagnostics.map(x => x.errors));
let sortedDiagnostics = allDiagnostics.sort((a, b) => a.line - b.line);
assert.equal(sortedDiagnostics.length > 0, true, `Failed to get linter results`);
let matchCount = 0;
for (let i in expected) {
Expand Down Expand Up @@ -423,7 +425,8 @@ It returns the number of bytes written and any write error encountered.
];
let errorsTestPath = path.join(fixturePath, 'errorsTest', 'errors.go');
return check(vscode.Uri.file(errorsTestPath), config).then(diagnostics => {
let sortedDiagnostics = diagnostics.sort((a, b) => {
const allDiagnostics = [].concat.apply([], diagnostics.map(x => x.errors));
let sortedDiagnostics = allDiagnostics.sort((a, b) => {
if (a.msg < b.msg)
return -1;
if (a.msg > b.msg)
Expand Down Expand Up @@ -1156,7 +1159,8 @@ encountered.

const checkWithTags = check(vscode.Uri.file(path.join(fixturePath, 'buildTags', 'hello.go')), config1).then(diagnostics => {
assert.equal(1, diagnostics.length, 'check with buildtag failed. Unexpected errors found');
assert.equal(diagnostics[0].msg, 'undefined: fmt.Prinln');
assert.equal(1, diagnostics[0].errors.length, 'check with buildtag failed. Unexpected errors found');
assert.equal(diagnostics[0].errors[0].msg, 'undefined: fmt.Prinln');
});

const config2 = Object.create(vscode.workspace.getConfiguration('go'), {
Expand All @@ -1168,7 +1172,8 @@ encountered.

const checkWithMultipleTags = check(vscode.Uri.file(path.join(fixturePath, 'buildTags', 'hello.go')), config2).then(diagnostics => {
assert.equal(1, diagnostics.length, 'check with multiple buildtags failed. Unexpected errors found');
assert.equal(diagnostics[0].msg, 'undefined: fmt.Prinln');
assert.equal(1, diagnostics[0].errors.length, 'check with multiple buildtags failed. Unexpected errors found');
assert.equal(diagnostics[0].errors[0].msg, 'undefined: fmt.Prinln');
});

const config3 = Object.create(vscode.workspace.getConfiguration('go'), {
Expand All @@ -1180,7 +1185,8 @@ encountered.

const checkWithoutTags = check(vscode.Uri.file(path.join(fixturePath, 'buildTags', 'hello.go')), config3).then(diagnostics => {
assert.equal(1, diagnostics.length, 'check without buildtags failed. Unexpected errors found');
assert.equal(diagnostics[0].msg.indexOf('can\'t load package: package test/testfixture/buildTags') > -1, true, `check without buildtags failed. Go files not excluded. ${diagnostics[0].msg}`);
assert.equal(1, diagnostics[0].errors.length, 'check without buildtags failed. Unexpected errors found');
assert.equal(diagnostics[0].errors[0].msg.indexOf('can\'t load package: package test/testfixture/buildTags') > -1, true, `check without buildtags failed. Go files not excluded. ${diagnostics[0].errors[0].msg}`);
});

Promise.all([checkWithTags, checkWithMultipleTags, checkWithoutTags]).then(() => done(), done);
Expand Down

0 comments on commit 03dc46a

Please sign in to comment.