Revert "internal/settings: drop "annotations" setting"

This reverts commit 5fe60fd (CL 639835), which removed the
ability for users to customize the subset of "annotations"
(a misnomer for categories of compiler optimization details).
Apparently some users were relying on this experimental feature.

Minor tweaks were made to comments but not to logic.

Fixes golang/go#71888

Change-Id: I3d0227f841582a2cb29521b9b999546226b670ef
Reviewed-on: https://go-review.googlesource.com/c/tools/+/652595
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>

Author: adonovan

gopls/doc/settings.md

@@ -355,6 +355,30 @@ These analyses are documented on
 
 Default: `false`.
 
+<a id='annotations'></a>
+### `annotations map[enum]bool`
+
+annotations specifies the various kinds of compiler
+optimization details that should be reported as diagnostics
+when enabled for a package by the "Toggle compiler
+optimization details" (`gopls.gc_details`) command.
+
+(Some users care only about one kind of annotation in their
+profiling efforts. More importantly, in large packages, the
+number of annotations can sometimes overwhelm the user
+interface and exceed the per-file diagnostic limit.)
+
+TODO(adonovan): rename this field to CompilerOptDetail.
+
+Each enum must be one of:
+
+* `"bounds"` controls bounds checking diagnostics.
+* `"escape"` controls diagnostics about escape choices.
+* `"inline"` controls diagnostics about inlining choices.
+* `"nil"` controls nil checks.
+
+Default: `{"bounds":true,"escape":true,"inline":true,"nil":true}`.
+
 <a id='vulncheck'></a>
 ### `vulncheck enum`
 

gopls/internal/doc/api.json

@@ -689,6 +689,41 @@
 				"Hierarchy": "ui.diagnostic",
 				"DeprecationMessage": ""
 			},
+			{
+				"Name": "annotations",
+				"Type": "map[enum]bool",
+				"Doc": "annotations specifies the various kinds of compiler\noptimization details that should be reported as diagnostics\nwhen enabled for a package by the \"Toggle compiler\noptimization details\" (`gopls.gc_details`) command.\n\n(Some users care only about one kind of annotation in their\nprofiling efforts. More importantly, in large packages, the\nnumber of annotations can sometimes overwhelm the user\ninterface and exceed the per-file diagnostic limit.)\n\nTODO(adonovan): rename this field to CompilerOptDetail.\n",
+				"EnumKeys": {
+					"ValueType": "bool",
+					"Keys": [
+						{
+							"Name": "\"bounds\"",
+							"Doc": "`\"bounds\"` controls bounds checking diagnostics.\n",
+							"Default": "true"
+						},
+						{
+							"Name": "\"escape\"",
+							"Doc": "`\"escape\"` controls diagnostics about escape choices.\n",
+							"Default": "true"
+						},
+						{
+							"Name": "\"inline\"",
+							"Doc": "`\"inline\"` controls diagnostics about inlining choices.\n",
+							"Default": "true"
+						},
+						{
+							"Name": "\"nil\"",
+							"Doc": "`\"nil\"` controls nil checks.\n",
+							"Default": "true"
+						}
+					]
+				},
+				"EnumValues": null,
+				"Default": "{\"bounds\":true,\"escape\":true,\"inline\":true,\"nil\":true}",
+				"Status": "",
+				"Hierarchy": "ui.diagnostic",
+				"DeprecationMessage": ""
+			},
 			{
 				"Name": "vulncheck",
 				"Type": "enum",

gopls/internal/golang/compileropt.go

@@ -16,6 +16,7 @@ import (
 
 	"golang.org/x/tools/gopls/internal/cache"
 	"golang.org/x/tools/gopls/internal/protocol"
+	"golang.org/x/tools/gopls/internal/settings"
 	"golang.org/x/tools/internal/event"
 )
 
@@ -65,7 +66,7 @@ func CompilerOptDetails(ctx context.Context, snapshot *cache.Snapshot, pkgDir pr
 	reports := make(map[protocol.DocumentURI][]*cache.Diagnostic)
 	var parseError error
 	for _, fn := range files {
-		uri, diagnostics, err := parseDetailsFile(fn)
+		uri, diagnostics, err := parseDetailsFile(fn, snapshot.Options())
 		if err != nil {
 			// expect errors for all the files, save 1
 			parseError = err
@@ -87,7 +88,7 @@ func CompilerOptDetails(ctx context.Context, snapshot *cache.Snapshot, pkgDir pr
 }
 
 // parseDetailsFile parses the file written by the Go compiler which contains a JSON-encoded protocol.Diagnostic.
-func parseDetailsFile(filename string) (protocol.DocumentURI, []*cache.Diagnostic, error) {
+func parseDetailsFile(filename string, options *settings.Options) (protocol.DocumentURI, []*cache.Diagnostic, error) {
 	buf, err := os.ReadFile(filename)
 	if err != nil {
 		return "", nil, err
@@ -118,30 +119,14 @@ func parseDetailsFile(filename string) (protocol.DocumentURI, []*cache.Diagnosti
 		if err := dec.Decode(d); err != nil {
 			return "", nil, err
 		}
-		if d.Source != "go compiler" {
-			continue
-		}
 		d.Tags = []protocol.DiagnosticTag{} // must be an actual slice
 		msg := d.Code.(string)
 		if msg != "" {
-			// Typical message prefixes gathered by grepping the source of
-			// cmd/compile for literal arguments in calls to logopt.LogOpt.
-			// (It is not a well defined set.)
-			//
-			// - canInlineFunction
-			// - cannotInlineCall
-			// - cannotInlineFunction
-			// - copy
-			// - escape
-			// - escapes
-			// - isInBounds
-			// - isSliceInBounds
-			// - iteration-variable-to-{heap,stack}
-			// - leak
-			// - loop-modified-{range,for}
-			// - nilcheck
 			msg = fmt.Sprintf("%s(%s)", msg, d.Message)
 		}
+		if !showDiagnostic(msg, d.Source, options) {
+			continue
+		}
 
 		// zeroIndexedRange subtracts 1 from the line and
 		// range, because the compiler output neglects to
@@ -186,6 +171,51 @@ func parseDetailsFile(filename string) (protocol.DocumentURI, []*cache.Diagnosti
 	return uri, diagnostics, nil
 }
 
+// showDiagnostic reports whether a given diagnostic should be shown to the end
+// user, given the current options.
+func showDiagnostic(msg, source string, o *settings.Options) bool {
+	if source != "go compiler" {
+		return false
+	}
+	if o.Annotations == nil {
+		return true
+	}
+
+	// The strings below were gathered by grepping the source of
+	// cmd/compile for literal arguments in calls to logopt.LogOpt.
+	// (It is not a well defined set.)
+	//
+	// - canInlineFunction
+	// - cannotInlineCall
+	// - cannotInlineFunction
+	// - escape
+	// - escapes
+	// - isInBounds
+	// - isSliceInBounds
+	// - leak
+	// - nilcheck
+	//
+	// Additional ones not handled by logic below:
+	// - copy
+	// - iteration-variable-to-{heap,stack}
+	// - loop-modified-{range,for}
+
+	switch {
+	case strings.HasPrefix(msg, "canInline") ||
+		strings.HasPrefix(msg, "cannotInline") ||
+		strings.HasPrefix(msg, "inlineCall"):
+		return o.Annotations[settings.Inline]
+	case strings.HasPrefix(msg, "escape") || msg == "leak":
+		return o.Annotations[settings.Escape]
+	case strings.HasPrefix(msg, "nilcheck"):
+		return o.Annotations[settings.Nil]
+	case strings.HasPrefix(msg, "isInBounds") ||
+		strings.HasPrefix(msg, "isSliceInBounds"):
+		return o.Annotations[settings.Bounds]
+	}
+	return false
+}
+
 func findJSONFiles(dir string) ([]string, error) {
 	ans := []string{}
 	f := func(path string, fi os.FileInfo, _ error) error {

gopls/internal/settings/default.go

@@ -91,6 +91,12 @@ func DefaultOptions(overrides ...func(*Options)) *Options {
 				},
 				UIOptions: UIOptions{
 					DiagnosticOptions: DiagnosticOptions{
+						Annotations: map[Annotation]bool{
+							Bounds: true,
+							Escape: true,
+							Inline: true,
+							Nil:    true,
+						},
 						Vulncheck:                 ModeVulncheckOff,
 						DiagnosticsDelay:          1 * time.Second,
 						DiagnosticsTrigger:        DiagnosticsOnEdit,

gopls/internal/settings/settings.go

@@ -18,6 +18,23 @@ import (
 	"golang.org/x/tools/gopls/internal/util/frob"
 )
 
+// An Annotation is a category of Go compiler optimization diagnostic.
+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"
+)
+
 // Options holds various configuration that affects Gopls execution, organized
 // by the nature or origin of the settings.
 //
@@ -436,6 +453,19 @@ type DiagnosticOptions struct {
 	// [Staticcheck's website](https://staticcheck.io/docs/checks/).
 	Staticcheck bool `status:"experimental"`
 
+	// Annotations specifies the various kinds of compiler
+	// optimization details that should be reported as diagnostics
+	// when enabled for a package by the "Toggle compiler
+	// optimization details" (`gopls.gc_details`) command.
+	//
+	// (Some users care only about one kind of annotation in their
+	// profiling efforts. More importantly, in large packages, the
+	// number of annotations can sometimes overwhelm the user
+	// interface and exceed the per-file diagnostic limit.)
+	//
+	// TODO(adonovan): rename this field to CompilerOptDetail.
+	Annotations map[Annotation]bool
+
 	// Vulncheck enables vulnerability scanning.
 	Vulncheck VulncheckMode `status:"experimental"`
 
@@ -1124,7 +1154,7 @@ func (o *Options) setOne(name string, value any) (applied []CounterPath, _ error
 		return setBoolMap(&o.Hints, value)
 
 	case "annotations":
-		return nil, &SoftError{"the 'annotations' setting was removed in gopls/v0.18.0; all compiler optimization details are now shown"}
+		return setAnnotationMap(&o.Annotations, value)
 
 	case "vulncheck":
 		return setEnum(&o.Vulncheck, value,
@@ -1420,6 +1450,51 @@ func setDuration(dest *time.Duration, value any) error {
 	return nil
 }
 
+func setAnnotationMap(dest *map[Annotation]bool, value any) ([]CounterPath, error) {
+	all, err := asBoolMap[string](value)
+	if err != nil {
+		return nil, err
+	}
+	var counters []CounterPath
+	// Default to everything enabled by default.
+	m := make(map[Annotation]bool)
+	for k, enabled := range all {
+		var a Annotation
+		cnts, err := setEnum(&a, k,
+			Nil,
+			Escape,
+			Inline,
+			Bounds)
+		if err != nil {
+			// In case of an error, process any legacy values.
+			switch k {
+			case "noEscape":
+				m[Escape] = false
+				return nil, fmt.Errorf(`"noEscape" is deprecated, set "Escape: false" instead`)
+
+			case "noNilcheck":
+				m[Nil] = false
+				return nil, fmt.Errorf(`"noNilcheck" is deprecated, set "Nil: false" instead`)
+
+			case "noInline":
+				m[Inline] = false
+				return nil, fmt.Errorf(`"noInline" is deprecated, set "Inline: false" instead`)
+
+			case "noBounds":
+				m[Bounds] = false
+				return nil, fmt.Errorf(`"noBounds" is deprecated, set "Bounds: false" instead`)
+
+			default:
+				return nil, err
+			}
+		}
+		counters = append(counters, cnts...)
+		m[a] = enabled
+	}
+	*dest = m
+	return counters, nil
+}
+
 func setBoolMap[K ~string](dest *map[K]bool, value any) ([]CounterPath, error) {
 	m, err := asBoolMap[K](value)
 	if err != nil {

gopls/internal/settings/settings_test.go

@@ -180,6 +180,17 @@ func TestOptions_Set(t *testing.T) {
 				return len(o.DirectoryFilters) == 0
 			},
 		},
+		{
+			name: "annotations",
+			value: map[string]any{
+				"Nil":      false,
+				"noBounds": true,
+			},
+			wantError: true,
+			check: func(o Options) bool {
+				return !o.Annotations[Nil] && !o.Annotations[Bounds]
+			},
+		},
 		{
 			name:      "vulncheck",
 			value:     []any{"invalid"},