src/goLanguageServer: propagate go.buildFlags,buildTags to gopls

They are passed as gopls.buildFlags only if gopls.buildFlags isn't
already explicitly set - users have to use JSON-based UI and add
the fields (including []).

If go.buildFlags has -tags, go.buildTags is ignored, which is
consistent with the extension's build behavior coded in goBuild
(goBuild.ts).

For #155

Change-Id: I03d5f5eb58d049454fb4a66d902a7daea4a1269f
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/273146
Trust: Hyang-Ah Hana Kim <[email protected]>
Trust: Suzy Mueller <[email protected]>
Run-TryBot: Hyang-Ah Hana Kim <[email protected]>
TryBot-Result: kokoro <[email protected]>
Reviewed-by: Suzy Mueller <[email protected]>

Author: hyangah

docs/settings.md

@@ -79,7 +79,7 @@ Default: `false`
 
 ### `go.buildFlags`
 
-Flags to `go build`/`go test` used during build-on-save or running tests. (e.g. ["-ldflags='-s'"])
+Flags to `go build`/`go test` used during build-on-save or running tests. (e.g. ["-ldflags='-s'"]) This is propagated to the language server if `gopls.buildFlags` is not specified.
 
 ### `go.buildOnSave`
 
@@ -91,7 +91,7 @@ Default: `package`
 
 ### `go.buildTags`
 
-The Go build tags to use for all commands, that support a `-tags '...'` argument. When running tests, go.testTags will be used instead if it was set.
+The Go build tags to use for all commands, that support a `-tags '...'` argument. When running tests, go.testTags will be used instead if it was set. This is propagated to the language server if `gopls.buildFlags` is not specified.
 
 Default: ``
 
@@ -617,6 +617,8 @@ buildFlags is the set of flags passed on to the build system when invoked.
 It is applied to queries like `go list`, which is used when discovering files.
 The most common use is to set `-tags`.
 
+If unspecified, values of `go.buildFlags, go.buildTags` will be propagated.
+
 
 #### `codelenses`
 codelenses overrides the enabled/disabled state of code lenses. See the "Code Lenses"
@@ -626,7 +628,7 @@ Example Usage:
 ```json5
 "gopls": {
 ...
-  "codelenses": {
+  "codelens": {
     "generate": false,  // Don't show the `go generate` lens.
     "gc_details": true  // Show a code lens toggling the display of gc's choices.
   }

package.json

@@ -1222,13 +1222,13 @@
             "type": "string"
           },
           "default": [],
-          "description": "Flags to `go build`/`go test` used during build-on-save or running tests. (e.g. [\"-ldflags='-s'\"])",
+          "description": "Flags to `go build`/`go test` used during build-on-save or running tests. (e.g. [\"-ldflags='-s'\"]) This is propagated to the language server if `gopls.buildFlags` is not specified.",
           "scope": "resource"
         },
         "go.buildTags": {
           "type": "string",
           "default": "",
-          "description": "The Go build tags to use for all commands, that support a `-tags '...'` argument. When running tests, go.testTags will be used instead if it was set.",
+          "description": "The Go build tags to use for all commands, that support a `-tags '...'` argument. When running tests, go.testTags will be used instead if it was set. This is propagated to the language server if `gopls.buildFlags` is not specified.",
           "scope": "resource"
         },
         "go.testTags": {
@@ -2068,7 +2068,7 @@
           "properties": {
             "buildFlags": {
               "type": "array",
-              "markdownDescription": "buildFlags is the set of flags passed on to the build system when invoked.\nIt is applied to queries like `go list`, which is used when discovering files.\nThe most common use is to set `-tags`.\n",
+              "markdownDescription": "buildFlags is the set of flags passed on to the build system when invoked.\nIt is applied to queries like `go list`, which is used when discovering files.\nThe most common use is to set `-tags`.\n\nIf unspecified, values of `go.buildFlags, go.buildTags` will be propagated.\n",
               "default": [],
               "scope": "resource"
             },
@@ -2130,7 +2130,7 @@
             },
             "codelenses": {
               "type": "object",
-              "markdownDescription": "codelenses overrides the enabled/disabled state of code lenses. See the \"Code Lenses\"\nsection of settings.md for the list of supported lenses.\n\nExample Usage:\n```json5\n\"gopls\": {\n...\n  \"codelenses\": {\n    \"generate\": false,  // Don't show the `go generate` lens.\n    \"gc_details\": true  // Show a code lens toggling the display of gc's choices.\n  }\n...\n}\n```\n",
+              "markdownDescription": "codelenses overrides the enabled/disabled state of code lenses. See the \"Code Lenses\"\nsection of settings.md for the list of supported lenses.\n\nExample Usage:\n```json5\n\"gopls\": {\n...\n  \"codelens\": {\n    \"generate\": false,  // Don't show the `go generate` lens.\n    \"gc_details\": true  // Show a code lens toggling the display of gc's choices.\n  }\n...\n}\n```\n",
               "default": {
                 "gc_details": false,
                 "generate": true,

src/goLanguageServer.ts

@@ -290,9 +290,7 @@ function buildLanguageClientOption(cfg: LanguageServerConfig): BuildLanguageClie
 // buildLanguageClient returns a language client built using the given language server config.
 // The returned language client need to be started before use.
 export async function buildLanguageClient(cfg: BuildLanguageClientOption): Promise<LanguageClient> {
-	let goplsWorkspaceConfig = getGoplsConfig() as any;
-	goplsWorkspaceConfig = filterDefaultConfigValues(goplsWorkspaceConfig, 'gopls', undefined);
-	goplsWorkspaceConfig = await adjustGoplsWorkspaceConfiguration(cfg, goplsWorkspaceConfig);
+	const goplsWorkspaceConfig = await adjustGoplsWorkspaceConfiguration(cfg, getGoplsConfig(), 'gopls', undefined);
 	const c = new LanguageClient(
 		'go',  // id
 		cfg.serverName,  // name e.g. gopls
@@ -505,8 +503,7 @@ export async function buildLanguageClient(cfg: BuildLanguageClientOption): Promi
 								const scopeUri = params.items[i].scopeUri;
 								const resource = scopeUri ? vscode.Uri.parse(scopeUri) : undefined;
 								const section = params.items[i].section;
-								workspaceConfig = filterDefaultConfigValues(workspaceConfig, section, resource);
-								workspaceConfig = await adjustGoplsWorkspaceConfiguration(cfg, workspaceConfig);
+								workspaceConfig = await adjustGoplsWorkspaceConfiguration(cfg, workspaceConfig, section, resource);
 							}
 							ret.push(workspaceConfig);
 						}
@@ -519,19 +516,15 @@ export async function buildLanguageClient(cfg: BuildLanguageClientOption): Promi
 	return c;
 }
 
-// filterDefaultConfigValues removes the entries filled based on the default values
+// filterGoplsDefaultConfigValues removes the entries filled based on the default values
 // and selects only those the user explicitly specifies in their settings.
-// This assumes workspaceConfig is a non-null(undefined) object type.
+// This returns a new object created based on the filtered properties of workspaceConfig.
 // Exported for testing.
-export function filterDefaultConfigValues(workspaceConfig: any, section: string, resource: vscode.Uri): any {
+export function filterGoplsDefaultConfigValues(workspaceConfig: any, resource: vscode.Uri): any {
 	if (!workspaceConfig) {
-		return workspaceConfig;
+		workspaceConfig = {};
 	}
-
-	const dot = section?.lastIndexOf('.') || -1;
-	const sectionKey = dot >= 0 ? section.substr(0, dot) : section;  // e.g. 'gopls'
-
-	const cfg = vscode.workspace.getConfiguration(sectionKey, resource);
+	const cfg = getGoplsConfig(resource);
 	const filtered = {} as { [key: string]: any };
 	for (const [key, value] of Object.entries(workspaceConfig)) {
 		if (typeof value === 'function') {
@@ -556,31 +549,63 @@ export function filterDefaultConfigValues(workspaceConfig: any, section: string,
 	return filtered;
 }
 
-// adjustGoplsWorkspaceConfiguration adds any extra options to the gopls
-// config. Right now, the only extra option is enabling experiments for the
-// Nightly extension.
-async function adjustGoplsWorkspaceConfiguration(cfg: LanguageServerConfig, config: any): Promise<any> {
-	if (!config) {
-		return config;
+// passGoConfigToGoplsConfigValues passes some of the relevant 'go.' settings to gopls settings.
+// This assumes `goplsWorkspaceConfig` is an output of filterGoplsDefaultConfigValues,
+// so it is modifiable and doesn't contain properties that are not explicitly set.
+//   - go.buildTags and go.buildFlags are passed as gopls.buildFlags
+//     if goplsWorkspaceConfig doesn't explicitly set it yet.
+// Exported for testing.
+export function passGoConfigToGoplsConfigValues(goplsWorkspaceConfig: any, goWorkspaceConfig: any): any {
+	if (!goplsWorkspaceConfig) {
+		goplsWorkspaceConfig = {};
+	}
+
+	// If gopls.buildFlags is set, don't touch it.
+	if (goplsWorkspaceConfig.buildFlags === undefined) {
+		const buildFlags = [] as string[];
+		if (goWorkspaceConfig?.buildFlags) {
+			buildFlags.push(...goWorkspaceConfig?.buildFlags);
+		}
+		if (goWorkspaceConfig?.buildTags && buildFlags.indexOf('-tags') === -1) {
+			buildFlags.push('-tags', goWorkspaceConfig?.buildTags);
+		}
+		if (buildFlags.length > 0) {
+			goplsWorkspaceConfig.buildFlags = buildFlags;
+		}
 	}
+	return goplsWorkspaceConfig;
+}
+
+// adjustGoplsWorkspaceConfiguration filters unnecessary options and adds any necessary, additional
+// options to the gopls config. See filterGoplsDefaultConfigValues, passGoConfigToGoplsConfigValues.
+// If this is for the nightly extension, we also request to activate features under experiments.
+async function adjustGoplsWorkspaceConfiguration(cfg: LanguageServerConfig, workspaceConfig: any, section: string, resource: vscode.Uri): Promise<any> {
+	// We process only gopls config
+	if (section !== 'gopls') {
+		return workspaceConfig;
+	}
+
+	workspaceConfig = filterGoplsDefaultConfigValues(workspaceConfig, resource);
+	// note: workspaceConfig is a modifiable, valid object.
+	workspaceConfig = passGoConfigToGoplsConfigValues(workspaceConfig, getGoConfig(resource));
+
 	// Only modify the user's configurations for the Nightly.
 	if (extensionId !== 'golang.go-nightly') {
-		return config;
+		return workspaceConfig;
 	}
 	// allExperiments is only available with gopls/v0.5.2 and above.
 	const version = await getLocalGoplsVersion(cfg);
 	if (!version) {
-		return config;
+		return workspaceConfig;
 	}
 	const sv = semver.parse(version, true);
 	if (!sv || semver.lt(sv, 'v0.5.2')) {
-		return config;
+		return workspaceConfig;
 	}
-	const newConfig = Object.assign({}, config);
-	if (!config['allExperiments']) {
-		newConfig['allExperiments'] = true;
+	if (!workspaceConfig['allExperiments']) {
+		workspaceConfig['allExperiments'] = true;
 	}
-	return newConfig;
+	return workspaceConfig;
 }
 
 // createTestCodeLens adds the go.test.cursor and go.debug.cursor code lens

src/util.ts

@@ -158,12 +158,12 @@ let toolsGopath: string;
 
 // getGoConfig is declared as an exported const rather than a function, so it can be stubbbed in testing.
 export const getGoConfig = (uri?: vscode.Uri) => {
-	return getConfig('go');
+	return getConfig('go', uri);
 };
 
 // getGoplsConfig returns the user's gopls configuration.
-export function getGoplsConfig() {
-	return getConfig('gopls');
+export function getGoplsConfig(uri?: vscode.Uri) {
+	return getConfig('gopls', uri);
 }
 
 // getCheckForToolsUpdatesConfig returns go.toolsManagement.checkForUpdates configuration.

test/gopls/configuration.test.ts

@@ -12,27 +12,33 @@ import * as lsp from '../../src/goLanguageServer';
 import { getTool, Tool } from '../../src/goTools';
 
 suite('gopls configuration tests', () => {
-	test('configuration', async () => {
+	test('filterGoplsDefaultConfigValues', async () => {
 		const defaultGoplsConfig = vscode.workspace.getConfiguration('gopls');
 		const defaultGoplsAnalysesConfig = vscode.workspace.getConfiguration('gopls.analyses');
 
 		interface TestCase {
 			name: string;
 			section: string;
-			base: any;
+			input: any;
 			want: any;
 		}
 		const testCases: TestCase[] = [
 			{
 				name: 'user set no gopls settings',
 				section: 'gopls',
-				base: defaultGoplsConfig,
+				input: defaultGoplsConfig,
 				want: {}
 			},
 			{
 				name: 'user set some gopls settings',
 				section: 'gopls',
-				base: defaultGoplsConfig,
+				input: Object.assign({}, defaultGoplsConfig, {
+					buildFlags: ['-something'],
+					env: { foo: 'bar' },
+					hoverKind: 'NoDocumentation',
+					usePlaceholders: true,
+					linkTarget: 'godoc.org'
+				}),
 				want: {
 					buildFlags: ['-something'],
 					env: { foo: 'bar' },
@@ -42,37 +48,119 @@ suite('gopls configuration tests', () => {
 				},
 			},
 			{
-				name: 'gopls asks analyses section, user set no analysis',
-				section: 'gopls.analyses',
-				base: defaultGoplsAnalysesConfig,
+				name: 'user set extra gopls settings',
+				section: 'gopls',
+				input: Object.assign({}, defaultGoplsConfig, {
+					undefinedGoplsSetting: true
+				}),
+				want: {
+					undefinedGoplsSetting: true,
+				},
+			},
+			{
+				name: 'never returns undefined',
+				section: 'undefined.section',
+				input: undefined,
 				want: {},
 			},
+		];
+		testCases.map((tc: TestCase) => {
+			const actual = lsp.filterGoplsDefaultConfigValues(tc.input, undefined);
+			assert.deepStrictEqual(actual, tc.want, `Failed: ${tc.name}`);
+		});
+	});
+
+	test('passGoConfigToGoplsConfigValues', async () => {
+		interface TestCase {
+			name: string;
+			goplsConfig: any;
+			goConfig: any;
+			want: any;
+		}
+		const testCases: TestCase[] = [
 			{
-				name: 'gopls asks analyses section, user set some',
-				section: 'gopls.analyses',
-				base: defaultGoplsAnalysesConfig,
-				want: {
-					coolAnalysis: true,
+				name: 'undefined gopls, go configs result in an empty config',
+				goplsConfig: undefined,
+				goConfig: undefined,
+				want: {}
+			},
+			{
+				name: 'empty gopls, go configs result in an empty config',
+				goplsConfig: {},
+				goConfig: {},
+				want: {}
+			},
+			{
+				name: 'empty gopls, default go configs result in an empty config',
+				goplsConfig: {},
+				goConfig: {
+					buildFlags: [],
+					buildTags: '',
 				},
+				want: {}
 			},
 			{
-				name: 'user set extra gopls settings',
-				section: 'gopls',
-				base: defaultGoplsConfig,
+				name: 'pass go config buildFlags to gopls config',
+				goplsConfig: {},
+				goConfig: { buildFlags: ['-modfile', 'gopls.mod', '-tags', 'tag1,tag2', '-modcacherw'] },
+				want: { buildFlags: ['-modfile', 'gopls.mod', '-tags', 'tag1,tag2', '-modcacherw']}
+			},
+			{
+				name: 'pass go config buildTags to gopls config',
+				goplsConfig: {},
+				goConfig: { buildTags: 'tag1,tag2' },
+				want: { buildFlags: ['-tags', 'tag1,tag2']}
+			},
+			{
+				name: 'do not pass go config buildTags if buildFlags already have tags',
+				goplsConfig: {},
+				goConfig: {
+					buildFlags: ['-tags', 'tag0'],
+					buildTags: 'tag1,tag2'
+				},
+				want: { buildFlags: ['-tags', 'tag0']}
+			},
+			{
+				name: 'do not mutate other gopls config but gopls.buildFlags',
+				goplsConfig: {
+					env: {GOPROXY: 'direct'}
+				},
+				goConfig: { buildFlags: ['-modfile', 'gopls.mod', '-tags', 'tag1,tag2', '-modcacherw'] },
 				want: {
-					undefinedGoplsSetting: true,
+					env: { GOPROXY : 'direct' },
+					buildFlags: ['-modfile', 'gopls.mod', '-tags', 'tag1,tag2', '-modcacherw']}
+			},
+
+			{
+				name: 'do not mutate misconfigured gopls.buildFlags',
+				goplsConfig: {
+					buildFlags: '-modfile gopls.mod',  // misconfiguration
+				},
+				goConfig: {
+					buildFlags: '-modfile go.mod -tags tag1 -modcacherw',
 				},
+				want: { buildFlags: '-modfile gopls.mod' }
 			},
 			{
-				name: 'gopls asks undefined config section',
-				section: 'undefined.section',
-				base: undefined,
-				want: {},
-			}
+				name: 'do not overwrite gopls config if it is explicitly set',
+				goplsConfig: {
+					env: {GOPROXY: 'direct'},
+					buildFlags: [],  // empty
+				},
+				goConfig: {
+					// expect only non-conflicting flags (tags, modcacherw) passing.
+					buildFlags: ['-modfile go.mod -tags tag1 -modcacherw'],
+					buildTags: 'tag3',
+				},
+				want: {
+					env: {GOPROXY: 'direct'},
+					buildFlags: [],
+				}  // gopls.buildFlags untouched.
+			},
+
 		];
 		testCases.map((tc: TestCase) => {
-			const input = Object.assign({}, tc.base, tc.want);
-			const actual = lsp.filterDefaultConfigValues(input, tc.section, undefined);
+			const actual = lsp.passGoConfigToGoplsConfigValues(tc.goplsConfig, tc.goConfig);
 			assert.deepStrictEqual(actual, tc.want, `Failed: ${tc.name}`);
 		});
 	});