-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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: crash during internal/imports.scanDirForPackage (crash) #67156
Comments
Thanks for the report @Sangwaniya |
Looks like root.Path has the same length as dir, but is not a prefix of it. func (r *ModuleResolver) scanDirForPackage(root gopathwalk.Root, dir string) directoryPackageInfo {
subdir := ""
if dir != root.Path {
subdir = dir[len(root.Path)+len("/"):] // slice bounds out of range [45:44]
} |
Moving to the v0.17.0 milestone, since this seems to be affecting only one user (and in golang/vscode-go#3401 (comment), it appears that the cause may be related to a modified GOROOT). |
hi @findleyr even after correcting the path and rebuilding gopls, still gopls is crashing ATTENTION: PLEASE PROVIDE THE DETAILS REQUESTED BELOW. Describe what you observed. panic: runtime error: slice bounds out of range [45:44] goroutine 1561 [running]: golang.org/x/tools/internal/imports.(*ModuleResolver).scanDirForPackage(0xc00099c210, {{0xc000580de5%3F, 0x336f25b8b5a7258c%3F}, 0x8%3F}, {0xc0007dd1d0, 0x2c}) mod.go:727 0x705 golang.org/x/tools/internal/imports.(*ModuleResolver).scan.func3({{0xc000580de5%3F, 0x1215e20%3F}, 0xc000737ca0%3F}, {0xc0007dd1d0%3F, 0x1c212a8%3F}) mod.go:601 0x50 golang.org/x/tools/internal/gopathwalk.(*walker).walk(0xc000770cf0, {0xc000617a00, 0x37}, 0x0, {0x1650af8, 0xc0001bac40}) walk.go:268 0x3d7 golang.org/x/tools/internal/gopathwalk.(*walker).walk(0xc000770cf0, {0xc000580de5, 0x2c}, 0x0, {0x1650b30, 0xc000737c60}) walk.go:334 0x87b golang.org/x/tools/internal/gopathwalk.walkDir({{0xc000580de5%3F, 0xc000bdff18%3F}, 0xc000bdff18%3F}, 0xc000737bf0, 0xc000737be0, {0x0%3F, 0x0%3F, 0x4%3F}) walk.go:120 0x366 golang.org/x/tools/internal/gopathwalk.WalkSkip(...) walk.go:77 golang.org/x/tools/internal/imports.(*ModuleResolver).scan.func4() mod.go:626 0x2db created by golang.org/x/tools/internal/imports.(*ModuleResolver).scan in goroutine 226 mod.go:610 0x465 gopls stats -anon{ "DirStats": { "Files": 2172, "TestdataFiles": 0, "GoFiles": 1626, "ModFiles": 2, "Dirs": 555 }, "GOARCH": "amd64", "GOOS": "windows", "GOPACKAGESDRIVER": "", "GOPLSCACHE": "", "GoVersion": "go1.21.6", "GoplsVersion": "v0.15.3", "InitialWorkspaceLoadDuration": "1.1454599s", "MemStats": { "HeapAlloc": 2580040, "HeapInUse": 5332992, "TotalAlloc": 36334560 }, "WorkspaceStats": { "Files": { "Total": 1, "Largest": 3429, "Errs": 0 }, "Views": [ { "GoCommandVersion": "go1.21.6", "AllPackages": { "Packages": 0, "LargestPackage": 0, "CompiledGoFiles": 0, "Modules": 0 }, "WorkspacePackages": { "Packages": 0, "LargestPackage": 0, "CompiledGoFiles": 0, "Modules": 0 }, "Diagnostics": 0 } ] } }OPTIONAL: If you would like to share more information, you can attach your complete gopls logs. NOTE: THESE MAY CONTAIN SENSITIVE INFORMATION ABOUT YOUR CODEBASE. <OPTIONAL: ATTACH LOGS HERE> |
golang/vscode-go#3456 is another crash report. The report came from Linux. (@feldgendler by any chance, are you using a modified go, or GOROOT source?) |
My Go comes from an official tarball, |
I tried adding debug prints just before the crash. Just before the crashing
They just accidentally happen to have the same length. |
Thanks @feldgendler, that's useful information that the paths are completely unrelated. That suggests the bug is elsewhere (in the code that expects scanned directories to be a subdirectory of the root...). My first guess is that there's something related to symlinks, but we'll need to investigate. |
In Now, I'd very much like to help debugging this because this bug is making my IDE almost completely useless. I don't understand the idea behind the code here — what should it really do with symlinks? Does it need to resolve them at all? |
Thanks, I will have more time to help investigate this later today. In general, the philosophy around symlinks is:
The code in question is very old, but has recently been modified to fix other bugs. It seems quite likely that some modification introduced a buggy handling of symlinks. |
Thanks! Meanwhile, I tried replacing the crashing line with this: if s, ok := strings.CutPrefix(dir, root.Path+"/"); ok {
subdir = s
} It seems to express the intent of the original line in a more robust way — assuming I understood it right. This change seems to fix the issue for me — at least the scan now finishes without crashing. |
@feldgendler yes, at a minimum we'll do something like that. Now that we have a someone reproducing the issue, it would be great if we could track down the root cause. If you don't mind, later today or tomorrow I'm going to come up with a list of things for you to check, which will help us narrow this down. In the meantime, please use your patch to unblock your work. |
Sure! Thanks a lot. |
D'oh, I only just noticed that the scanned dir actually is a subdirectory of the root... if you clean the Root path. Presumably you have a relative GOPATH, GOROOT, or replace directive, and that is being mishandled by goimports. I'm pretty confident that I can reproduce this -- peeking at the code indeed indicates that roots are not clean, and yet scanned directories are. Thanks for taking the time to debug. |
Is there anything else I can do to help you here? |
@feldgendler I've reproduced this crash in a test, and will send a fix shortly. If you could confirm that |
Change https://go.dev/cl/603635 mentions this issue: |
@feldgendler yes correct |
|
I just tested |
@feldgendler indeed we have not yet cherry-picked the fix to a prerelease (we were actually just testing our new release automation when we cut -pre.1). We will fix in a -pre.2, thanks. |
I'm not good at this syntax — is there a Go version expression I could use with |
@feldgendler the best way to test the fix would be to install gopls@master, which you can do with the following command (we need to install from source as there is a replace directive in the gopls go.mod file during development).
|
No crash when installed in this way, thank you! |
Is this a leftover from before workspaces were added, or is there a reason to keep this replacement instead of go.work? |
@dottedmag this is from before workspaces were added, but no we keep it because best practice is to not check in go.work files (see documentation here: https://go.dev/ref/mod#go-work-file). In particular, while correctness requires that golang.org/x/tools/gopls imports the local golang.org/x/tools during development (because it uses the internals of x/tools), we want to support developers working on additional modules if they so choose. Checking in a |
gopls version: v0.15.3/go1.21.6
gopls flags:
update flags: proxy
extension version: 0.41.4
environment: Visual Studio Code win32
initialization error: undefined
issue timestamp: Thu, 02 May 2024 17:33:19 GMT
restart history:
Thu, 02 May 2024 17:30:10 GMT: activation (enabled: true)
ATTENTION: PLEASE PROVIDE THE DETAILS REQUESTED BELOW.
Describe what you observed.
gopls stats -anon
{ "DirStats": { "Files": 2035, "TestdataFiles": 0, "GoFiles": 1626, "ModFiles": 2, "Dirs": 528 }, "GOARCH": "amd64", "GOOS": "windows", "GOPACKAGESDRIVER": "", "GOPLSCACHE": "", "GoVersion": "go1.21.6", "GoplsVersion": "v0.15.3", "InitialWorkspaceLoadDuration": "7.6558984s", "MemStats": { "HeapAlloc": 31132976, "HeapInUse": 41328640, "TotalAlloc": 79278536 }, "WorkspaceStats": { "Files": { "Total": 2207, "Largest": 1654271, "Errs": 0 }, "Views": [ { "GoCommandVersion": "go1.21.6", "AllPackages": { "Packages": 417, "LargestPackage": 134, "CompiledGoFiles": 2217, "Modules": 49 }, "WorkspacePackages": { "Packages": 0, "LargestPackage": 0, "CompiledGoFiles": 0, "Modules": 0 }, "Diagnostics": 0 } ] } }OPTIONAL: If you would like to share more information, you can attach your complete gopls logs.
NOTE: THESE MAY CONTAIN SENSITIVE INFORMATION ABOUT YOUR CODEBASE.
DO NOT SHARE LOGS IF YOU ARE WORKING IN A PRIVATE REPOSITORY.
<OPTIONAL: ATTACH LOGS HERE>
The text was updated successfully, but these errors were encountered: