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: adding a file to a package reports failed imports in the other package files #35949

Closed
bmhatfield opened this issue Dec 3, 2019 · 7 comments
Assignees
Labels
Milestone

Comments

@bmhatfield
Copy link

@bmhatfield bmhatfield commented Dec 3, 2019

This is consistently reproducible with gopls+VSCode, on many versions of gopls up through db047d72 (go 1.13.4).

Example workflow:
I have a package with 4 files in it. 3 of the 4 files have various import statements, mixed between standard library and external packages. The 4th file only defines some constants and does not have any import statements. This package is within a multi-package repository, all packages share the same go.mod (~2 filesystem levels up the tree).

If I add a 5th file, the pre-existing 3 files with import statements in the same package all turn red immediately (in the VSCode file navigator). In those pre-existing 3 files, all of the import statements are now highlighted red with the error "could not import X (no package for import X)".

Adding a blank line to one of the files reporting an import error and re-saving the file does gofmt the file (to remove the blank line), but does not resolve the reported import errors.

Using VSCode's "Go: Restart Language Server" feature immediately updates the VSCode UI to no longer report broken imports.

Suspicions/Observations:

I noticed that the editor turns red immediately, while the new file is totally empty. I know that for go, an empty .go file results in a compilation error with go build:

[bhatfield server]% go build
can't load package: package github.com/digits/go-services/shared/server:
grpc.go:1:1: expected 'package', found 'EOF'

Unfortunately, adding package server to the empty file does not resolve the reported import errors in the other files.

Additionally, after restarting gopls in the workflow above, removing the package declaration from the new file and re-saving does not cause the other files to turn back red. However, as soon as I type package in the new/empty file, the other files turn red again.

Expectations:
I would expect a few different behaviors here:

  • I would not expect an empty file to bring gopls into a state where all files in the package are reported as failed, as they are not, in fact, broken in any way.
  • I would expect gopls to recover once the package declaration is corrected in the new file.

Impact:
The result of this error, which I have been living with (just regularly restarting gopls whenever it happens), means that I "feel like" gopls fights me when I try to refactor code by moving it between files, adding new files, etc.

@gopherbot gopherbot added this to the Unreleased milestone Dec 3, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 3, 2019

Thank you for filing a gopls issue! Please take a look at the Troubleshooting guide, and make sure that you have provided all of the relevant information here.

@stamblerre stamblerre changed the title x/tools/gopls: Adding a file to a package reports failed imports in the other package files x/tools/gopls: adding a file to a package reports failed imports in the other package files Dec 3, 2019
@stamblerre stamblerre modified the milestones: Unreleased, gopls v1.0 Dec 3, 2019
@heschik heschik self-assigned this Dec 4, 2019
@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 4, 2019

Notes:

As suggested above, the problem is that go list dies horribly when it encounters a file with no package declaration. VS Code seems to like to create a file as empty, then didChange contents into it, which means that we start off with a packages.Load call that triggers the go list bug. https://golang.org/cl/201220 then hides the error from us, giving back an apparently successful result that has no Imports or Deps. My feeling is that that CL was a mistake and we should let the error through, but we haven't decided that yet.

That leaves the question of why the problem sticks around...

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Dec 4, 2019

For reference: #35973.

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Dec 4, 2019

The reason it sticks around is that (*snapshot).shouldCheck doesn't think a reload is needed -- no deps are missing, because there are none, and unless you add an import that gopls hasn't seen yet, it won't ever do a reload. I don't think there's any option but to roll back https://golang.org/cl/201220.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 5, 2019

Change https://golang.org/cl/209978 mentions this issue: go/packages: revert "handle invalid files in overlays"

@bmhatfield

This comment has been minimized.

Copy link
Author

@bmhatfield bmhatfield commented Dec 5, 2019

Confirmed that the fix for this works in VSCode for me!

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 6, 2019

Change https://golang.org/cl/210206 mentions this issue: go/packages: revert "handle invalid files in overlays"

gopherbot pushed a commit to golang/tools that referenced this issue Dec 6, 2019
This reverts commit 6f5e273, golang.org/cl/201220.

Reason for revert: Produces unusable Package results. See golang/go#35949 and golang/go#35973.

Fixes golang/go#35949.

Change-Id: Ic4632fe7f00366b1edf59840c83160d66499ba12
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209978
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
(cherry picked from commit 427c522)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/210206
Reviewed-by: Heschi Kreinick <heschi@google.com>
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
4 participants
You can’t perform that action at this time.