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: inconsistent output for circular dependencies in tests #36265

Open
stamblerre opened this issue Dec 24, 2019 · 3 comments
Open

x/tools/gopls: inconsistent output for circular dependencies in tests #36265

stamblerre opened this issue Dec 24, 2019 · 3 comments

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Dec 24, 2019

Given 2 packages that import each other, go list-ing one of the packages will result in the dependency being listed as having no imports. This avoids infinite cycles, but it has the consequences of producing inconsistent results. I noticed this in gopls, when I first ran go/packages on one of these packages, followed by the other. gopls doesn't necessarily update dependencies after go/packages runs, so the result created a circular dependency. The first work-around I can think of is always updating the metadata for all dependencies, which may be too slow.

I was curious if there is any other way that this could be handled - maybe the alphabetically first package could be picked as the "top level" one, since everything in this case is broken anyway? If not, I will figure out a work-around in gopls, but I just wanted to raise this.

/cc @jayconrod @matloob

Repro:

$ go list -e -json -compiled -deps golang.org/x/tools/internal/lsp/testdata/circular/double/b/
{
	"ImportPath": "golang.org/x/tools/internal/lsp/circular/double/one",
        ...
	"Error": {
		"ImportStack": [
			"golang.org/x/tools/internal/lsp/testdata/circular/double/b",
			"golang.org/x/tools/internal/lsp/circular/double/one"
		],
		"Pos": "golang.org/x/tools/internal/lsp/testdata/circular/double/b/b.go:4:2",
		"Err": "..."
	}
}
{
	"ImportPath": "golang.org/x/tools/internal/lsp/testdata/circular/double/b",
         ...
	"GoFiles": [
		"b.go"
	],
	"CompiledGoFiles": [
		"b.go"
	],
	"Imports": [
		"golang.org/x/tools/internal/lsp/circular/double/one"
	],
	"Deps": [
		"golang.org/x/tools/internal/lsp/circular/double/one"
	],
	"DepsErrors": [
		{
			"ImportStack": [
				"golang.org/x/tools/internal/lsp/testdata/circular/double/b",
				"golang.org/x/tools/internal/lsp/circular/double/one"
			],
			"Pos": "golang.org/x/tools/internal/lsp/testdata/circular/double/b/b.go:4:2",
			"Err": "..."
		}
	]
}
deps golang.org/x/tools/internal/lsp/testdata/circular/double/b/

$ go list -e -json -compiled -deps golang.org/x/tools/internal/lsp/testdata/circular/double/one/
...<opposite>...
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 24, 2019

Change https://golang.org/cl/212102 mentions this issue: internal/lsp: parallelize initial workspace load

@cagedmantis cagedmantis added this to the Backlog milestone Dec 30, 2019
@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Jan 3, 2020

This is the error I'm actually seeing on the internal package. I think /testdata/ is missing from the import in both packages, so the imported package doesn't actually exist.

{
	"ImportPath": "golang.org/x/tools/internal/lsp/circular/double/one",
	"DepOnly": true,
	"Incomplete": true,
	"Stale": true,
	"StaleReason": "build ID mismatch",
	"Error": {
		"ImportStack": [
			"golang.org/x/tools/internal/lsp/testdata/circular/double/b",
			"golang.org/x/tools/internal/lsp/circular/double/one"
		],
		"Pos": "internal/lsp/testdata/circular/double/b/b.go:4:2",
		"Err": "no matching versions for query \"latest\""
	}
}

The error message is pretty bad, but the behavior seems correct. Note that import errors are attached to the imported package, which may be a dummy package (that can confuse go/packages; see #36188), not the importing package.

@stamblerre Could you just confirm it's /testdata/ that's missing? Here's the output I get after fixing the imports. It seems like what you want.

$ go list -e -json -compiled -deps ./internal/lsp/testdata/circular/double/b
{
	"Dir": "/Users/jayconrod/go/src/golang.org/x/tools/internal/lsp/testdata/circular/double/one",
	"ImportPath": "golang.org/x/tools/internal/lsp/testdata/circular/double/one",
	"Name": "one",
	"Root": "/Users/jayconrod/go/src/golang.org/x/tools",
	"Module": {
		"Path": "golang.org/x/tools",
		"Main": true,
		"Dir": "/Users/jayconrod/go/src/golang.org/x/tools",
		"GoMod": "/Users/jayconrod/go/src/golang.org/x/tools/go.mod",
		"GoVersion": "1.11"
	},
	"DepOnly": true,
	"Incomplete": true,
	"Stale": true,
	"StaleReason": "stale dependency: golang.org/x/tools/internal/lsp/testdata/circular/double/b",
	"GoFiles": [
		"one.go"
	],
	"CompiledGoFiles": [
		"one.go"
	],
	"Imports": [
		"golang.org/x/tools/internal/lsp/testdata/circular/double/b"
	],
	"Deps": [
		"golang.org/x/tools/internal/lsp/testdata/circular/double/b"
	],
	"DepsErrors": [
		{
			"ImportStack": [
				"golang.org/x/tools/internal/lsp/testdata/circular/double/b",
				"golang.org/x/tools/internal/lsp/testdata/circular/double/one",
				"golang.org/x/tools/internal/lsp/testdata/circular/double/b"
			],
			"Pos": "",
			"Err": "import cycle not allowed"
		}
	]
}
{
	"Dir": "/Users/jayconrod/go/src/golang.org/x/tools/internal/lsp/testdata/circular/double/b",
	"ImportPath": "golang.org/x/tools/internal/lsp/testdata/circular/double/b",
	"Name": "b",
	"Root": "/Users/jayconrod/go/src/golang.org/x/tools",
	"Module": {
		"Path": "golang.org/x/tools",
		"Main": true,
		"Dir": "/Users/jayconrod/go/src/golang.org/x/tools",
		"GoMod": "/Users/jayconrod/go/src/golang.org/x/tools/go.mod",
		"GoVersion": "1.11"
	},
	"Match": [
		"./internal/lsp/testdata/circular/double/b"
	],
	"Incomplete": true,
	"Stale": true,
	"StaleReason": "build ID mismatch",
	"GoFiles": [
		"b.go"
	],
	"CompiledGoFiles": [
		"b.go"
	],
	"Imports": [
		"golang.org/x/tools/internal/lsp/testdata/circular/double/one"
	],
	"Deps": [
		"golang.org/x/tools/internal/lsp/testdata/circular/double/b",
		"golang.org/x/tools/internal/lsp/testdata/circular/double/one"
	],
	"Error": {
		"ImportStack": [
			"golang.org/x/tools/internal/lsp/testdata/circular/double/b",
			"golang.org/x/tools/internal/lsp/testdata/circular/double/one",
			"golang.org/x/tools/internal/lsp/testdata/circular/double/b"
		],
		"Pos": "",
		"Err": "import cycle not allowed"
	},
	"DepsErrors": [
		{
			"ImportStack": [
				"golang.org/x/tools/internal/lsp/testdata/circular/double/b",
				"golang.org/x/tools/internal/lsp/testdata/circular/double/one",
				"golang.org/x/tools/internal/lsp/testdata/circular/double/b"
			],
			"Pos": "",
			"Err": "import cycle not allowed"
		}
	]
}
@stamblerre

This comment has been minimized.

Copy link
Contributor Author

@stamblerre stamblerre commented Jan 6, 2020

Thanks for looking into this. The fact that these files are in testdata does seem to be the problem. The missing testdata in the imports is handled by the test harness, but it seems like something isn't working correctly either in our tests or in gopackagestest. I confirmed that go list works as expected outside of the testdata. Repurposing this issue to figure out what's going on with the tests.

@stamblerre stamblerre changed the title {cmd/go,x/tools/go/packages}: inconsistent output for circular dependencies x/tools/gopls: inconsistent output for circular dependencies in tests Jan 6, 2020
gopherbot pushed a commit to golang/tools that referenced this issue Jan 7, 2020
The initial workspace load was happening when a view was created, in serial.
It should really just be kicked off in a separate goroutine once we create a
new view. Implementing this change required some other significant changes,
particularly the additional work being done by the WorkspacePackageIDs
method.

Some other changes had to be made while debugging. In particular, the
modification to the circular dependencies test was a consequence of
golang/go#36265.

Change-Id: I97586c9574f6c4106172d7983e4c6fad412e6aa1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212102
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@stamblerre stamblerre modified the milestones: Backlog, gopls/v1.0.0 Jan 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.