internal/lsp: use an enum for GC annotations settings

The annotations map should use an enum to indicate expected values. For now, we reuse the EnumValues to expose the information in the settings. Later, we'll create a separate EnumKeys field to expose this information more correctly. Also, adjust some of the logic that applies the settings because it was incorrect. Both gopls/doc/settings.md and internal/lsp/source/api_json.go are generated files. Updates golang/go#42961 Change-Id: Ifb032b70caaae73defe9a540df20d098d313e68e Reviewed-on: https://go-review.googlesource.com/c/tools/+/280354 Trust: Rebecca Stambler <rstambler@golang.org> Run-TryBot: Rebecca Stambler <rstambler@golang.org> gopls-CI: kokoro <noreply+kokoro@google.com> TryBot-Result: Go Bot <gobot@golang.org> Reviewed-by: Peter Weinberger <pjw@google.com>
2020-12-26 02:13:31 -05:00 · 2020-12-26 02:13:31 -05:00 · 34cd474b99
--- a/gopls/doc/generate.go
+++ b/gopls/doc/generate.go
@ -163,12 +163,21 @@ func loadOptions(category reflect.Value, pkg *packages.Package) ([]*source.Optio
 			typ = "enum"
 		}

+		// Track any maps whose keys are enums.
+		enumValues := enums[typesField.Type()]
+		if m, ok := typesField.Type().(*types.Map); ok {
+			if e, ok := enums[m.Key()]; ok {
+				enumValues = e
+				typ = strings.Replace(typ, m.Key().String(), m.Key().Underlying().String(), 1)
+			}
+		}
+
 		opts = append(opts, &source.OptionJSON{
 			Name:       lowerFirst(typesField.Name()),
 			Type:       typ,
 			Doc:        lowerFirst(astField.Doc.Text()),
 			Default:    string(defBytes),
-			EnumValues: enums[typesField.Type()],
+			EnumValues: enumValues,
 		})
 	}
 	return opts, nil
@ -378,14 +387,23 @@ func rewriteSettings(doc []byte, api *source.APIJSON) ([]byte, error) {
 		for _, opt := range opts {
 			var enumValues strings.Builder
 			if len(opt.EnumValues) > 0 {
-				enumValues.WriteString("Must be one of:\n\n")
-				for _, val := range opt.EnumValues {
+				var msg string
+				if opt.Type == "enum" {
+					msg = "\nMust be one of:\n\n"
+				} else {
+					msg = "\nCan contain any of:\n\n"
+				}
+				enumValues.WriteString(msg)
+				for i, val := range opt.EnumValues {
 					if val.Doc != "" {
 						// Don't break the list item by starting a new paragraph.
 						unbroken := parBreakRE.ReplaceAllString(val.Doc, "\\\n")
-						fmt.Fprintf(&enumValues, " * %s\n", unbroken)
+						fmt.Fprintf(&enumValues, "* %s", unbroken)
 					} else {
-						fmt.Fprintf(&enumValues, " * `%s`\n", val.Value)
+						fmt.Fprintf(&enumValues, "* `%s`", val.Value)
+					}
+					if i < len(opt.EnumValues)-1 {
+						fmt.Fprint(&enumValues, "\n")
 					}
 				}
 			}
--- a/gopls/doc/settings.md
+++ b/gopls/doc/settings.md
@ -36,18 +36,18 @@ Default: `{}`.
 ### **hoverKind** *enum*
 hoverKind controls the information that appears in the hover text.
 SingleLine and Structured are intended for use only by authors of editor plugins.
+
 Must be one of:

- * `"FullDocumentation"`
- * `"NoDocumentation"`
- * `"SingleLine"`
- * `"Structured"` is an experimental setting that returns a structured hover format.
+* `"FullDocumentation"`
+* `"NoDocumentation"`
+* `"SingleLine"`
+* `"Structured"` is an experimental setting that returns a structured hover format.
 This format separates the signature from the documentation, so that the client
 can do more manipulation of these fields.\
 This should only be used by clients that support this behavior.

- * `"SynopsisDocumentation"`
-
+* `"SynopsisDocumentation"`

 Default: `"FullDocumentation"`.
 ### **usePlaceholders** *bool*
@ -120,32 +120,32 @@ Default: `true`.
 ### **importShortcut** *enum*
 importShortcut specifies whether import statements should link to
 documentation or go to definitions.
+
 Must be one of:

- * `"Both"`
- * `"Definition"`
- * `"Link"`
-
+* `"Both"`
+* `"Definition"`
+* `"Link"`

 Default: `"Both"`.
 ### **matcher** *enum*
 matcher sets the algorithm that is used when calculating completion candidates.
+
 Must be one of:

- * `"CaseInsensitive"`
- * `"CaseSensitive"`
- * `"Fuzzy"`
-
+* `"CaseInsensitive"`
+* `"CaseSensitive"`
+* `"Fuzzy"`

 Default: `"Fuzzy"`.
 ### **symbolMatcher** *enum*
 symbolMatcher sets the algorithm that is used when finding workspace symbols.
+
 Must be one of:

- * `"CaseInsensitive"`
- * `"CaseSensitive"`
- * `"Fuzzy"`
-
+* `"CaseInsensitive"`
+* `"CaseSensitive"`
+* `"Fuzzy"`

 Default: `"Fuzzy"`.
 ### **symbolStyle** *enum*
@ -159,21 +159,21 @@ Example Usage:
 ...
 }
 ```
+
 Must be one of:

- * `"Dynamic"` uses whichever qualifier results in the highest scoring
+* `"Dynamic"` uses whichever qualifier results in the highest scoring
 match for the given symbol query. Here a "qualifier" is any "/" or "."
 delimited suffix of the fully qualified symbol. i.e. "to/pkg.Foo.Field" or
 just "Foo.Field".

- * `"Full"` is fully qualified symbols, i.e.
+* `"Full"` is fully qualified symbols, i.e.
 "path/to/pkg.Foo.Field".

- * `"Package"` is package qualified symbols i.e.
+* `"Package"` is package qualified symbols i.e.
 "pkg.Foo.Field".


-
 Default: `"Dynamic"`.
 ### **directoryFilters** *[]string*
 directoryFilters can be used to exclude unwanted directories from the
@ -198,15 +198,21 @@ The below settings are considered experimental. They may be deprecated or change

 <!-- BEGIN Experimental: DO NOT MANUALLY EDIT THIS SECTION -->
 ### **annotations** *map[string]bool*
-annotations suppress various kinds of optimization diagnostics
-that would be reported by the gc_details command.
- * noNilcheck suppresses display of nilchecks.
- * noEscape suppresses escape choices.
- * noInline suppresses inlining choices.
- * noBounds suppresses bounds checking diagnostics.
+annotations specifies the various kinds of optimization diagnostics
+that should be reported by the gc_details command.
+
+Can contain any of:
+
+* `"bounds"` controls bounds checking diagnostics.
+
+* `"escape"` controls diagnostics about escape choices.
+
+* `"inline"` controls diagnostics about inlining choices.
+
+* `"nil"` controls nil checks.


-Default: `{}`.
+Default: `{"bounds":true,"escape":true,"inline":true,"nil":true}`.
 ### **staticcheck** *bool*
 staticcheck enables additional analyses from staticcheck.io.

--- a/internal/lsp/source/api_json.go
+++ b/internal/lsp/source/api_json.go
@ -22,11 +22,28 @@ var GeneratedAPIJSON = &APIJSON{
 		},
 		"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: nil,
-				Default:    "{}",
+				Name: "annotations",
+				Type: "map[string]bool",
+				Doc:  "annotations specifies the various kinds of optimization diagnostics\nthat should be reported by the gc_details command.\n",
+				EnumValues: []EnumValue{
+					{
+						Value: "\"bounds\"",
+						Doc:   "`\"bounds\"` controls bounds checking diagnostics.\n",
+					},
+					{
+						Value: "\"escape\"",
+						Doc:   "`\"escape\"` controls diagnostics about escape choices.\n",
+					},
+					{
+						Value: "\"inline\"",
+						Doc:   "`\"inline\"` controls diagnostics about inlining choices.\n",
+					},
+					{
+						Value: "\"nil\"",
+						Doc:   "`\"nil\"` controls nil checks.\n",
+					},
+				},
+				Default: "{\"bounds\":true,\"escape\":true,\"inline\":true,\"nil\":true}",
 			},
 			{
 				Name:       "staticcheck",
--- a/internal/lsp/source/gc_annotations.go
+++ b/internal/lsp/source/gc_annotations.go
@ -19,6 +19,22 @@ import (
 	"golang.org/x/tools/internal/span"
 )

+type Annotation string
+
+const (
+	// Nil controls nil checks.
+	Nil Annotation = "nil"
+
+	// Escape controls diagnostics about escape choices.
+	Escape Annotation = "escape"
+
+	// Inline controls diagnostics about inlining choices.
+	Inline Annotation = "inline"
+
+	// Bounds controls bounds checking diagnostics.
+	Bounds Annotation = "bounds"
+)
+
 func GCOptimizationDetails(ctx context.Context, snapshot Snapshot, pkgDir span.URI) (map[VersionedFileIdentity][]*Diagnostic, error) {
 	outDir := filepath.Join(os.TempDir(), fmt.Sprintf("gopls-%d.details", os.Getpid()))

@ -113,7 +129,7 @@ func parseDetailsFile(filename string, options *Options) (span.URI, []*Diagnosti
 		if msg != "" {
 			msg = fmt.Sprintf("%s(%s)", msg, d.Message)
 		}
-		if skipDiagnostic(msg, d.Source, options) {
+		if !showDiagnostic(msg, d.Source, options) {
 			continue
 		}
 		var related []RelatedInformation
@ -138,24 +154,27 @@ func parseDetailsFile(filename string, options *Options) (span.URI, []*Diagnosti
 	return uri, diagnostics, nil
 }

-// skipDiagnostic reports whether a given diagnostic should be shown to the end
+// showDiagnostic reports whether a given diagnostic should be shown to the end
 // user, given the current options.
-func skipDiagnostic(msg, source string, o *Options) bool {
+func showDiagnostic(msg, source string, o *Options) bool {
 	if source != "go compiler" {
 		return false
 	}
+	if o.Annotations == nil {
+		return true
+	}
 	switch {
-	case o.Annotations["noInline"]:
-		return strings.HasPrefix(msg, "canInline") ||
-			strings.HasPrefix(msg, "cannotInline") ||
-			strings.HasPrefix(msg, "inlineCall")
-	case o.Annotations["noEscape"]:
-		return strings.HasPrefix(msg, "escape") || msg == "leak"
-	case o.Annotations["noNilcheck"]:
-		return strings.HasPrefix(msg, "nilcheck")
-	case o.Annotations["noBounds"]:
-		return strings.HasPrefix(msg, "isInBounds") ||
-			strings.HasPrefix(msg, "isSliceInBounds")
+	case strings.HasPrefix(msg, "canInline") ||
+		strings.HasPrefix(msg, "cannotInline") ||
+		strings.HasPrefix(msg, "inlineCall"):
+		return o.Annotations[Inline]
+	case strings.HasPrefix(msg, "escape") || msg == "leak":
+		return o.Annotations[Escape]
+	case strings.HasPrefix(msg, "nilcheck"):
+		return o.Annotations[Nil]
+	case strings.HasPrefix(msg, "isInBounds") ||
+		strings.HasPrefix(msg, "isSliceInBounds"):
+		return o.Annotations[Bounds]
 	}
 	return false
 }
--- a/internal/lsp/source/options.go
+++ b/internal/lsp/source/options.go
@ -125,6 +125,12 @@ func DefaultOptions() *Options {
 				ExpandWorkspaceToModule:      true,
 				ExperimentalPackageCacheKey:  true,
 				ExperimentalDiagnosticsDelay: 250 * time.Millisecond,
+				Annotations: map[Annotation]bool{
+					Bounds: true,
+					Escape: true,
+					Inline: true,
+					Nil:    true,
+				},
 			},
 			InternalOptions: InternalOptions{
 				LiteralCompletions:      true,
@ -323,13 +329,9 @@ type Hooks struct {
 // only exists while these features are under development.
 type ExperimentalOptions struct {

-	// Annotations suppress various kinds of optimization diagnostics
-	// that would be reported by the gc_details command.
-	//  * noNilcheck suppresses display of nilchecks.
-	//  * noEscape suppresses escape choices.
-	//  * noInline suppresses inlining choices.
-	//  * noBounds suppresses bounds checking diagnostics.
-	Annotations map[string]bool
+	// Annotations specifies the various kinds of optimization diagnostics
+	// that should be reported by the gc_details command.
+	Annotations map[Annotation]bool

 	// Staticcheck enables additional analyses from staticcheck.io.
 	Staticcheck bool
@ -610,7 +612,6 @@ func (o *Options) Clone() *Options {
 		return dst
 	}
 	result.Analyses = copyStringMap(o.Analyses)
-	result.Annotations = copyStringMap(o.Annotations)
 	result.Codelenses = copyStringMap(o.Codelenses)

 	copySlice := func(src []string) []string {
@ -762,16 +763,7 @@ func (o *Options) set(name string, value interface{}) OptionResult {
 		result.setBoolMap(&o.Analyses)

 	case "annotations":
-		result.setBoolMap(&o.Annotations)
-		for k := range o.Annotations {
-			switch k {
-			case "noEscape", "noNilcheck", "noInline", "noBounds":
-				continue
-			default:
-				result.Name += ":" + k // put mistake(s) in the message
-				result.State = OptionUnexpected
-			}
-		}
+		result.setAnnotationMap(&o.Annotations)

 	case "codelenses", "codelens":
 		var lensOverrides map[string]bool
@ -915,10 +907,55 @@ func (r *OptionResult) setDuration(d *time.Duration) {
 }

 func (r *OptionResult) setBoolMap(bm *map[string]bool) {
+	m := r.asBoolMap()
+	*bm = m
+}
+
+func (r *OptionResult) setAnnotationMap(bm *map[Annotation]bool) {
+	all := r.asBoolMap()
+	if all == nil {
+		return
+	}
+	// Default to everything enabled by default.
+	m := make(map[Annotation]bool)
+	for k, enabled := range all {
+		a, err := asOneOf(
+			k,
+			string(Nil),
+			string(Escape),
+			string(Inline),
+			string(Bounds),
+		)
+		if err != nil {
+			// In case of an error, process any legacy values.
+			switch k {
+			case "noEscape":
+				m[Escape] = false
+				r.errorf(`"noEscape" is deprecated, set "Escape: false" instead`)
+			case "noNilcheck":
+				m[Nil] = false
+				r.errorf(`"noNilcheck" is deprecated, set "Nil: false" instead`)
+			case "noInline":
+				m[Inline] = false
+				r.errorf(`"noInline" is deprecated, set "Inline: false" instead`)
+			case "noBounds":
+				m[Bounds] = false
+				r.errorf(`"noBounds" is deprecated, set "Bounds: false" instead`)
+			default:
+				r.errorf(err.Error())
+			}
+			continue
+		}
+		m[Annotation(a)] = enabled
+	}
+	*bm = m
+}
+
+func (r *OptionResult) asBoolMap() map[string]bool {
 	all, ok := r.Value.(map[string]interface{})
 	if !ok {
 		r.errorf("invalid type %T for map[string]bool option", r.Value)
-		return
+		return nil
 	}
 	m := make(map[string]bool)
 	for a, enabled := range all {
@ -926,10 +963,10 @@ func (r *OptionResult) setBoolMap(bm *map[string]bool) {
 			m[a] = enabled
 		} else {
 			r.errorf("invalid type %T for map key %q", enabled, a)
-			return
+			return m
 		}
 	}
-	*bm = m
+	return m
 }

 func (r *OptionResult) asString() (string, bool) {
@ -946,14 +983,21 @@ func (r *OptionResult) asOneOf(options ...string) (string, bool) {
 	if !ok {
 		return "", false
 	}
-	lower := strings.ToLower(s)
+	s, err := asOneOf(s, options...)
+	if err != nil {
+		r.errorf(err.Error())
+	}
+	return s, err == nil
+}
+
+func asOneOf(str string, options ...string) (string, error) {
+	lower := strings.ToLower(str)
 	for _, opt := range options {
 		if strings.ToLower(opt) == lower {
-			return opt, true
+			return opt, nil
 		}
 	}
-	r.errorf("invalid option %q for enum", r.Value)
-	return "", false
+	return "", fmt.Errorf("invalid option %q for enum", str)
 }

 func (r *OptionResult) setString(s *string) {
--- a/internal/lsp/source/options_test.go
+++ b/internal/lsp/source/options_test.go
@ -148,6 +148,17 @@ func TestSetOption(t *testing.T) {
 				return len(o.DirectoryFilters) == 0
 			},
 		},
+		{
+			name: "annotations",
+			value: map[string]interface{}{
+				"Nil":      false,
+				"noBounds": true,
+			},
+			wantError: true,
+			check: func(o Options) bool {
+				return !o.Annotations[Nil] && !o.Annotations[Bounds]
+			},
+		},
 	}

 	for _, test := range tests {