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

Jump to bottom

x/tools/go/packages: report ImportStack for import cycles #35964

Closed
stamblerre opened this issue Dec 4, 2019 · 5 comments
Closed

x/tools/go/packages: report ImportStack for import cycles #35964

stamblerre opened this issue Dec 4, 2019 · 5 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@stamblerre
Copy link
Contributor

When go list detects an import cycle, it reports the following error message:

$ go list -e -json ~/mod1/foo/
...
"Error": {
	"ImportStack": [
		"mod1/foo",
		"mod1/foo"
	],
	"Pos": "",
	"Err": "import cycle not allowed"
}
...

go/packages does not propagate the ImportStack, so it's not possible to determine the position in the file that the offending import corresponds to. Ideally, go/packages would propagate the ImportStack in the error, which would allow gopls to extract a position for this error message. Even more ideally, go list should report a position. 😄

/cc @ridersofrohan @matloob @jayconrod
@gopherbot gopherbot added this to the Unreleased milestone Dec 4, 2019
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Dec 4, 2019
@ridersofrohan ridersofrohan mentioned this issue Dec 4, 2019
Closed
@ridersofrohan
Copy link

ridersofrohan commented Dec 4, 2019 •
edited

When go/packages detects an import cycle, it says that the packages.ErrorKind is a UnknownError not a ListError as defined on line 38 of internal/lsp/cache/errors.go

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 5, 2019
@gopherbot
Copy link

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

@ridersofrohan
Copy link

This for the most part got handled in commit/0d08730. The packages.ErrorKind is still an UnknownError as opposed to a ListError.

@stamblerre
Copy link
Contributor Author

@ridersofrohan: It should be a one-line fix to a kind of List on line 818 of go/packages/golist.go, if you or @matloob would like to mail that CL.

gopherbot pushed a commit to golang/tools that referenced this issue Dec 13, 2019
@ridersofrohan
internal/lsp: add diagnostic on import causing import cycle
7ebc6af 
After the addition of golang/go#35964, the import cycle error now
has the import stack attached in the message. This CL parses that
stack and attached the import cycle diagnostic to the import versus
just adding it to the first character of the .go file.

Fixes golang/go#33085

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

Change https://golang.org/cl/212138 mentions this issue: go/packages: change import cycle errorkind from UnknownError to ListError

@golang golang locked and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@toothrot @ridersofrohan @stamblerre @gopherbot