goInstallTools: don't lint with staticcheck when it's enabled in gopls

If a user has staticcheck enabled via gopls, we shouldn't require them to install the staticcheck binary separately. Also, we do not run the goLint function when the lintTool is staticcheck and staticcheck is enabled in gopls. Change-Id: I2b6c1260bdf1e5de0cb1d0009b25a752c434cd31 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/301053 Trust: Rebecca Stambler <[email protected]> Run-TryBot: Rebecca Stambler <[email protected]> TryBot-Result: kokoro <[email protected]> Reviewed-by: Hyang-Ah Hana Kim <[email protected]>
lzmoonlight · Mar 11, 2021 · 0058bd1 · 0058bd1
1 parent 8f2bcc1
commit 0058bd1
Show file tree

Hide file tree

Showing 6 changed files with 45 additions and 17 deletions.
diff --git a/src/goInstallTools.ts b/src/goInstallTools.ts
@@ -13,7 +13,7 @@ import fs = require('fs');
 import path = require('path');
 import semver = require('semver');
 import { ConfigurationTarget } from 'vscode';
-import { getGoConfig } from './config';
+import { getGoConfig, getGoplsConfig } from './config';
 import { toolExecutionEnvironment, toolInstallationEnvironment } from './goEnv';
 import { addGoRuntimeBaseToPATH, clearGoRuntimeBaseFromPATH } from './goEnvironmentStatus';
 import { logVerbose } from './goLogging';
@@ -51,7 +51,7 @@ const declinedInstalls: Tool[] = [];
 
 export async function installAllTools(updateExistingToolsOnly = false) {
 	const goVersion = await getGoVersion();
-	let allTools = getConfiguredTools(goVersion, getGoConfig());
+	let allTools = getConfiguredTools(goVersion, getGoConfig(), getGoplsConfig());
 
 	// exclude tools replaced by alternateTools.
 	const alternateTools: { [key: string]: string } = getGoConfig().get('alternateTools');
@@ -551,7 +551,7 @@ export async function offerToInstallTools() {
 }
 
 function getMissingTools(goVersion: GoVersion): Promise<Tool[]> {
-	const keys = getConfiguredTools(goVersion, getGoConfig());
+	const keys = getConfiguredTools(goVersion, getGoConfig(), getGoplsConfig());
 	return Promise.all<Tool>(
 		keys.map(
 			(tool) =>

diff --git a/src/goLint.ts b/src/goLint.ts
@@ -5,10 +5,11 @@
 
 import path = require('path');
 import vscode = require('vscode');
-import { getGoConfig } from './config';
+import { getGoConfig, getGoplsConfig } from './config';
 import { toolExecutionEnvironment } from './goEnv';
 import { lintDiagnosticCollection } from './goMain';
 import { diagnosticsStatusBarItem, outputChannel } from './goStatus';
+import { goplsStaticcheckEnabled } from './goTools';
 import { getWorkspaceFolderPath, handleDiagnosticErrors, ICheckResult, resolvePath, runTool } from './util';
 /**
  * Runs linter on the current file, package or workspace.
@@ -28,12 +29,13 @@ export function lintCode(scope?: string) {
 
 	const documentUri = editor ? editor.document.uri : null;
 	const goConfig = getGoConfig(documentUri);
+	const goplsConfig = getGoplsConfig(documentUri);
 
 	outputChannel.clear(); // Ensures stale output from lint on save is cleared
 	diagnosticsStatusBarItem.show();
 	diagnosticsStatusBarItem.text = 'Linting...';
 
-	goLint(documentUri, goConfig, scope)
+	goLint(documentUri, goConfig, goplsConfig, scope)
 		.then((warnings) => {
 			handleDiagnosticErrors(editor ? editor.document : null, warnings, lintDiagnosticCollection, 'go-lint');
 			diagnosticsStatusBarItem.hide();
@@ -54,8 +56,14 @@ export function lintCode(scope?: string) {
 export function goLint(
 	fileUri: vscode.Uri,
 	goConfig: vscode.WorkspaceConfiguration,
+	goplsConfig: vscode.WorkspaceConfiguration,
 	scope?: string
 ): Promise<ICheckResult[]> {
+	const lintTool = goConfig['lintTool'] || 'staticcheck';
+	if (lintTool === 'staticcheck' && goplsStaticcheckEnabled(goConfig, goplsConfig)) {
+		return;
+	}
+
 	epoch++;
 	const closureEpoch = epoch;
 	if (tokenSource) {
@@ -74,7 +82,6 @@ export function goLint(
 		return Promise.resolve([]);
 	}
 
-	const lintTool = goConfig['lintTool'] || 'staticcheck';
 	const lintFlags: string[] = goConfig['lintFlags'] || [];
 	const lintEnv = toolExecutionEnvironment();
 	const args: string[] = [];

diff --git a/src/goMain.ts b/src/goMain.ts
@@ -9,7 +9,7 @@
 'use strict';
 
 import * as path from 'path';
-import { getGoConfig, initConfig, IsInCloudIDE } from './config';
+import { getGoConfig, getGoplsConfig, initConfig, IsInCloudIDE } from './config';
 import { browsePackages } from './goBrowsePackage';
 import { buildCode } from './goBuild';
 import { check, notifyIfGeneratedFile, removeTestStatus } from './goCheck';
@@ -925,7 +925,7 @@ async function getConfiguredGoToolsCommand() {
 	outputChannel.appendLine('');
 
 	const goVersion = await getGoVersion();
-	const allTools = getConfiguredTools(goVersion, getGoConfig());
+	const allTools = getConfiguredTools(goVersion, getGoConfig(), getGoplsConfig());
 
 	allTools.forEach((tool) => {
 		const toolPath = getBinPath(tool.name);

diff --git a/src/goTools.ts b/src/goTools.ts
@@ -109,7 +109,11 @@ export function isGocode(tool: Tool): boolean {
 	return tool.name === 'gocode' || tool.name === 'gocode-gomod';
 }
 
-export function getConfiguredTools(goVersion: GoVersion, goConfig: { [key: string]: any }): Tool[] {
+export function getConfiguredTools(
+	goVersion: GoVersion,
+	goConfig: { [key: string]: any },
+	goplsConfig: { [key: string]: any }
+): Tool[] {
 	// If language server is enabled, don't suggest tools that are replaced by gopls.
 	// TODO(github.com/golang/vscode-go/issues/388): decide what to do when
 	// the go version is no longer supported by gopls while the legacy tools are
@@ -172,8 +176,12 @@ export function getConfiguredTools(goVersion: GoVersion, goConfig: { [key: strin
 		maybeAddTool(getFormatTool(goConfig));
 	}
 
-	// Add the linter that was chosen by the user.
-	maybeAddTool(goConfig['lintTool']);
+	// Add the linter that was chosen by the user, but don't add staticcheck
+	// if it is enabled via gopls.
+	const goplsStaticheckEnabled = useLanguageServer && goplsStaticcheckEnabled(goConfig, goplsConfig);
+	if (goConfig['lintTool'] !== 'staticcheck' || !goplsStaticheckEnabled) {
+		maybeAddTool(goConfig['lintTool']);
+	}
 
 	// Add the language server if the user has chosen to do so.
 	// Even though we arranged this to run after the first attempt to start gopls
@@ -189,6 +197,17 @@ export function getConfiguredTools(goVersion: GoVersion, goConfig: { [key: strin
 	return tools;
 }
 
+export function goplsStaticcheckEnabled(
+	goConfig: { [key: string]: any },
+	goplsConfig: { [key: string]: any }
+): boolean {
+	if (goConfig['useLanguageServer'] !== true || goplsConfig['ui.diagnostic.staticcheck'] !== true) {
+		return false;
+	}
+	const features = goConfig['languageServerExperimentalFeatures'];
+	return !features || features['diagnostics'] === true;
+}
+
 export const allToolsInformation: { [key: string]: Tool } = {
 	'gocode': {
 		name: 'gocode',

diff --git a/test/integration/extension.test.ts b/test/integration/extension.test.ts
@@ -12,7 +12,7 @@ import * as fs from 'fs-extra';
 import * as path from 'path';
 import * as sinon from 'sinon';
 import * as vscode from 'vscode';
-import { getGoConfig } from '../../src/config';
+import { getGoConfig, getGoplsConfig } from '../../src/config';
 import { FilePatch, getEdits, getEditsFromUnifiedDiffStr } from '../../src/diffUtils';
 import { check } from '../../src/goCheck';
 import { GoDefinitionProvider } from '../../src/goDeclaration';
@@ -399,10 +399,11 @@ It returns the number of bytes written and any write error encountered.
 			lintTool: { value: process.platform !== 'win32' ? 'sleep' : 'timeout' },
 			lintFlags: { value: process.platform !== 'win32' ? ['2'] : ['/t', '2'] }
 		});
+		const goplsConfig = Object.create(getGoplsConfig(), {});
 
 		const results = await Promise.all([
-			goLint(vscode.Uri.file(path.join(fixturePath, 'linterTest', 'linter_1.go')), config),
-			goLint(vscode.Uri.file(path.join(fixturePath, 'linterTest', 'linter_2.go')), config)
+			goLint(vscode.Uri.file(path.join(fixturePath, 'linterTest', 'linter_1.go')), config, goplsConfig),
+			goLint(vscode.Uri.file(path.join(fixturePath, 'linterTest', 'linter_2.go')), config, goplsConfig)
 		]);
 		assert.equal(util.runTool.callCount, 2, 'should have launched 2 lint jobs');
 		assert.equal(
@@ -431,6 +432,7 @@ It returns the number of bytes written and any write error encountered.
 				// but this test depends on ST1003 (MixedCaps package name) presented in both files
 				// in the same package. So, enable that.
 			}),
+			Object.create(getGoplsConfig(), {}),
 			'package'
 		);
 

diff --git a/test/integration/install.test.ts b/test/integration/install.test.ts
@@ -209,21 +209,21 @@ function shouldRunSlowTests(): boolean {
 
 suite('getConfiguredTools', () => {
 	test('do not require legacy tools when using language server', async () => {
-		const configured = getConfiguredTools(fakeGoVersion('1.15.6'), { useLanguageServer: true });
+		const configured = getConfiguredTools(fakeGoVersion('1.15.6'), { useLanguageServer: true }, {});
 		const got = configured.map((tool) => tool.name) ?? [];
 		assert(got.includes('gopls'), `omitted 'gopls': ${JSON.stringify(got)}`);
 		assert(!got.includes('guru') && !got.includes('gocode'), `suggested legacy tools: ${JSON.stringify(got)}`);
 	});
 
 	test('do not require gopls when not using language server', async () => {
-		const configured = getConfiguredTools(fakeGoVersion('1.15.6'), { useLanguageServer: false });
+		const configured = getConfiguredTools(fakeGoVersion('1.15.6'), { useLanguageServer: false }, {});
 		const got = configured.map((tool) => tool.name) ?? [];
 		assert(!got.includes('gopls'), `suggested 'gopls': ${JSON.stringify(got)}`);
 		assert(got.includes('guru') && got.includes('gocode'), `omitted legacy tools: ${JSON.stringify(got)}`);
 	});
 
 	test('do not require gopls when the go version is old', async () => {
-		const configured = getConfiguredTools(fakeGoVersion('1.9'), { useLanguageServer: true });
+		const configured = getConfiguredTools(fakeGoVersion('1.9'), { useLanguageServer: true }, {});
 		const got = configured.map((tool) => tool.name) ?? [];
 		assert(!got.includes('gopls'), `suggested 'gopls' for old go: ${JSON.stringify(got)}`);
 		assert(got.includes('guru') && got.includes('gocode'), `omitted legacy tools: ${JSON.stringify(got)}`);