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

cmd/go: expose InvalidGoFiles as part of `go list` API #39986

Open
stamblerre opened this issue Jul 1, 2020 · 5 comments
Open

cmd/go: expose InvalidGoFiles as part of `go list` API #39986

stamblerre opened this issue Jul 1, 2020 · 5 comments
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Jul 1, 2020

What version of Go are you using (go version)?

$ go version
go version devel +a4ba411b19 Wed Jul 1 15:29:29 2020 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes

Repro steps

Consider a directory structure like the following:

mod2
├── go.mod
├── go.sum
├── inner
│   └── inner.go
└── main.go

1 directory, 4 files

If inner.go and main.go are both empty files, go list returns the following:

$ go list -e -json -compiled -test ./...
{
	"Dir": "/Users/rstambler/modules/mod2",
	"ImportPath": "mod2",
	"Root": "/Users/rstambler/modules/mod2",
	"Match": [
		"./..."
	],
	"Incomplete": true,
	"Error": {
		"ImportStack": [],
		"Pos": "main.go:1:1",
		"Err": "expected 'package', found 'EOF'"
	}
}
{
	"Dir": "/Users/rstambler/modules/mod2/inner",
	"ImportPath": "mod2/inner",
	"Root": "/Users/rstambler/modules/mod2",
	"Match": [
		"./..."
	],
	"Incomplete": true,
	"Error": {
		"ImportStack": [],
		"Pos": "inner/inner.go:1:1",
		"Err": "expected 'package', found 'EOF'"
	}
}

To extract the Go files associated with these packages, go/packages will have to parse the error messages.
Is there any possibility of listing the empty files in the GoFiles field?

/cc @jayconrod @bcmills @matloob

@stamblerre stamblerre added the GoCommand label Jul 1, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 2, 2020

Change https://golang.org/cl/240098 mentions this issue: internal/lsp, go/packages: reproduce and fix golang/go#39646

@dmitshur dmitshur added this to the Backlog milestone Jul 3, 2020
@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Jul 6, 2020

I think we could do this. go/build.Context.ImportDir a package with these files InvalidGoFiles, along with an error. The go command sees that error and ignores the returned package. If the package is non-nil though, we could copy a few fields like this where it makes sense.

@jayconrod jayconrod modified the milestones: Backlog, Go1.16 Jul 6, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 8, 2020

Change https://golang.org/cl/241577 mentions this issue: go/build: include files with parse errors in GoFiles and other lists

gopherbot pushed a commit to golang/tools that referenced this issue Jul 10, 2020
This test exposes a lot of fundamental issues with `go list` overlays.
Now that we have the regression test framework, we can copy it over and
make any flakes completely reproducible by waiting for each change to
complete.

The issue here ended up being that initial workspace load returns
workspace packages with no associated files due to golang/go#39986.
Working around this allows invalidation to proceed as usual.

In the process of debugging this, I also noticed that packages can get
loaded as command-line-arguments even when we can get the correct
package path for them. It's pretty complicated to fix this, so
restructured the code a tiny bit and left a TODO. I'd like to come back
to this at some point, but it's not pressing since I was able to fix
this bug.

Fixes golang/go#39646.
Updates golang/go#39986.

Change-Id: Id6b08a5e92d28eddc731feb0ef3fd3b3fc69e64b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/240098
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Jul 15, 2020

We've encountered some limitations with these CLs - specifically with this corner case:

hello.go and hello_test.go are both empty on disk, but in overlays, their contents are package hello and package hello_test. In this case, go list will report both of these files as part of hello. The result of this is that a go/packages contains query will report hello_test's package as the test variant of hello, not as an x test. This results in a lot of complexity for go/packages to work-around, and the goal of this was to avoid such work-arounds, so it doesn't seem worth it.

As an alternative, we proposed exposing the InvalidFiles list from go/build as part of the go list API. This would allow go/packages to treat the invalid files separately from a package, and then create the correct packages based on the overlays.

/cc @matloob

@stamblerre stamblerre changed the title cmd/go: go list reports packages with no associated GoFiles cmd/go: expose InvalidFiles as part of `go list` API Jul 15, 2020
@jayconrod jayconrod changed the title cmd/go: expose InvalidFiles as part of `go list` API cmd/go: expose InvalidGoFiles as part of `go list` API Jul 16, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Jul 21, 2020

Change https://golang.org/cl/244028 mentions this issue: go/packages: find filenames in error strings if not in position

gopherbot pushed a commit to golang/tools that referenced this issue Jul 23, 2020
In the case of an invalid package name (the package name is a keyword),
`go list` doesn't report any filenames associated with the package.
As we have done previously, we can parse these out of the error message.
However, it seems like, in this case, the filename is in the error
string itself, not the error position.

Updates golang/go#39986
Updates golang/go#39763

Change-Id: Id9c68a93ac4f545e4e2152540ca85b92f1df4640
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244028
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
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.