extension/src: refactor required tool function

Validate the behavior of third party tool installation. When gopls
is enabled, third party tools that is replaced by gopls will not
be installed by default. (e.g. "gotests", "gomodifytags") But will
be installed if user explicitly triggers the legacy commands.

For #1652

Change-Id: I8754f013f252b221a981f6d7ebc5d71aa2440d14
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/713680
Reviewed-by: Peter Weinberger <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: h9jiang

extension/src/commands/getConfiguredGoTools.ts

@@ -9,7 +9,7 @@ import path from 'path';
 import { CommandFactory } from '.';
 import { getGoConfig } from '../config';
 import { inspectGoToolVersion } from '../goInstallTools';
-import { getConfiguredTools } from '../goTools';
+import { getRequiredTools } from '../goTools';
 import { getBinPath, getCurrentGoPath, getGoEnv, getGoVersion, getToolsGopath } from '../util';
 import { getEnvPath, initialEnvPath, getCurrentGoRoot } from '../utils/pathUtils';
 
@@ -40,7 +40,7 @@ export const getConfiguredGoTools: CommandFactory = () => {
 		buf.push('\n## Tools\n');
 		try {
 			const goVersion = await getGoVersion();
-			const allTools = getConfiguredTools(getGoConfig());
+			const allTools = getRequiredTools(getGoConfig());
 			const goVersionTooOld = goVersion?.lt('1.18') || false;
 
 			buf.push(`\tgo:\t${goVersion?.binaryPath}: ${goVersion?.version}`);

extension/src/goExplorer.ts

@@ -8,7 +8,7 @@ import os = require('os');
 import path = require('path');
 import { getGoConfig } from './config';
 import { getBinPath } from './util';
-import { getConfiguredTools } from './goTools';
+import { getRequiredTools } from './goTools';
 import { inspectGoToolVersion } from './goInstallTools';
 import { runGoEnv } from './goModules';
 
@@ -159,7 +159,7 @@ export class GoExplorerProvider implements vscode.TreeDataProvider<vscode.TreeIt
 	}
 
 	private async toolTreeItems() {
-		const allTools = getConfiguredTools(getGoConfig());
+		const allTools = getRequiredTools(getGoConfig());
 		const toolsInfo = await Promise.all(allTools.map((tool) => this.toolDetailCache.get(tool.name)));
 		const items = [];
 		for (const t of toolsInfo) {

extension/src/goInstallTools.ts

@@ -18,7 +18,7 @@ import { toolExecutionEnvironment, toolInstallationEnvironment } from './goEnv';
 import { addGoRuntimeBaseToPATH, clearGoRuntimeBaseFromPATH } from './goEnvironmentStatus';
 import { GoExtensionContext } from './context';
 import { addGoStatus, initGoStatusBar, outputChannel, removeGoStatus } from './goStatus';
-import { containsTool, getConfiguredTools, getImportPathWithVersion, getTool, Tool, ToolAtVersion } from './goTools';
+import { containsTool, getRequiredTools, getImportPathWithVersion, getTool, Tool, ToolAtVersion } from './goTools';
 import {
 	getBinPath,
 	getBinPathWithExplanation,
@@ -71,7 +71,7 @@ export const defaultToolsManager: IToolsManager = {
 
 export async function installAllTools(updateExistingToolsOnly = false) {
 	const goVersion = await getGoVersion();
-	let allTools = getConfiguredTools(getGoConfig());
+	let allTools = getRequiredTools(getGoConfig());
 
 	// exclude tools replaced by alternateTools.
 	const alternateTools: { [key: string]: string } = getGoConfig().get('alternateTools') ?? {};
@@ -701,7 +701,7 @@ async function updateImportantToolsStatus(tm: IToolsManager = defaultToolsManage
  * If matcher is provided, only the tools that match the filter will be checked.
  */
 function getMissingTools(matcher?: (value: Tool) => boolean): Promise<Tool[]> {
-	let keys = getConfiguredTools(getGoConfig());
+	let keys = getRequiredTools(getGoConfig());
 	if (matcher) {
 		keys = keys.filter(matcher);
 	}
@@ -883,7 +883,7 @@ export async function suggestUpdates() {
 		return;
 	}
 
-	const allTools = getConfiguredTools(getGoConfig());
+	const allTools = getRequiredTools(getGoConfig());
 	const toolsToUpdate = await listOutdatedTools(configuredGoVersion, allTools);
 	if (toolsToUpdate.length === 0) {
 		return;

extension/src/goTools.ts

@@ -111,45 +111,53 @@ export function hasModSuffix(tool: Tool): boolean {
 	return tool.name.endsWith('-gomod');
 }
 
-export function getConfiguredTools(goConfig: vscode.WorkspaceConfiguration): Tool[] {
+/**
+ * getRequiredTools returns the tools that required explicitly by the user
+ * through settings like "go.lintTool", "go.formatTool" and implicitly by the
+ * extension's functionality like "goplay".
+ *
+ * Tools that is replaced by gopls will not be returned if gopls is enabled.
+ */
+export function getRequiredTools(goConfig: vscode.WorkspaceConfiguration): 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
 	// no longer working (or we remove the legacy language feature providers completely).
-	const useLanguageServer = goConfig['useLanguageServer'];
+	const useLanguageServer = goConfig.get<boolean>('useLanguageServer');
 
-	const tools: Tool[] = [];
-	function maybeAddTool(name: string) {
-		const tool = allToolsInformation.get(name);
-		if (tool) {
-			if (!useLanguageServer || !tool.replacedByGopls) {
-				tools.push(tool);
-			}
-		}
-	}
-
-	// Add the language server if the user has chosen to do so.
-	if (useLanguageServer) {
-		maybeAddTool('gopls');
-	}
-
-	// Start with default tools that should always be installed.
-	for (const name of ['gotests', 'gomodifytags', 'impl', 'goplay']) {
-		maybeAddTool(name);
-	}
+	// The default tools that is required by the extension's functionality.
+	const toolNames = new Set<string>(['gotests', 'gomodifytags', 'impl', 'goplay']);
 
 	// Check if the system supports dlv, i.e. is 64-bit.
 	// There doesn't seem to be a good way to check if the mips and s390
 	// families are 64-bit, so just try to install it and hope for the best.
 	if (process.arch.match(/^(mips|mipsel|ppc64|s390|s390x|x64|arm64)$/)) {
-		maybeAddTool('dlv');
+		toolNames.add('dlv');
 	}
 
-	maybeAddTool(getFormatTool(goConfig));
+	// Configurable linter and formatter through "go.formatTool" and "go.lintTool".
+	toolNames.add(getFormatTool(goConfig));
+	const lintToolName = goConfig.get<string>('lintTool');
+	if (lintToolName) {
+		toolNames.add(lintToolName);
+	}
 
-	maybeAddTool(goConfig['lintTool']);
+	// Add the language server if the user has chosen to do so.
+	if (useLanguageServer) {
+		toolNames.add('gopls');
+	}
 
-	// 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);
+	const tools: Tool[] = [];
+	for (const name of toolNames) {
+		const tool = allToolsInformation.get(name);
+		if (!tool) {
+			continue;
+		}
+		// If language server is enabled, filter out tools that are replaced by gopls.
+		if (useLanguageServer && tool.replacedByGopls) {
+			continue;
+		}
+		tools.push(tool);
+	}
+	return tools;
 }

extension/test/integration/goExplorer.test.ts

@@ -9,7 +9,7 @@ import { TreeItem, Uri, window, workspace } from 'vscode';
 import { getGoConfig } from '../../src/config';
 
 import { GoExplorerProvider } from '../../src/goExplorer';
-import { getConfiguredTools } from '../../src/goTools';
+import { getRequiredTools } from '../../src/goTools';
 import { resolveHomeDir } from '../../src/utils/pathUtils';
 import { MockExtensionContext } from '../mocks/MockContext';
 
@@ -68,7 +68,7 @@ suite('GoExplorerProvider', () => {
 	});
 
 	test('tools tree items', async () => {
-		const allTools = getConfiguredTools(getGoConfig());
+		const allTools = getRequiredTools(getGoConfig());
 		const expectTools = allTools.map((t) => t.name);
 		const [, tools] = await explorer.getChildren()!;
 		const items = (await explorer.getChildren(tools)) as TreeItem[];

extension/test/integration/install.test.ts

@@ -15,7 +15,7 @@ import {
 	installTools,
 	maybeInstallImportantTools
 } from '../../src/goInstallTools';
-import { Tool, getConfiguredTools, getTool, getToolAtVersion } from '../../src/goTools';
+import { Tool, getRequiredTools, getTool, getToolAtVersion } from '../../src/goTools';
 import { getBinPath, getGoVersion, GoVersion, rmdirRecursive } from '../../src/util';
 import { correctBinname } from '../../src/utils/pathUtils';
 import cp = require('child_process');
@@ -355,28 +355,47 @@ function shouldRunSlowTests(): boolean {
 	return !!process.env['VSCODEGO_BEFORE_RELEASE_TESTS'];
 }
 
-suite('getConfiguredTools', () => {
-	test('require gopls when using language server', async () => {
-		const configured = getConfiguredTools(
-			new MockWorkspaceConfiguration(
-				getGoConfig(),
-				new Map<string, any>([['useLanguageServer', true]])
-			)
-		);
-		const got = configured.map((tool) => tool.name) ?? [];
-		assert(got.includes('gopls'), `omitted 'gopls': ${JSON.stringify(got)}`);
-	});
+suite('getRequiredTools', () => {
+	const testCases: {
+		name: string;
+		goConfig: Map<string, any>;
+		expectedTools: string[];
+	}[] = [
+		{
+			name: 'include gopls when {"go.useLanguageServer": true}',
+			goConfig: new Map<string, any>([['useLanguageServer', true]]),
+			// exclude third party tool that is replaced by gopls
+			expectedTools: ['gopls', 'dlv', 'gotests', 'impl', 'goplay']
+		},
+		{
+			name: 'exclude gopls when {"go.useLanguageServer": true}',
+			goConfig: new Map<string, any>([['useLanguageServer', false]]),
+			expectedTools: [
+				'dlv',
+				'impl',
+				'goplay',
+				// include third party tool that is replaced by gopls
+				'gomodifytags',
+				'gotests'
+			]
+		},
+		{
+			name: 'include golangci-lint specified in "go.lintTool"',
+			goConfig: new Map<string, any>([
+				['useLanguageServer', true],
+				['lintTool', 'golangci-lint']
+			]),
+			expectedTools: ['golangci-lint', 'gopls', 'dlv', 'gotests', 'impl', 'goplay']
+		}
+	];
 
-	test('do not require gopls when not using language server', async () => {
-		const configured = getConfiguredTools(
-			new MockWorkspaceConfiguration(
-				getGoConfig(),
-				new Map<string, any>([['useLanguageServer', false]])
-			)
-		);
-		const got = configured.map((tool) => tool.name) ?? [];
-		assert(!got.includes('gopls'), `suggested 'gopls': ${JSON.stringify(got)}`);
-	});
+	for (const tc of testCases) {
+		test(tc.name, async () => {
+			const configured = getRequiredTools(new MockWorkspaceConfiguration(getGoConfig(), tc.goConfig));
+			const got = configured.map((tool) => tool.name) ?? [];
+			assert.deepStrictEqual(got.sort(), tc.expectedTools.sort());
+		});
+	}
 });
 
 function fakeGoVersion(versionStr: string) {