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

Split up the DiagnosticCollections per tool type #2109

Merged
merged 6 commits into from
Nov 13, 2018
Merged

Split up the DiagnosticCollections per tool type #2109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3989532
Split up the DiagnosticCollections per tool type
segevfiner Nov 9, 2018
13f031e
Simplify handleDiagnosticErrors a bit
segevfiner Nov 9, 2018
b552cdc
Also dedupe build & lint warnings together
segevfiner Nov 9, 2018
7099d2f
Clear all diagnostics in runBuilds
segevfiner Nov 9, 2018
90b6e84
Small refactorings
ramya-rao-a Nov 12, 2018
2072fd7
Revert test.only
ramya-rao-a Nov 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/goBuild.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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 @@ -437,11 +440,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
Open in desktop
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
Open in desktop
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[] {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is really more generic than just for the buildDiagnostics. Might be a bit hard to come up with good "generic" names for the parameters though 😛

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
Open in desktop
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 @@ -1154,7 +1157,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 @@ -1166,7 +1170,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 @@ -1178,7 +1183,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