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: renaming an imported package name does not rename the imported package itself #68096

Open
Fumbibi opened this issue Jun 15, 2024 · 6 comments
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@Fumbibi
Copy link

Fumbibi commented Jun 15, 2024

Type: Bug

step 1, have a very simple application file app/mapper.go and a simple test file in the same package, but a different directory app_test/mapper_test.go
both have first line 'package app'

step 2, start fixing your file structure, create a new directory named 'mapper' and put mapper.go and mapper_test.go both in there.

step 3, change the package for mapper to 'package mapper' and mapper_test to 'package mapper_test'

step 4, right click the 'app' in rooms := &app.Generate()

step 5, left-click Rename Symbol

step 6, type in 'mapper' to match the package name properly

step 7, ctrl + s saves the file

Import list before step 7

import (
"data-algo/app"
"fmt"
"math/rand"
"testing"
"time"
)

after step 7

import (
mapper "data-algo/app"
"fmt"
"math/rand"
"testing"
"time"
)

I have many folders I'm fixing like this and I repeated this bug consistently

Extension version: 0.41.4
VS Code version: Code 1.90.1 (611f9bfce64f25108829dd295f54a6894e87339d, 2024-06-11T21:02:43.666Z)
OS version: Linux x64 6.6.25-01713-g2f237acc8e50
Modes:

System Info
Item Value
CPUs Intel(R) Celeron(R) N4500 @ 1.10GHz (2 x 0)
GPU Status 2d_canvas: enabled
canvas_oop_rasterization: disabled_off
direct_rendering_display_compositor: disabled_off_ok
gpu_compositing: enabled
multiple_raster_threads: disabled_off
opengl: enabled_on
rasterization: enabled
raw_draw: disabled_off_ok
skia_graphite: disabled_off
video_decode: enabled
video_encode: disabled_software
vulkan: disabled_off
webgl: enabled
webgl2: enabled
webgpu: disabled_off
Load (avg) 0, 1, 1
Memory (System) 2.68GB (1.94GB free)
Process Argv . --crash-reporter-id 3a1bcf46-b7c7-4601-b24a-48d4ebbe4fd8
Screen Reader no
VM 100%
DESKTOP_SESSION undefined
XDG_CURRENT_DESKTOP X-Generic
XDG_SESSION_DESKTOP undefined
XDG_SESSION_TYPE wayland
A/B Experiments
vsliv368:30146709
vspor879:30202332
vspor708:30202333
vspor363:30204092
vscorecescf:30445987
vscod805:30301674
binariesv615:30325510
vsaa593:30376534
py29gd2263:31024239
c4g48928:30535728
azure-dev_surveyone:30548225
a9j8j154:30646983
962ge761:30959799
pythongtdpath:30769146
welcomedialogc:30910334
pythonidxpt:30866567
pythonnoceb:30805159
asynctok:30898717
pythontestfixt:30902429
pythonregdiag2:30936856
pythonmypyd1:30879173
2e7ec940:31000449
pythontbext0:30879054
accentitlementst:30995554
dsvsc016:30899300
dsvsc017:30899301
dsvsc018:30899302
cppperfnew:31000557
dsvsc020:30976470
pythonait:31006305
jchc7451:31067544
chatpanelt:31048053
dsvsc021:30996838
g316j359:31013175
pythoncenvpt:31062603
a69g1124:31058053
dvdeprecation:31068756
pythonprt:31056678
dwnewjupyter:31046869
newcmakeconfigv2:31071590

@Fumbibi
Copy link
Author

Fumbibi commented Jun 15, 2024

another odd behavior along the same line was, I updated world := &bonus.World{} in main to world := &worldroller.World{} by typing it manually, and ctrl +s removed the import line for the old bonus package and and didn't add the one for the worldroller package. But then hit undo, selected bonus, started typing, saw predicted text "world roller" hit tab, and that updated the import perfectly correctly even before I saved.

When doing this in a file with more than one instance, it kept the old 'bad' import and added the new one on a new line, which makes sense because it was still referenced in the code.

update, further testing showed that when you right click the directory name in the import list and you click "rename symbol" it will populate the box so like
import "data/ops" displays "ops" in the box, and you can type "dllist" to replace it, and it will replace all the references except it just aliases the import which doesn't fix update the line to import "data/dllist" like one might expect.

further investigation reveals that if you try to delete the import to prevent the mistake, then you can't use "rename symbol" on the code because it no longer recognizes it.

@adonovan
Copy link
Member

adonovan commented Jun 20, 2024

There are three issues here. Let's call this one issue 1.

step 4, right click the 'app' in rooms := &app.Generate()

The symbol app is an imported package name, whose scope is just the current file. When you rename it, you are changing the name by which the package is imported into the current file, not the name of the package itself, so the rename tool is working as intended when it introduces a renaming import: import mapper "data-algo/app".

To rename a package, invoke the Rename operation while selecting the package app identifier at the top of the file.

@adonovan
Copy link
Member

adonovan commented Jun 20, 2024

Issue 2:

another odd behavior along the same line

I tried to reproduce this using the encoding/xml and encoding/json packages, which both define NewEncoder:

  • add var _ = xml.NewEncoder and save file. import "encoding/xml" appears.
  • replace "xml." with "json." and save file. The import changes to "encoding/json".
  • add var _ = xml. and start typing. Completions within xml package are observed, even though it is not imported.
  • hit tab to accept completion. import "encoding/xml" appears. (We now have both imports.) This is normal.
  • If instead of the previous two steps, we replace the existing var _ = json... with var _ = xml., then start typing NewEnc, accepting the completion adds an xml import but doesn't remove the json import that is no longer needed. This too is normal; redundant imports are not removed until the file is saved.

If you see something different, could you please give instructions for reproducing it with these standard library symbols? Ideally this should be a separate GitHub issue, since it is unrelated to the other two items.

@adonovan
Copy link
Member

adonovan commented Jun 20, 2024

Issue 3:

update, further testing showed that when you right click the directory name...

Correct, renaming an import declaration such as "fmt" is equivalent to renaming the local package name such as fmt.Println. It changes only the current file, not the imported package. To rename a package, open a source file in that package and rename the package declaration on the first line. This issue is a different manifestation of issue 1. The tool is working as intended.

further investigation reveals that if you try to delete the import to prevent the mistake, then you can't use "rename symbol" on the code because it no longer recognizes it.

Indeed; to undo this mistake you should rename again to the original name.

@adonovan adonovan changed the title import on save after rename symbol x/tools/gopls: renaming an imported package name does not rename the imported package itself Jun 20, 2024
@adonovan adonovan added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 20, 2024
@adonovan adonovan transferred this issue from golang/vscode-go Jun 20, 2024
@adonovan adonovan added this to the gopls/v0.17.0 milestone Jun 20, 2024
@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 Jun 20, 2024
@adonovan
Copy link
Member

@findleyr wonders whether issue 2 may have been affected by https://go.dev/cl/590377.
Could you try again after installing today's v0.16 release of gopls?
go install golang.org/x/tools/gopls@latest

@Fumbibi
Copy link
Author

Fumbibi commented Jun 22, 2024

Issue 2:

another odd behavior along the same line

I tried to reproduce this using the encoding/xml and encoding/json packages, which both define NewEncoder:

  • add var _ = xml.NewEncoder and save file. import "encoding/xml" appears.
  • replace "xml." with "json." and save file. The import changes to "encoding/json".
  • add var _ = xml. and start typing. Completions within xml package are observed, even though it is not imported.
  • hit tab to accept completion. import "encoding/xml" appears. (We now have both imports.) This is normal.
  • If instead of the previous two steps, we replace the existing var _ = json... with var _ = xml., then start typing NewEnc, accepting the completion adds an xml import but doesn't remove the json import that is no longer needed. This too is normal; redundant imports are not removed until the file is saved.

If you see something different, could you please give instructions for reproducing it with these standard library symbols? Ideally this should be a separate GitHub issue, since it is unrelated to the other two items.

I installed the latest gopls as per @findleyr and followed the steps for issue 2 and everything behaved as you described as normal with the redundant imports not being removed until saving the file and the xml completions showing before the package is imported on-save.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

3 participants