From f38e34851acc2d6e4f7fce1ed34670941330e57d Mon Sep 17 00:00:00 2001 From: Hana Date: Wed, 16 Dec 2020 14:42:07 -0500 Subject: [PATCH] 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 golang/vscode-go#155 Change-Id: I03d5f5eb58d049454fb4a66d902a7daea4a1269f Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/273146 Trust: Hyang-Ah Hana Kim Trust: Suzy Mueller Run-TryBot: Hyang-Ah Hana Kim TryBot-Result: kokoro Reviewed-by: Suzy Mueller --- docs/settings.md | 8 +- package.json | 8 +- src/goLanguageServer.ts | 79 +++++++++++------- src/util.ts | 6 +- test/gopls/configuration.test.ts | 134 +++++++++++++++++++++++++------ tools/goplssetting/main.go | 10 ++- 6 files changed, 184 insertions(+), 61 deletions(-) diff --git a/docs/settings.md b/docs/settings.md index 80cf4bf93e..5d6b5773a9 100644 --- a/docs/settings.md +++ b/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. } diff --git a/package.json b/package.json index bd1e5ef2eb..ad9512e208 100644 --- a/package.json +++ b/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, diff --git a/src/goLanguageServer.ts b/src/goLanguageServer.ts index 8ef78b9b45..9d4db9d0b8 100644 --- a/src/goLanguageServer.ts +++ b/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 { - 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 { - 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 { + // 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 diff --git a/src/util.ts b/src/util.ts index 248d569c7e..6211b57fbb 100644 --- a/src/util.ts +++ b/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. diff --git a/test/gopls/configuration.test.ts b/test/gopls/configuration.test.ts index e2adea4077..5d68043b6e 100644 --- a/test/gopls/configuration.test.ts +++ b/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}`); }); }); diff --git a/tools/goplssetting/main.go b/tools/goplssetting/main.go index 868a027243..e10955f133 100644 --- a/tools/goplssetting/main.go +++ b/tools/goplssetting/main.go @@ -194,7 +194,11 @@ func writeAsVSCodeSettings(f io.Writer, options []*OptionJSON) { line(` "type": %q,`, typ) // TODO: consider 'additionalProperties' if gopls api-json outputs acceptable peoperties. - line(` "markdownDescription": %q,`, o.Doc) + doc := o.Doc + if mappedTo, ok := associatedToExtensionProperties[o.Name]; ok { + doc = fmt.Sprintf("%v\nIf unspecified, values of `%v` will be propagated.\n", doc, strings.Join(mappedTo, ", ")) + } + line(` "markdownDescription": %q,`, doc) var enums, enumDocs []string for _, v := range o.EnumValues { @@ -226,6 +230,10 @@ func writeAsVSCodeSettings(f io.Writer, options []*OptionJSON) { line(`}`) // { } +var associatedToExtensionProperties = map[string][]string{ + "buildFlags": []string{"go.buildFlags", "go.buildTags"}, +} + func propertyType(t string) string { switch t { case "string":