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: show diagnostic for for circular dependency #33085

Open
inliquid opened this issue Jul 12, 2019 · 7 comments
Open

x/tools/gopls: show diagnostic for for circular dependency #33085

inliquid opened this issue Jul 12, 2019 · 7 comments
Assignees

Comments

@inliquid
Copy link

@inliquid inliquid commented Jul 12, 2019

VS Code. Say, we have a circular dependency. In this case go vet would warn that it's not allowed. However it seems that gopls doesn't check this and just stops working (not providing hover outputs, references, what so ever) with these non-informative errors in output:

[Error - 21:27:10] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:10] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:10] Request textDocument/hover failed.
  Message: no AST for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go
  Code: 0 
[Error - 21:27:12] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:13] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:13] Request textDocument/definition failed.
  Message: no AST for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go
  Code: 0 
[Error - 21:27:13] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:14] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:14] Request textDocument/hover failed.
  Message: no AST for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go
  Code: 0 
[Error - 21:27:15] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:16] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:16] Request textDocument/definition failed.
  Message: no AST for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go
  Code: 0 
[Error - 21:27:16] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:17] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:17] Request textDocument/hover failed.
  Message: no AST for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go
  Code: 0 
[Error - 21:27:18] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:18] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:18] Request textDocument/definition failed.
  Message: no AST for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go
  Code: 0 
[Error - 21:27:19] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:20] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking
[Error - 21:27:20] no highlight for c:\Users\***\go\src\path\to\workdir\pkg\client\ui\ui.go:21:25: no AST for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go
[Error - 21:27:21] unable to check package for file:///c:/Users/***/go/src/path/to/workdir/pkg/client/ui/ui.go: package path/to/workdir/pkg/client/ui has errors, skipping type-checking

@gopherbot gopherbot added this to the Unreleased milestone Jul 12, 2019
@gopherbot gopherbot added the gopls label Jul 12, 2019
@stamblerre stamblerre changed the title x/cmd/gopls: gopls doesn't vet package for circular dependency x/tools/gopls: show diagnostic for for circular dependency Jul 12, 2019
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jul 12, 2019

Good catch, thank you for noticing this. You are right, gopls will not even bother to check your package if it notices a circular dependency (https://github.com/golang/tools/blob/d5f455491e2f028578d3349b43426ca65e2e0ee0/internal/lsp/cache/check.go#L51), so we should show a diagnostic for those cases.

@jbszczepaniak

This comment has been minimized.

Copy link

@jbszczepaniak jbszczepaniak commented Nov 14, 2019

I would like to claim this one ;)

@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Nov 14, 2019

Great! Let me know if you need any advice on how to tackle it, or please feel free to reach out to me directly on Gophers Slack.

@jbszczepaniak

This comment has been minimized.

Copy link

@jbszczepaniak jbszczepaniak commented Nov 20, 2019

As @stamblerre suggested I will take something easier for first task. Leaving this for now.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 4, 2019

Change https://golang.org/cl/209857 mentions this issue: internal/lsp: add error handling for self imports

@ridersofrohan

This comment has been minimized.

Copy link

@ridersofrohan ridersofrohan commented Dec 4, 2019

After #35964 gets fixed. We need to adjust/write the diagnostic tests inside (tools/internal/lsp/testdata/circular/) to test the behavior and ensure that it is working.

gopherbot pushed a commit to golang/tools that referenced this issue Dec 6, 2019
This change will provide a more useful error when you
are self importing a package. It has TODOs in place to propagate the
"import cycle not allowed" error from go list to the user.

Updates golang/go#33085

Change-Id: Ia868a7c688b0f0a7a9689cfda5ea8cea8ae1faff
Reviewed-on: https://go-review.googlesource.com/c/tools/+/209857
Run-TryBot: Rohan Challa <rohan@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 11, 2019

Change https://golang.org/cl/210942 mentions this issue: internal/lsp: add diagnostic on import causing import cycle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.