Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/gopls: filter noisy code actions when triggerKind is automatic #65167

Open
hyangah opened this issue Jan 19, 2024 · 8 comments
Open

x/tools/gopls: filter noisy code actions when triggerKind is automatic #65167

hyangah opened this issue Jan 19, 2024 · 8 comments
Labels
gopls/analysis Issues related to running analysis in gopls gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@hyangah
Copy link
Contributor

hyangah commented Jan 19, 2024

gopls version: built @ c467be361ab7f9641e4ffefaa23e0eedfb7ea10e

What did I do?

Triggered printf diagnostics and SA1006 diagnostics

	fmt.Fprintln(os.Stderr, "run uploading\n")

Settings:

    "gopls": {
        "ui.diagnostic.staticcheck": true,
    },
    "go.languageServerFlags": ["-rpc.trace"]

What did you expect

Gopls detects the issue and provides quick fixes for these diagnostics.

What did you see instead

Gopls detected the issues, but code actions are not available from the editor (neither with the light bulb nor the Quick Fix hover)

  • Light bulb on editor and Hover on editor - don't work.
    Screenshot 2024-01-19 at 10 07 13 AM
    Screenshot 2024-01-19 at 10 23 46 AM

  • Light bulb on PROBLEMS panel - sort of worked
    Screenshot 2024-01-19 at 10 06 57 AM

  • Lightbulb for SA1006 worked (except the duplicated code actions).
  • Lightbulb for printf analysis is not present.

Traces

It looks like VS Code triggers code actions in different ways.
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_codeAction

  1. Light bulb on editor

VS Code sent a request asking for quick fixes for detected problems. (See "context" filled with the diagnostic&its source, and triggerKind:2 (TriggerKind: "Automatic"). Gopls returned "inline call to Fprintln" code action, which doesn't make sense.

[Trace - 10:28:39.053 AM] Sending request 'textDocument/codeAction - (93)'.
Params: {"textDocument":{"uri":"file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go"},"range":{"start":{"line":291,"character":1},"end":{"line":291,"character":1}},"context":{"diagnostics":[{"range":{"start":{"line":291,"character":1},"end":{"line":291,"character":43}},"message":"fmt.Fprintln arg list ends with redundant newline","code":"default","codeDescription":{"href":"https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/printf"},"severity":2,"source":"printf"}],"triggerKind":2}}


[Trace - 10:28:39.055 AM] Received response 'textDocument/codeAction - (93)' in 1ms.
Result: [{"title":"Inline call to Fprintln","kind":"refactor.inline","command":{"title":"Inline call to Fprintln","command":"gopls.apply_fix","arguments":[{"Fix":"inline_call","URI":"file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go","Range":{"start":{"line":291,"character":1},"end":{"line":291,"character":1}},"ResolveEdits":false}]}}]

Note that VS Code asked quick fixes for "printf" analyzer, but not for SA1006 analyzer's report. This can be a client-side issue and I will investigate it separately.

  1. Hover on editor

VS Code sent the request asking for quick fixes for detected problems. (See "context" filled with the diagnostic&its source, and triggerKind:1 (TriggerKind "Invoked"). Gopls returned nothing.

[Trace - 10:31:05.080 AM] Sending request 'textDocument/codeAction - (97)'.
Params: {"textDocument":{"uri":"file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go"},"range":{"start":{"line":291,"character":1},"end":{"line":291,"character":43}},"context":{"diagnostics":[{"range":{"start":{"line":291,"character":1},"end":{"line":291,"character":43}},"message":"fmt.Fprintln arg list ends with redundant newline","code":"default","codeDescription":{"href":"https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/printf"},"severity":2,"source":"printf"}],"only":["quickfix"],"triggerKind":1}}


[Trace - 10:31:05.081 AM] Received response 'textDocument/codeAction - (97)' in 0ms.
Result: null
  1. Light bulb on PROBLEMS panel

VS Code sent request with "quickfix" and "triggerKind: 1". Gopls returned some code actions for SA1006 diagnostic (but with duplications)

[Trace - 10:46:11.180 AM] Sending request 'textDocument/codeAction - (125)'.
Params: {"textDocument":{"uri":"file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go"},"range":{"start":{"line":162,"character":1},"end":{"line":162,"character":53}},"context":{"diagnostics":[{"range":{"start":{"line":162,"character":1},"end":{"line":162,"character":53}},"message":"printf-style function with dynamic format string and no further arguments should use print-style function instead","code":"default","severity":2,"source":"SA1006"}],"only":["quickfix"],"triggerKind":1}}


[Trace - 10:46:11.181 AM] Received response 'textDocument/codeAction - (125)' in 0ms.
Result: [{"title":"use fmt.Fprint instead of fmt.Fprintf","kind":"quickfix","diagnostics":[{"range":{"start":{"line":162,"character":1},"end":{"line":162,"character":53}},"severity":2,"code":"default","source":"SA1006","message":"printf-style function with dynamic format string and no further arguments should use print-style function instead"}],"edit":{"documentChanges":[{"textDocument":{"version":407,"uri":"file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go"},"edits":[{"range":{"start":{"line":162,"character":1},"end":{"line":162,"character":12}},"newText":"fmt.Fprint"}]}]}},{"title":"use fmt.Fprint instead of fmt.Fprintf","kind":"quickfix","diagnostics":[{"range":{"start":{"line":162,"character":1},"end":{"line":162,"character":53}},"severity":2,"code":"default","source":"SA1006","message":"printf-style function with dynamic format string and no further arguments should use print-style function instead"}],"edit":{"documentChanges":[{"textDocument":{"version":407,"uri":"file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go"},"edits":[{"range":{"start":{"line":162,"character":1},"end":{"line":162,"character":12}},"newText":"fmt.Fprint"}]}]}}]

But didn't return codeactions for printf diagnostic.

[Trace - 10:47:04.164 AM] Sending request 'textDocument/codeAction - (126)'.
Params: {"textDocument":{"uri":"file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go"},"range":{"start":{"line":291,"character":1},"end":{"line":291,"character":43}},"context":{"diagnostics":[{"range":{"start":{"line":291,"character":1},"end":{"line":291,"character":43}},"message":"fmt.Fprintln arg list ends with redundant newline","code":"default","codeDescription":{"href":"https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/printf"},"severity":2,"source":"printf"}],"only":["quickfix"],"triggerKind":1}}


[Trace - 10:47:04.165 AM] Received response 'textDocument/codeAction - (126)' in 1ms.
Result: null
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jan 19, 2024
@gopherbot gopherbot added this to the Unreleased milestone Jan 19, 2024
@hyangah
Copy link
Contributor Author

hyangah commented Jan 19, 2024

The behavior is non-deterministic. Sometimes for the same request (case 1 Light bulb on editor), gopls returns the quickfix for printf analyzer. During the same session (without editing...):

[Trace - 11:03:34.199 AM] Sending request 'textDocument/codeAction - (62)'.
Params: {"textDocument":{"uri":"file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go"},"range":{"start":{"line":291,"character":26},"end":{"line":291,"character":26}},"context":{"diagnostics":[{"range":{"start":{"line":291,"character":1},"end":{"line":291,"character":43}},"message":"fmt.Fprintln arg list ends with redundant newline","code":"default","codeDescription":{"href":"https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/printf"},"severity":2,"source":"printf"}],"triggerKind":2}}


[Trace - 11:03:34.202 AM] Received response 'textDocument/codeAction - (62)' in 2ms.
Result: [{"title":"Convert to raw string literal","kind":"refactor.rewrite","edit":{"documentChanges":[{"textDocument":{"version":1,"uri":"file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go"},"edits":[{"range":{"start":{"line":291,"character":25},"end":{"line":291,"character":42}},"newText":"`run uploading\n`"}]}]}},{"title":"Inline call to Fprintln","kind":"refactor.inline","command":{"title":"Inline call to Fprintln","command":"gopls.apply_fix","arguments":[{"Fix":"inline_call","URI":"file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go","Range":{"start":{"line":291,"character":26},"end":{"line":291,"character":26}},"ResolveEdits":false}]}}]

vs

[Trace - 11:10:26.252 AM] Sending request 'textDocument/codeAction - (156)'.
Params: {"textDocument":{"uri":"file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go"},"range":{"start":{"line":296,"character":4},"end":{"line":296,"character":4}},"context":{"diagnostics":[{"range":{"start":{"line":296,"character":1},"end":{"line":296,"character":48}},"message":"fmt.Fprintln arg list ends with redundant newline","code":"default","codeDescription":{"href":"https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/printf"},"severity":2,"source":"printf"}],"triggerKind":2}}


[Trace - 11:10:26.254 AM] Received response 'textDocument/codeAction - (156)' in 2ms.
Result: [{"title":"Inline call to Fprintln","kind":"refactor.inline","command":{"title":"Inline call to Fprintln","command":"gopls.apply_fix","arguments":[{"Fix":"inline_call","URI":"file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go","Range":{"start":{"line":296,"character":4},"end":{"line":296,"character":4}},"ResolveEdits":false}]}}]

@hyangah hyangah added the gopls/analysis Issues related to running analysis in gopls label Jan 19, 2024
@findleyr findleyr modified the milestones: Unreleased, gopls/v0.15.0 Jan 19, 2024
@findleyr
Copy link
Contributor

It looks like gopls doesn't return 'inline' when the request specifies "only":["quickfix"]. So it looks like the behavior is in fact deterministic.

Is the bug that VS Code should specify "quickfix"? Or should gopls not return inline when the action is automatic.

@findleyr findleyr changed the title x/tools/gopls: code action for diagnostics do not include useful fixes x/tools/gopls: filter noisy code actions when triggerKind is automatic Jan 19, 2024
@findleyr
Copy link
Contributor

Per discussion: the problem is simply that "inline" is often not useful, and so therefore should be filtered when the triggerKind is automatic.

@findleyr
Copy link
Contributor

We should do this filtering for gopls@v0.15.0, since the inline lightbulb is distracting.

@adonovan do you want to do this, since you're currently working in the codeaction codebase?

@adonovan
Copy link
Member

adonovan commented Jan 19, 2024

Sure, I can take this.

Selecting a statement triggers the "extract" lightbulb. Should it be treated similarly?
Or is inline special because it triggers any time the cursor is inside a function call, which is common?
Similarly, placing the cursor in a string literal (again, common) triggers the "flip quotes" action.

Should all of these actions appear only on the Refactor menu (no lightbulb)?
Or should the ones currently triggered by cursor position (no selection required) be so relegated?
Or should CodeAction stop offering commands based on mere cursor placement and require a selection (of a complete or partial string literal, or a function call, respectively)?
I'm not really sure what the ideal VS Code UX is here.

@findleyr
Copy link
Contributor

I'll defer to @hyangah for what the VS Code UI should do, but it seems reasonable that if the region is a single point and trigger is automatic, there is no user intent.

@gopherbot
Copy link

Change https://go.dev/cl/556555 mentions this issue: gopls: consolidate analyzers, eliminating "convenience" analyzers

gopherbot pushed a commit to golang/tools that referenced this issue Jan 22, 2024
Make several mechanical clean-ups to analyzer implementations, reducing
the complexity of their configuration and normalizing behavior.

 - Remove the "type error" analyzer category, since it is now an unused
   distinction.
 - Make "stubmethods" an ordinary analyzer, since it no longer needs
   special logic in the code action handler.
 - Revert infertypeargs to be an ordinary analyzer (golang/go#63821),
   but make its severity "hint", so that it doesn't show up in the
   problems tab.
 - Remove the "convenience" analyzer category, which only existed for
   the enabled/disabled configuration. The only effect of this is that
   the "fillstruct" analyzer can no longer be disabled, but since code
   actions should not be disruptive (golang/go#65167), it should be OK
   to have them all available. Notably, we don't allow disabling other
   code actions.

Fixes golang/go#63821
Fixes golang/go#61559

Change-Id: I2a1cd01ea331c26fdbb35d6b5c97562c2a1f9240
Reviewed-on: https://go-review.googlesource.com/c/tools/+/556555
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
@findleyr
Copy link
Contributor

findleyr commented Feb 6, 2024

I don't think this is going to make the v0.15.0 release. It's OK to move it to v0.16.0.

@findleyr findleyr modified the milestones: gopls/v0.15.0, gopls/v0.16.0 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls/analysis Issues related to running analysis in gopls gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants