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

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

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

stamblerre opened this issue Dec 24, 2019 · 4 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. Tools This label describes issues relating to any tools in the x/tools repository.

Comments

@stamblerre
Copy link
Contributor

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
Copy link

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
@cagedmantis cagedmantis added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 30, 2019
@jayconrod
Copy link
Contributor

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"
		}
	]
}

@jayconrod jayconrod added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 3, 2020
@stamblerre
Copy link
Contributor Author

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 gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jan 6, 2020
gopherbot pushed a commit to golang/tools that referenced this issue Jan 7, 2020
@stamblerre
internal/lsp: parallelize initial workspace load
7201abb 
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 removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 26, 2020
@stamblerre stamblerre modified the milestones: Backlog, gopls/v1.0.0 Jan 26, 2020
@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.4.0 Feb 2, 2020
@stamblerre stamblerre modified the milestones: gopls/v0.4.0, gopls/v0.5.0 Apr 2, 2020
@stamblerre stamblerre changed the title x/tools/gopls: inconsistent output for circular dependencies in tests x/tools/gopls: inconsistent output for circular dependencies in testdata May 10, 2020
@stamblerre stamblerre added the Testing An issue that has been verified to require only test changes, not just a test failure. label Jun 24, 2020
@stamblerre stamblerre removed this from the gopls/v0.5.0 milestone Jun 24, 2020
@stamblerre
Copy link
Contributor Author

I think this is a duplicate of #40332 - I don't think it's worth figuring out what's going on with the testdata.

@golang golang locked and limited conversation to collaborators Jul 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@cagedmantis @jayconrod @stamblerre @gopherbot