extension/src: remove default setting for "go.lintTool"

The "lintTool" will be the source of truth indicating which tool
will be run as linter. vscode-go will now execute the exactly
tool based on this setting without any hidden yield logic.

Previously, the default value for "go.lintTool" is "staticcheck",
vscode-go have special logic that yield the linter functionality
to gopls if the lint tool is staticcheck and gopls staticheck is
enabled. It confuses the user which tool will be executed.

This CL removes this default option from "go.lintTool", consider
the new default as "". An empty lintTool indicates the user does
not want any linter to run from the extension.

This CL also change the description of "go.lintTool" to the lint
tool in addition to gopls, also mention in the description that
the linting functionality provided by gopls (the language server)
can be configured in a seprate setting.

To eliminate further confusions, a validation logic will be
triggered when gopls or go configuration changed: if "staticcheck"
is chosen as lintTool and gopls staticcheck is enabled. Warning
will be sent to user to disable one of them to avoid duplicate
diagnostics.

Alternative considered: introduce "gopls" as default lintTool.

For #3862

Change-Id: Ia00ca115ca7ec34315043ad61517b5cdfb9ca1ed
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/709035
Reviewed-by: Madeline Kalil <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: h9jiang

docs/settings.md

@@ -379,10 +379,15 @@ Allowed Options:
 Default: `"package"`
 ### `go.lintTool`
 
-Specifies Lint tool name.<br/>
-Allowed Options: `staticcheck`, `golint`, `golangci-lint`, `golangci-lint-v2`, `revive`
+Specifies an additional client-side linting tool that should be run by the Go extension. By default (unset), no additional linter is run. This feature is additional to diagnostics reported by the language server, gopls. Since Gopls incorporates the entire staticcheck analyzer suite, it is typically unnecessary to run the staticcheck tool as well. To configure gopls's linting, see the 'gopls.ui.diagnostic' settings.<br/>
+Allowed Options:
+
+* `staticcheck`: Run `staticcheck`.
+* `golint`: Run `golint`.
+* `golangci-lint`: Run `golangci-lint` v1.
+* `golangci-lint-v2`: Run `golangci-lint` v2.
+* `revive`: Run `revive`.
 
-Default: `"staticcheck"`
 ### `go.logging.level (deprecated)`
 
 This setting is deprecated. Use 'Developer: Set Log Level...' command to control logging level instead.

extension/package.json

@@ -1182,15 +1182,21 @@
         },
         "go.lintTool": {
           "type": "string",
-          "default": "staticcheck",
-          "description": "Specifies Lint tool name.",
+          "description": "Specifies an additional client-side linting tool that should be run by the Go extension. By default (unset), no additional linter is run. This feature is additional to diagnostics reported by the language server, gopls. Since Gopls incorporates the entire staticcheck analyzer suite, it is typically unnecessary to run the staticcheck tool as well. To configure gopls's linting, see the 'gopls.ui.diagnostic' settings.",
           "scope": "resource",
           "enum": [
             "staticcheck",
             "golint",
             "golangci-lint",
             "golangci-lint-v2",
             "revive"
+          ],
+          "markdownEnumDescriptions": [
+            "Run `staticcheck`.",
+            "Run `golint`.",
+            "Run `golangci-lint` v1.",
+            "Run `golangci-lint` v2.",
+            "Run `revive`."
           ]
         },
         "go.lintFlags": {

extension/src/config.ts

@@ -60,3 +60,64 @@ export class ExtensionInfo {
 }
 
 export const extensionInfo = new ExtensionInfo();
+
+/**
+ * Performs cross-validation between the 'go' and 'gopls' configurations.
+ *
+ * This function's purpose is to detect conflicts where a setting in 'go'
+ * impacts the behavior of the gopls. It should be called when relevant
+ * settings in either configuration change.
+ *
+ * Note: This does not validate gopls settings internally, as the gopls
+ * server is responsible for that itself.
+ */
+export async function validateConfig(
+	goConfig: vscode.WorkspaceConfiguration,
+	goplsConfig: vscode.WorkspaceConfiguration
+) {
+	// Lint tool check.
+	const lintTool = goConfig.get<string>('lintTool');
+	if (lintTool && lintTool === 'staticcheck' && goplsStaticcheckEnabled(goConfig, goplsConfig)) {
+		const message = `Warning: staticcheck is configured to run both client side (go.lintTool=staticcheck) and server side (gopls.ui.diagnostic.staticcheck=true).
+This will result in duplicate diagnostics.`;
+
+		const selected = await vscode.window.showWarningMessage(message, 'Open Settings');
+		if (selected === 'Open Settings') {
+			vscode.commands.executeCommand('workbench.action.openSettingsJson');
+		}
+	}
+}
+
+/**
+ * Checks if the staticcheck analyzer is enabled in the gopls configuration.
+ *
+ * @param goConfig The 'go' extension configuration.
+ * @param goplsConfig The 'gopls' configuration.
+ * @returns true if staticcheck is enabled in gopls, false otherwise.
+ */
+export function goplsStaticcheckEnabled(
+	goConfig: vscode.WorkspaceConfiguration,
+	goplsConfig: vscode.WorkspaceConfiguration
+): boolean {
+	if (!goConfig.get<boolean>('useLanguageServer')) {
+		return false;
+	}
+
+	/**
+	 * Direct property access is used here instead of the standard
+	 * `goplsConfig.get()` method.
+	 *
+	 * The `.get('a.b')` API interprets dots as a path to a nested property.
+	 * However, 'gopls' configuration object has "flattened" keys that literally
+	 * contain dots (e.g., the key is the string 'ui.diagnostic.staticcheck').
+	 * Bracket notation is the only way to access such a property.
+	 */
+	if (goplsConfig['ui.diagnostic.staticcheck'] === false) {
+		return false;
+	}
+
+	// "ui.diagnostic.staticcheck" unset or true means partially enabled and
+	// fully enabled.
+	// See https://go.googlesource.com/tools/+/refs/heads/master/gopls/doc/analyzers.md
+	return true;
+}

extension/src/goCheck.ts

@@ -9,7 +9,6 @@
 
 import path = require('path');
 import vscode = require('vscode');
-import { getGoplsConfig } from './config';
 import { goBuild } from './goBuild';
 import { goLint } from './goLint';
 import { isModSupported } from './goModules';
@@ -120,9 +119,8 @@ export function check(
 	}
 
 	if (lintDiagnosticCollection && !!goConfig['lintOnSave'] && goConfig['lintOnSave'] !== 'off') {
-		const goplsConfig = getGoplsConfig(fileUri);
 		runningToolsPromises.push(
-			goLint(fileUri, goConfig, goplsConfig, goConfig['lintOnSave']).then((errors) => ({
+			goLint(fileUri, goConfig, goConfig['lintOnSave']).then((errors) => ({
 				diagnosticCollection: lintDiagnosticCollection,
 				errors
 			}))

extension/src/goLint.ts

@@ -6,12 +6,10 @@
 import path = require('path');
 import vscode = require('vscode');
 import { CommandFactory } from './commands';
-import { getGoConfig, getGoplsConfig } from './config';
+import { getGoConfig } from './config';
 import { toolExecutionEnvironment } from './goEnv';
 import { diagnosticsStatusBarItem, outputChannel } from './goStatus';
-import { goplsStaticcheckEnabled } from './goTools';
-import { inspectGoToolVersion } from './goInstallTools';
-import { getBinPath, getWorkspaceFolderPath, handleDiagnosticErrors, ICheckResult, resolvePath, runTool } from './util';
+import { getWorkspaceFolderPath, handleDiagnosticErrors, ICheckResult, resolvePath, runTool } from './util';
 
 /**
  * Runs linter on the current file, package or workspace.
@@ -34,13 +32,12 @@ export function lintCode(scope?: string): CommandFactory {
 
 		const documentUri = editor ? editor.document.uri : undefined;
 		const goConfig = getGoConfig(documentUri);
-		const goplsConfig = getGoplsConfig(documentUri);
 
 		outputChannel.info('Linting...');
 		diagnosticsStatusBarItem.show();
 		diagnosticsStatusBarItem.text = 'Linting...';
 
-		goLint(documentUri, goConfig, goplsConfig, scope)
+		goLint(documentUri, goConfig, scope)
 			.then((warnings) => {
 				handleDiagnosticErrors(
 					goCtx,
@@ -58,7 +55,8 @@ export function lintCode(scope?: string): CommandFactory {
 }
 
 /**
- * Runs linter and presents the output in the 'Go' channel and in the diagnostic collections.
+ * Runs linter and presents the output in the 'Go' channel and in the diagnostic
+ * collections.
  *
  * @param fileUri Document uri.
  * @param goConfig Configuration for the Go extension.
@@ -67,11 +65,11 @@ export function lintCode(scope?: string): CommandFactory {
 export async function goLint(
 	fileUri: vscode.Uri | undefined,
 	goConfig: vscode.WorkspaceConfiguration,
-	goplsConfig: vscode.WorkspaceConfiguration,
 	scope?: string
 ): Promise<ICheckResult[]> {
-	const lintTool = goConfig['lintTool'] || 'staticcheck';
-	if (lintTool === 'staticcheck' && goplsStaticcheckEnabled(goConfig, goplsConfig)) {
+	const linter = goConfig.get<string>('lintTool');
+	if (linter === undefined || linter === '') {
+		// Yield the linter functionality to gopls by returning empty.
 		return Promise.resolve([]);
 	}
 
@@ -93,7 +91,7 @@ export async function goLint(
 		return Promise.resolve([]);
 	}
 
-	const lintFlags: string[] = goConfig['lintFlags'] || [];
+	const lintFlags = goConfig.get<string[]>('lintFlags') === undefined ? [] : goConfig.get<string[]>('lintFlags')!;
 	const lintEnv = toolExecutionEnvironment();
 	const args: string[] = [];
 
@@ -113,9 +111,9 @@ export async function goLint(
 		}
 		args.push(flag);
 	});
-	if (lintTool.startsWith('golangci-lint')) {
+	if (linter.startsWith('golangci-lint')) {
 		let version = 1;
-		if (lintTool === 'golangci-lint-v2') {
+		if (linter === 'golangci-lint-v2') {
 			version = 2;
 		}
 
@@ -174,7 +172,7 @@ export async function goLint(
 	}
 
 	running = true;
-	const lintPromise = runTool(args, cwd, 'warning', false, lintTool, lintEnv, false, tokenSource.token).then(
+	const lintPromise = runTool(args, cwd, 'warning', false, linter, lintEnv, false, tokenSource.token).then(
 		(result) => {
 			if (closureEpoch === epoch) {
 				running = false;

extension/src/goMain.ts

@@ -8,7 +8,6 @@
 
 'use strict';
 
-import { extensionInfo, getGoConfig } from './config';
 import { browsePackages } from './goBrowsePackage';
 import { buildCode } from './goBuild';
 import { notifyIfGeneratedFile, removeTestStatus } from './goCheck';
@@ -39,7 +38,7 @@ import {
 	maybeInstallVSCGO,
 	maybeInstallImportantTools
 } from './goInstallTools';
-import { RestartReason, showServerOutputChannel, watchLanguageServerConfiguration } from './language/goLanguageServer';
+import { RestartReason, showServerOutputChannel, promptAboutGoplsOptOut } from './language/goLanguageServer';
 import { lintCode } from './goLint';
 import { GO_MODE } from './goMode';
 import { GO111MODULE, goModInit } from './goModules';
@@ -58,10 +57,8 @@ import {
 } from './stateUtils';
 import { cancelRunningTests, showTestOutput } from './testUtils';
 import { cleanupTempDir, getBinPath, getToolsGopath } from './util';
-import { clearCacheForTools } from './utils/pathUtils';
 import { WelcomePanel } from './welcome';
 import vscode = require('vscode');
-import { getFormatTool } from './language/legacy/goFormat';
 import { resetSurveyStates, showSurveyStates } from './goSurvey';
 import { ExtensionAPI } from './export';
 import extensionAPI from './extensionAPI';
@@ -75,6 +72,9 @@ import { toggleVulncheckCommandFactory } from './goVulncheck';
 import { GoTaskProvider } from './goTaskProvider';
 import { setTelemetryEnvVars, activationLatency, telemetryReporter } from './goTelemetry';
 import { experiments } from './experimental';
+import { extensionInfo, getGoConfig, getGoplsConfig, validateConfig } from './config';
+import { clearCacheForTools } from './utils/pathUtils';
+import { getFormatTool } from './language/legacy/goFormat';
 
 const goCtx: GoExtensionContext = {};
 
@@ -222,7 +222,7 @@ export async function activate(ctx: vscode.ExtensionContext): Promise<ExtensionA
 	registerCommand('go.survey.showConfig', showSurveyStates);
 	registerCommand('go.survey.resetConfig', resetSurveyStates);
 
-	addOnDidChangeConfigListeners(ctx);
+	addConfigChangeListener(ctx);
 	addOnChangeTextDocumentListeners(ctx);
 	addOnChangeActiveTextEditorListeners(ctx);
 	addOnSaveTextDocumentListeners(ctx);
@@ -251,21 +251,48 @@ export function deactivate() {
 	]);
 }
 
-function addOnDidChangeConfigListeners(ctx: vscode.ExtensionContext) {
+export function addConfigChangeListener(ctx: vscode.ExtensionContext) {
 	// Subscribe to notifications for changes to the configuration
 	// of the language server, even if it's not currently in use.
 	ctx.subscriptions.push(
-		vscode.workspace.onDidChangeConfiguration((e) => watchLanguageServerConfiguration(goCtx, e))
+		vscode.workspace.onDidChangeConfiguration((e) => {
+			const goConfig = getGoConfig();
+			const goplsConfig = getGoplsConfig();
+
+			validateConfig(goConfig, goplsConfig);
+
+			if (!e.affectsConfiguration('go')) {
+				return;
+			}
+
+			if (
+				e.affectsConfiguration('go.useLanguageServer') ||
+				e.affectsConfiguration('go.languageServerFlags') ||
+				e.affectsConfiguration('go.alternateTools') ||
+				e.affectsConfiguration('go.toolsEnvVars') ||
+				e.affectsConfiguration('go.formatTool')
+				// TODO: Should we check http.proxy too? That affects toolExecutionEnvironment too.
+			) {
+				vscode.commands.executeCommand('go.languageserver.restart', RestartReason.CONFIG_CHANGE);
+			}
+
+			if (e.affectsConfiguration('go.useLanguageServer') && goConfig['useLanguageServer'] === false) {
+				promptAboutGoplsOptOut(goCtx);
+			}
+		})
 	);
 	ctx.subscriptions.push(
 		vscode.workspace.onDidChangeConfiguration(async (e: vscode.ConfigurationChangeEvent) => {
 			if (!e.affectsConfiguration('go')) {
 				return;
 			}
-			const updatedGoConfig = getGoConfig();
+			const goConfig = getGoConfig();
+			const goplsConfig = getGoplsConfig();
+
+			validateConfig(goConfig, goplsConfig);
 
 			if (e.affectsConfiguration('go.goroot')) {
-				const configGOROOT = updatedGoConfig['goroot'];
+				const configGOROOT = goConfig['goroot'];
 				if (configGOROOT) {
 					await setGOROOTEnvVar(configGOROOT);
 				}
@@ -285,25 +312,19 @@ function addOnDidChangeConfigListeners(ctx: vscode.ExtensionContext) {
 			}
 
 			if (e.affectsConfiguration('go.formatTool')) {
-				const tool = getFormatTool(updatedGoConfig);
+				const tool = getFormatTool(goConfig);
 				// When language server gopls is active (by default), existence
 				// checks are skipped only for tools that gopls replaces. Other
 				// tools are still checked.
-				if (
-					updatedGoConfig['useLanguageServer'] === false ||
-					!new Set(['gofmt', 'goimports', 'goformat']).has(tool)
-				) {
+				if (goConfig['useLanguageServer'] === false || !new Set(['gofmt', 'goimports', 'goformat']).has(tool)) {
 					checkToolExists(tool);
 				}
 			}
-			if (e.affectsConfiguration('go.lintTool')) {
-				checkToolExists(updatedGoConfig['lintTool']);
-			}
 			if (e.affectsConfiguration('go.docsTool')) {
-				checkToolExists(updatedGoConfig['docsTool']);
+				checkToolExists(goConfig['docsTool']);
 			}
 			if (e.affectsConfiguration('go.coverageDecorator')) {
-				updateCodeCoverageDecorators(updatedGoConfig['coverageDecorator']);
+				updateCodeCoverageDecorators(goConfig['coverageDecorator']);
 			}
 			if (e.affectsConfiguration('go.toolsEnvVars')) {
 				const env = toolExecutionEnvironment();
@@ -318,7 +339,9 @@ function addOnDidChangeConfigListeners(ctx: vscode.ExtensionContext) {
 				}
 			}
 			if (e.affectsConfiguration('go.lintTool')) {
-				const lintTool = lintDiagnosticCollectionName(updatedGoConfig['lintTool']);
+				checkToolExists(goConfig['lintTool']);
+
+				const lintTool = lintDiagnosticCollectionName(goConfig['lintTool']);
 				if (goCtx.lintDiagnosticCollection && goCtx.lintDiagnosticCollection.name !== lintTool) {
 					goCtx.lintDiagnosticCollection.dispose();
 					goCtx.lintDiagnosticCollection = vscode.languages.createDiagnosticCollection(lintTool);
@@ -381,6 +404,9 @@ function addOnChangeActiveTextEditorListeners(ctx: vscode.ExtensionContext) {
 }
 
 function checkToolExists(tool: string) {
+	if (tool === '') {
+		return;
+	}
 	if (tool === getBinPath(tool)) {
 		promptForMissingTool(tool);
 	}

extension/src/goTools.ts

@@ -150,26 +150,9 @@ export function getConfiguredTools(goConfig: { [key: string]: any }, goplsConfig
 		maybeAddTool(getFormatTool(goConfig));
 	}
 
-	// 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']);
-	}
+	maybeAddTool(goConfig['lintTool']);
 
-	// Remove duplicates since tools like golangci-lint v2 are both linter and formatter
+	// Remove duplicates since tools like golangci-lint v2 and gopls are both
+	// capable of linting and formatting.
 	return tools.filter((v, i, self) => self.indexOf(v) === i);
 }
-
-export function goplsStaticcheckEnabled(
-	goConfig: { [key: string]: any },
-	goplsConfig: { [key: string]: any }
-): boolean {
-	if (
-		goplsConfig['ui.diagnostic.staticcheck'] === false ||
-		(goplsConfig['ui.diagnostic.staticcheck'] === undefined && goplsConfig['staticcheck'] !== true)
-	) {
-		return false;
-	}
-	return true;
-}