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: corrupted file= query with files of different cases #38602

Closed
stamblerre opened this issue Apr 22, 2020 · 5 comments
Closed

x/tools/gopls: corrupted file= query with files of different cases #38602

stamblerre opened this issue Apr 22, 2020 · 5 comments
Labels
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 22, 2020

See the discussion on microsoft/vscode-go#3193.

/cc @heschik for case-insensitive file systems

@heschik
Copy link
Contributor

@heschik heschik commented Apr 23, 2020

I tracked this down.

panic: GetFile on  /tmp/scratch/case-insensitive import collision
golang.org/x/tools/internal/lsp/cache.(*snapshot).GetFile
golang.org/x/tools/internal/lsp/source.clearReports
golang.org/x/tools/internal/lsp/source.Diagnostics
golang.org/x/tools/internal/lsp.(*Server).diagnose.func2

clearReports probably shouldn't register a file that doesn't already exist, and we should ideally not treat case-insensitive import collision: "example.com/foo/x" and "example.com/Foo/x" as an error in the file "case-insensitive import collision".

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Apr 24, 2020

Ah, thanks for tracking this down. I was probably too ambitious when I removed "FindFile" from the snapshot. Do you have a good sense of how we could write a regtest for this? I guess we would have to check on the condition of no diagnostics for "case-insensitive import collision"?

@heschik
Copy link
Contributor

@heschik heschik commented Apr 24, 2020

Are you more worried about the gratuitous Loads or the fact that the diagnostic got swallowed? I don't know how to write a test for the former, but the latter should be easy.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Apr 24, 2020

Really the former, but I think the latter could account for both? I'll try to figure out a good regtest for this. I can have a fix out for this soon - unless you already have one?

@gopherbot
Copy link

@gopherbot gopherbot commented Apr 28, 2020

Change https://golang.org/cl/230315 mentions this issue: internal/lsp: make sure diagnostics only refer to exisiting files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.