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/go/analysis: -fix creates unnecessary import decls #71647

Closed
adonovan opened this issue Feb 10, 2025 · 2 comments
Closed

x/tools/go/analysis: -fix creates unnecessary import decls #71647

adonovan opened this issue Feb 10, 2025 · 2 comments
Assignees
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsFix The path to resolution is known, but the work has not been done. Refactoring Issues related to refactoring tools
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Feb 10, 2025

Here is a fairly typical diff hunk from an analyzer, as applied by the -fix flag. Observe that it creates a new import decl, even though there's an existing one that it could add to. The original patch was computed by analysisinternal.AddImport, but applying gofmt (as -fix does) didn't smooth the two declarations into one; you need goimports for that.

--- /Users/adonovan/w/xtools/playground/socket/socket.go (old)
+++ /Users/adonovan/w/xtools/playground/socket/socket.go (new)
@@ -14,7 +14,9 @@
 // This will not run on App Engine as WebSockets are not supported there.
 package socket // import "[golang.org/x/tools/playground/socket](http://golang.org/x/tools/playground/socket)"
 
-import (
+import "slices"
+
+import (
     "bytes"
     "encoding/json"
     "errors"

AddImport could do a better job here. Perhaps it could always insert the new imports inside the top of any existing import decl; this may improve the final result, especially for std imports, which will be sorted. However, it may not be better (though nor is it worse) for non-std imports, which often appear in a second or later group separate by a blank line. In any case, there is room for improvement.

cc: @jba

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 10, 2025
@gopherbot gopherbot added this to the Unreleased milestone Feb 10, 2025
@adonovan adonovan added Refactoring Issues related to refactoring tools Analysis Issues related to static analysis (vet, x/tools/go/analysis) and removed Tools This label describes issues relating to any tools in the x/tools repository. labels Feb 10, 2025
@gabyhelp
Copy link

Related Issues

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gabyhelp gabyhelp added the ToolProposal Issues describing a requested change to a Go tool or command-line program. label Feb 10, 2025
@jba jba removed the ToolProposal Issues describing a requested change to a Go tool or command-line program. label Feb 10, 2025
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Feb 10, 2025
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/648136 mentions this issue: internal/analysisinternal: AddImport puts new import in a group

gopherbot pushed a commit to golang/tools that referenced this issue Feb 10, 2025
If AddImport needs to add a new import, and the file's first
declaration is a grouped import, then add it to that import.

This is one step towards a full implementation of the issue below,
and perhaps is good enough.

For golang/go#71647.

Change-Id: I8327b07c21c3efbd189c519e51c339b7aa4751d8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/648136
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Jonathan Amsterdam <jba@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsFix The path to resolution is known, but the work has not been done. Refactoring Issues related to refactoring tools
Projects
None yet
Development

No branches or pull requests

5 participants