settings: a top-level "gopls" block is unrecognized

[stamblerre]

For the settings UI, it's better to have the "gopls" settings always be prefixed with gopls., but it would still be nice if users who already have "gopls" blocks could have their settings recognized.

[hyangah]

Option 1. The current implementation (available in Nightly) defines each "gopls" setting as a top-level configuration property with gopls. prefix.

That allows the settings UI to recognize and render them nicely. But also this exposes too many gopls settings through the UI (~30 gopls entries currently).

The settings written in the established style (i.e. defining in a "gopls" block) are internally recognized and users can keep using the style, but the settings JSON editor doesn't recognize the style.

Option 2. Another option is to define all gopls settings as a single top-level configuration property ("gopls") of object type.

The settings UI doesn't display individual gopls settings any more and users have to edit from their settings.json files.

But enough intellisense and diagnostics feature is available in the settings.json editor.

(We can make invalidFlag or invalidValue in the example above not to be flagged if we want - for example, if we want to allow users to use newly available settings before the extension is updated).

Overall, Option2 doesn't seem too bad -

• The extension will try to plumb important build flags and environment settings if necessary.
• Users will not need to modify gopls settings in most cases. The default should work in most cases.
• Settings UI looks less cluttered. :-)

Should we change to option 2?

[marwan-at-work]

Just my 2c:

I like that Option 1 is very convenient to find: I just hit cmd + , , type gopls and all the options are there with really good documentation.

But I'd love to also get auto-complete when one of the options has to be done through "edit json" which feels more like option 2. For example, I don't get auto complete on gopls.annotations when I try to suppress some of the said diagnostics.

[hyangah]

@marwan-at-work Option 2 doesn't help with json editing of go.annotations much either. We use gopls api-json to generate the configuration and it doesn't provide a list of possible values for annotations yet - I guess it's partly because this setting is still experimental.

      {
        "Name": "annotations",
        "Type": "map[string]bool",
        "Doc": "annotations suppress various kinds of optimization diagnostics\nthat would be reported by the gc_details command.\n * noNilcheck suppresses display of nilchecks.\n * noEscape suppresses escape choices.\n * noInline suppresses inlining choices.\n * noBounds suppresses bounds checking diagnostics.\n",
        "EnumValues": null,
        "Default": "{}"
      },

gopls.codelens provides some default values, so that behaves a bit better.

We discussed this in our team meeting today. Once we clean up the existing go extension settings and sort out gopls settings, I think we will be in a better state to promote stable gopls settings to top-level go extension settings (go.buldFlags instead of gopls.buildFlags, go.analyses instead of gopls.analyses) but we are not there yet. We should do this setting promotion one by one, rather than converting every settings automatically.

There are still a handful number of experimental settings that may change, and I think some stable settings work better in VSCode Settings UI if they are renamed/reorganized. Go extension has many other settings unrelated to gopls.

E.g.,

• local, gofumpt -> better names?
• usePlaceholders -> completion.usePlaceholders? (VSCode setting UI uses . in names to define the namespace/hierarchy so, it allows us to present all related settings together if we use . wisely. See vscode configuration schema).
• linkTarget, hoverKind, linksInHover -> documentation.linkTarget, documentation.hoverKind, documentation.linksInHover?

On the other hand, we still want users to experiment with the features that are not yet promoted to the top-level (i.e. go.-prefixed). So, we start with a catch-all gopls block definition now. As gopls approaches closer to v1.0, and we know what settings are required and worth exposing to users, we can try to promote to the top level, and encourage users to configure those selected settings using the promoted settings.

[hyangah]

Bad news: Mix of "gopls": { "var": ... } style and "gopls.var": ... is problematic.
Earlier, I said it's ok, but sorry that's not true. It causes the extension to ignore all the "gopls.var": ... settings even with Option 1 setup.

I tried with the following mixed setting in the current master (Option 1).

  "gopls.linkTarget": "OPTION1",
  
 "gopls": {
    "local": "OPTION2",
  },

vscode.workspace.getConfiguration('gopls') returned the followings. See "linkTarget" has the default value, while "local" was set from the option2 style section.

{
  "buildFlags": [],
  ...
  "linkTarget": "pkg.go.dev",
  "local": "OPTION2",
  ...
 }

If a user exclusively uses one of those styles, that works fine.

The current use of "gopls."-prefix is a bad choice. :-( If we stick with OPTION1 (I hear a couple of people favor having all the info accessible through the UI), we need a different prefix (e.g."go.gopls." or "go."), and actively deprecate the use of "gopls": {} style setting.

[hyangah]

We took option 2, and the top-level "gopls" block is now recognized.
When we need some settings to be configured by users more frequently, we will explicitly promote them top-level and propagate them to gopls.