From 03dc46aba947e901df1008666deb27b60047c404 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Tue, 13 Nov 2018 03:49:16 +0200 Subject: [PATCH] Split up the DiagnosticCollections per tool type (#2109) * 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 --- src/goBuild.ts | 5 ++-- src/goCheck.ts | 22 +++++++++++----- src/goLint.ts | 3 ++- src/goLiveErrors.ts | 7 ++--- src/goMain.ts | 26 +++++++++++-------- src/goVet.ts | 3 ++- src/util.ts | 62 ++++++++++++++++++--------------------------- test/go.test.ts | 18 ++++++++----- 8 files changed, 79 insertions(+), 67 deletions(-) diff --git a/src/goBuild.ts b/src/goBuild.ts index b09a3cdf5..524008fd8 100644 --- a/src/goBuild.ts +++ b/src/goBuild.ts @@ -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. */ @@ -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 => { @@ -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; \ No newline at end of file +let running = false; diff --git a/src/goCheck.ts b/src/goCheck.ts index 102a31e30..533b3ed7e 100644 --- a/src/goCheck.ts +++ b/src/goCheck.ts @@ -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'; @@ -46,7 +47,12 @@ export function notifyIfGeneratedFile(e: vscode.TextDocumentChangeEvent) { } } -export function check(fileUri: vscode.Uri, goConfig: vscode.WorkspaceConfiguration): Promise { +interface IToolCheckResults { + diagnosticCollection: vscode.DiagnosticCollection; + errors: ICheckResult[]; +} + +export function check(fileUri: vscode.Uri, goConfig: vscode.WorkspaceConfiguration): Promise { diagnosticsStatusBarItem.hide(); outputChannel.clear(); let runningToolsPromises = []; @@ -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']) { @@ -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']) { @@ -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); } diff --git a/src/goLint.ts b/src/goLint.ts index 39148c49c..d293556e8 100644 --- a/src/goLint.ts +++ b/src/goLint.ts @@ -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. */ @@ -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 => { diff --git a/src/goLiveErrors.ts b/src/goLiveErrors.ts index 67e505d28..d3bf00778 100644 --- a/src/goLiveErrors.ts +++ b/src/goLiveErrors.ts @@ -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 { @@ -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 @@ -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) { @@ -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); }); } }); diff --git a/src/goMain.ts b/src/goMain.ts index 3c999bae9..9218a0a8f 100644 --- a/src/goMain.ts +++ b/src/goMain.ts @@ -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']; @@ -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); @@ -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); diff --git a/src/goVet.ts b/src/goVet.ts index 87dadee05..e633862c4 100644 --- a/src/goVet.ts +++ b/src/goVet.ts @@ -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. @@ -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 => { diff --git a/src/util.ts b/src/util.ts index c67eaa6da..6d0171cfd 100644 --- a/src/util.ts +++ b/src/util.ts @@ -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; @@ -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> = new Map(); + let diagnosticMap: Map = new Map(); errors.forEach(error => { let canonicalFile = vscode.Uri.file(error.file).toString(); let startColumn = 0; @@ -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(); - } - 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) { @@ -954,4 +942,4 @@ export function runGodoc(packagePath: string, symbol: string, token: vscode.Canc } }); }); -} \ No newline at end of file +} diff --git a/test/go.test.ts b/test/go.test.ts index 6f6abebd9..350a17f9c 100644 --- a/test/go.test.ts +++ b/test/go.test.ts @@ -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' }, @@ -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) { @@ -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) @@ -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'), { @@ -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'), { @@ -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);