[tools] gopls/internal/cache: discard fixes that touch generated files
Alan Donovan (Gerrit)
Alan Donovan has uploaded the change for review![]( "Open in Gerrit")
Commit message
gopls/internal/cache: discard fixes that touch generated files
This is the gopls counterpart of CL 718505 to the other drivers.
+ integration test and marker test (long story).
Fixes golang/go#75948
Change diff
diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go
index 93df623..2b39fc9 100644
--- a/gopls/internal/cache/analysis.go
+++ b/gopls/internal/cache/analysis.go
@@ -1016,12 +1016,21 @@
factFilter[reflect.TypeOf(f)] = true
}
+ // Tabulate generated files.
+ fset := apkg.pkg.FileSet()
+ generated := make(map[*token.File]bool)
+ for _, file := range apkg.files {
+ if ast.IsGenerated(file) {
+ generated[fset.File(file.Pos())] = true
+ }
+ }
+
// Now run the (pkg, analyzer) action.
var diagnostics []gobDiagnostic
pass := &analysis.Pass{
Analyzer: analyzer,
- Fset: apkg.pkg.FileSet(),
+ Fset: fset,
Files: apkg.files,
OtherFiles: nil, // since gopls doesn't handle non-Go (e.g. asm) files
IgnoredFiles: nil, // zero-config gopls should analyze these files in another view
@@ -1036,10 +1045,21 @@
// ValidateFixes allows a fix.End to be slightly beyond
// EOF to avoid spurious assertions when reporting
// fixes as the end of truncated files; see #71659.
- if err := driverutil.ValidateFixes(apkg.pkg.FileSet(), analyzer, d.SuggestedFixes); err != nil {
+ if err := driverutil.ValidateFixes(fset, analyzer, d.SuggestedFixes); err != nil {
bug.Reportf("invalid SuggestedFixes: %v", err)
d.SuggestedFixes = nil
}
+
+ // Discard fixes that include edits to generated files (#75948).
+ d.SuggestedFixes = slices.DeleteFunc(d.SuggestedFixes, func(fix analysis.SuggestedFix) bool {
+ for _, edit := range fix.TextEdits {
+ if generated[fset.File(edit.Pos)] {
+ return true
+ }
+ }
+ return false
+ })
+
diagnostic, err := toGobDiagnostic(apkg.pkg, analyzer, d)
if err != nil {
// Don't bug.Report here: these errors all originate in
diff --git a/gopls/internal/cache/diagnostics.go b/gopls/internal/cache/diagnostics.go
index 05ff75c..5c8284f 100644
--- a/gopls/internal/cache/diagnostics.go
+++ b/gopls/internal/cache/diagnostics.go
@@ -96,9 +96,11 @@
return hash
}
+// ToProtocolDiagnostics converts Diagnostics to LSP form.
+// Beware: only command-based fixes are preserved; edit-based fixes are discarded!
func ToProtocolDiagnostics(diagnostics ...*Diagnostic) []protocol.Diagnostic {
// TODO(rfindley): support bundling edits, and bundle all suggested fixes here.
- // (see cache.bundleLazyFixes).
+ // (see [bundleLazyFixes]).
reports := []protocol.Diagnostic{}
for _, diag := range diagnostics {
diff --git a/gopls/internal/test/integration/misc/fix_test.go b/gopls/internal/test/integration/misc/fix_test.go
index 261b584..442b574 100644
--- a/gopls/internal/test/integration/misc/fix_test.go
+++ b/gopls/internal/test/integration/misc/fix_test.go
@@ -161,3 +161,55 @@
// yay, no panic
})
}
+
+func TestNoFixesInGeneratedFiles(t *testing.T) {
+ // (see issue #75948)
+ const src = `
+-- go.mod --
+module example.com
+go 1.21
+
+-- a/a1.go --
+package a
+
+var _ interface{}
+
+-- a/a2.go --
+// Code generated by hand. DO NOT EDIT.
+
+package a
+
+var _ interface{}
+`
+ Run(t, src, func(t *testing.T, env *Env) {
+ for _, test := range []struct {
+ filename string
+ wantFixes int
+ }{
+ {"a/a1.go", 1},
+ {"a/a2.go", 0},
+ } {
+ env.OpenFile(test.filename)
+
+ // Expect a diagnostic from the 'any' modernizer.
+ var d protocol.PublishDiagnosticsParams
+ env.AfterChange(
+ Diagnostics(
+ env.AtRegexp(test.filename, `interface{}`),
+ WithMessage("interface{} can be replaced by any")),
+ ReadDiagnostics(test.filename, &d),
+ )
+
+ // The diagnostic should carry a fix unless the file is generated.
+ var quickFixes []*protocol.CodeAction
+ for _, act := range env.CodeActionForFile(test.filename, d.Diagnostics) {
+ if act.Kind == protocol.QuickFix {
+ quickFixes = append(quickFixes, &act)
+ }
+ }
+ if len(quickFixes) != test.wantFixes {
+ t.Fatalf("got %d quickfixes, want %d", len(quickFixes), test.wantFixes)
+ }
+ }
+ })
+}
Change information
- M gopls/internal/cache/analysis.go
- M gopls/internal/cache/diagnostics.go
- M gopls/internal/test/integration/misc/fix_test.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan voted Auto-Submit+1![]( "Open in Gerrit")
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 1 comment![]( "Open in Gerrit")
Sorry, I didn't understand the goal when we discussed earlier.
Why do we want to discard *fixes* in an interactive environment? Doesn't the user ultimately decide whether to apply a fix?
I thought we'd suppress the analyses altogether.
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
Sorry, I didn't understand the goal when we discussed earlier.
Why do we want to discard *fixes* in an interactive environment? Doesn't the user ultimately decide whether to apply a fix?
I thought we'd suppress the analyses altogether.
Why do we want to discard fixes in an interactive environment?
Doesn't the user ultimately decide whether to apply a fix?
Yes, the user decides, so we could leave things as is, though it may be a distracting nuisance to be offered fixes in generated code. There may also be some marginal value in consistency across drivers, but I agree that the interactive setting is potentially significantly different.
> I thought we'd suppress the analyses altogether.
Do you here mean: suppress these analyzer's diagnostics in generated files? There may be value in knowing that the generated code is suboptimal. Of course, the action in that case is not to apply the fix, but to fix the generator. If the diagnostic is only hint-level, it may not be worth the effort, but that's why hint-level diagnostics are pretty discrete even for ordinary hand-written source.
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robert Findley (Gerrit)
Robert Findley added 1 comment![]( "Open in Gerrit")
Alan DonovanSorry, I didn't understand the goal when we discussed earlier.
Why do we want to discard *fixes* in an interactive environment? Doesn't the user ultimately decide whether to apply a fix?
I thought we'd suppress the analyses altogether.
Why do we want to discard fixes in an interactive environment?
Doesn't the user ultimately decide whether to apply a fix?Yes, the user decides, so we could leave things as is, though it may be a distracting nuisance to be offered fixes in generated code. There may also be some marginal value in consistency across drivers, but I agree that the interactive setting is potentially significantly different.
> I thought we'd suppress the analyses altogether.Do you here mean: suppress these analyzer's diagnostics in generated files? There may be value in knowing that the generated code is suboptimal. Of course, the action in that case is not to apply the fix, but to fix the generator. If the diagnostic is only hint-level, it may not be worth the effort, but that's why hint-level diagnostics are pretty discrete even for ordinary hand-written source.
I don't see any value in suppressing the fix if we're showing the diagnostic. The fix is ~0 cost in the UI.
Perhaps we should suppress diagnostics in generated files unless they're open (IIRC, we already do this for 'hint' level diagnostics).
Related details
- Alan Donovan
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
Alan DonovanSorry, I didn't understand the goal when we discussed earlier.
Why do we want to discard *fixes* in an interactive environment? Doesn't the user ultimately decide whether to apply a fix?
I thought we'd suppress the analyses altogether.
Robert FindleyWhy do we want to discard fixes in an interactive environment?
Doesn't the user ultimately decide whether to apply a fix?Yes, the user decides, so we could leave things as is, though it may be a distracting nuisance to be offered fixes in generated code. There may also be some marginal value in consistency across drivers, but I agree that the interactive setting is potentially significantly different.
> I thought we'd suppress the analyses altogether.Do you here mean: suppress these analyzer's diagnostics in generated files? There may be value in knowing that the generated code is suboptimal. Of course, the action in that case is not to apply the fix, but to fix the generator. If the diagnostic is only hint-level, it may not be worth the effort, but that's why hint-level diagnostics are pretty discrete even for ordinary hand-written source.
I don't see any value in suppressing the fix if we're showing the diagnostic. The fix is ~0 cost in the UI.
Perhaps we should suppress diagnostics in generated files unless they're open (IIRC, we already do this for 'hint' level diagnostics).
I don't see any value in suppressing the fix if we're showing the diagnostic. The fix is ~0 cost in the UI.
I was vaguely thinking about what might happen if the CLI was used to apply a batch of fixes en masse, but that's not a supported interface.
Perhaps we should suppress diagnostics in generated files unless they're open.
This issue was about discarding fixes, not suppressing diagnostics. As you point out, we already make hint-level diagnostics very subtle, and I don't see any reason not to keep displaying them even in generated files (so that you can opt to fix the generator if you wish).
I'll revert the change.
Related details
- Robert Findley
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |