Skip to content

Commit

Permalink
internal/settings: drop "annotations" setting
Browse files Browse the repository at this point in the history 
This obscure suboption of the already obscure Toggle GC Details
feature seems unnecessary. We should just show all the details.

Change-Id: I9deff8f317c7a97fe01af1547bf022a506c37f80
Reviewed-on: https://go-review.googlesource.com/c/tools/+/639835
Reviewed-by: Robert Findley <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
  • Loading branch information
@adonovan @gopherbot
adonovan authored and gopherbot committed Jan 3, 2025
1 parent c61a2b0 commit 5fe60fd
Show file tree
Hide file tree
Showing 6 changed files with 22 additions and 191 deletions.
19 changes: 0 additions & 19 deletions gopls/doc/settings.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,25 +315,6 @@ These analyses are documented on

Default: `false`.

<a id='annotations'></a>
### `annotations map[enum]bool`

**This setting is experimental and may be deleted.**

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.

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`

Expand Down
34 changes: 0 additions & 34 deletions gopls/internal/doc/api.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -635,40 +635,6 @@
"Status": "experimental",
"Hierarchy": "ui.diagnostic"
},
{
"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",
"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": "experimental",
"Hierarchy": "ui.diagnostic"
},
{
"Name": "vulncheck",
"Type": "enum",
Expand Down
73 changes: 21 additions & 52 deletions gopls/internal/golang/compileropt.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"golang.org/x/tools/gopls/internal/cache"
"golang.org/x/tools/gopls/internal/cache/metadata"
"golang.org/x/tools/gopls/internal/protocol"
"golang.org/x/tools/gopls/internal/settings"
"golang.org/x/tools/internal/event"
)

Expand Down Expand Up @@ -69,10 +68,9 @@ func CompilerOptDetails(ctx context.Context, snapshot *cache.Snapshot, mp *metad
return nil, err
}
reports := make(map[protocol.DocumentURI][]*cache.Diagnostic)
opts := snapshot.Options()
var parseError error
for _, fn := range files {
uri, diagnostics, err := parseDetailsFile(fn, opts)
uri, diagnostics, err := parseDetailsFile(fn)
if err != nil {
// expect errors for all the files, save 1
parseError = err
Expand All @@ -93,7 +91,7 @@ func CompilerOptDetails(ctx context.Context, snapshot *cache.Snapshot, mp *metad
}

// parseDetailsFile parses the file written by the Go compiler which contains a JSON-encoded protocol.Diagnostic.
func parseDetailsFile(filename string, options *settings.Options) (protocol.DocumentURI, []*cache.Diagnostic, error) {
func parseDetailsFile(filename string) (protocol.DocumentURI, []*cache.Diagnostic, error) {
buf, err := os.ReadFile(filename)
if err != nil {
return "", nil, err
Expand Down Expand Up @@ -124,14 +122,30 @@ func parseDetailsFile(filename string, options *settings.Options) (protocol.Docu
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
Expand Down Expand Up @@ -176,51 +190,6 @@ func parseDetailsFile(filename string, options *settings.Options) (protocol.Docu
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 {
Expand Down
6 changes: 0 additions & 6 deletions gopls/internal/settings/default.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,12 +89,6 @@ 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,
Expand Down
70 changes: 1 addition & 69 deletions gopls/internal/settings/settings.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,26 +16,6 @@ import (
"golang.org/x/tools/gopls/internal/util/frob"
)

// An Annotation is a category of Go compiler optimization diagnostic.
//
// TODO(adonovan): this seems like a large control surface. Let's
// remove it, and just show all kinds when the flag is enabled.
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.
//
Expand Down Expand Up @@ -426,12 +406,6 @@ 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.
Annotations map[Annotation]bool `status:"experimental"`

// Vulncheck enables vulnerability scanning.
Vulncheck VulncheckMode `status:"experimental"`

Expand Down Expand Up @@ -1043,7 +1017,7 @@ func (o *Options) setOne(name string, value any) error {
return setBoolMap(&o.Hints, value)

case "annotations":
return setAnnotationMap(&o.Annotations, value)
return deprecatedError("the 'annotations' setting was removed in gopls/v0.18.0; all compiler optimization details are now shown")

case "vulncheck":
return setEnum(&o.Vulncheck, value,
Expand Down Expand Up @@ -1299,48 +1273,6 @@ func setDuration(dest *time.Duration, value any) error {
return nil
}

func setAnnotationMap(dest *map[Annotation]bool, value any) error {
all, err := asBoolMap[string](value)
if err != nil {
return err
}
if all == nil {
return nil
}
// Default to everything enabled by default.
m := make(map[Annotation]bool)
for k, enabled := range all {
var a Annotation
if err := setEnum(&a, k,
Nil,
Escape,
Inline,
Bounds); err != nil {
// In case of an error, process any legacy values.
switch k {
case "noEscape":
m[Escape] = false
return fmt.Errorf(`"noEscape" is deprecated, set "Escape: false" instead`)
case "noNilcheck":
m[Nil] = false
return fmt.Errorf(`"noNilcheck" is deprecated, set "Nil: false" instead`)

case "noInline":
m[Inline] = false
return fmt.Errorf(`"noInline" is deprecated, set "Inline: false" instead`)
case "noBounds":
m[Bounds] = false
return fmt.Errorf(`"noBounds" is deprecated, set "Bounds: false" instead`)
default:
return err
}
}
m[a] = enabled
}
*dest = m
return nil
}

func setBoolMap[K ~string](dest *map[K]bool, value any) error {
m, err := asBoolMap[K](value)
if err != nil {
Expand Down
11 changes: 0 additions & 11 deletions gopls/internal/settings/settings_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,17 +180,6 @@ 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"},
Expand Down

0 comments on commit 5fe60fd

Please sign in to comment.