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

Closed
stamblerre opened this issue Jul 1, 2020 · 10 comments
Closed

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

stamblerre opened this issue Jul 1, 2020 · 10 comments
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@stamblerre
Copy link
Contributor

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 cmd/go label Jul 1, 2020
@gopherbot
Copy link

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

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 3, 2020
@dmitshur dmitshur added this to the Backlog milestone Jul 3, 2020
@jayconrod
Copy link
Contributor

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
@jayconrod jayconrod added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 6, 2020
@gopherbot
Copy link

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

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

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>
@jayconrod jayconrod modified the milestones: Go1.16, Go1.17 Jan 7, 2021
@ianlancetaylor
Copy link
Contributor

Are we going to do anything here for 1.17? Thanks.

@bcmills
Copy link
Contributor

bcmills commented May 5, 2021

It looks like this will be the easiest route to fixing #45827, so this may yet happen for 1.17.

@bcmills bcmills self-assigned this May 5, 2021
@gopherbot
Copy link

Change https://golang.org/cl/317409 mentions this issue: go/packages: in overlay_test, allow test variant in second_file case

@gopherbot
Copy link

Change https://golang.org/cl/317299 mentions this issue: go/build: avoid duplicates in InvalidGoFiles

gopherbot pushed a commit to golang/tools that referenced this issue May 10, 2021
When CL 241577 lands, 'go list -e' will include unparseable files
(like a file without a package declaration) in GoFiles in addition to
InvalidGoFiles.

This makes go/packages more tolerant of empty test files. With
CL 241577, if a file=src.go query is given when an empty _test.go is
present, packages.Load will return two matching packages: the library
under test, and the internal test package.

This CL broadens overlay_test.go so the new behavior doesn't break the
test.

For golang/go#39986

Change-Id: I14d019129465e928a3f60a2230edd2e2d1d74687
Reviewed-on: https://go-review.googlesource.com/c/tools/+/317409
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue May 10, 2021
For #45827
For #39986
Updates #45999

Change-Id: I0c919b6a2e56e7003b90425487eafe0f0eadc609
Reviewed-on: https://go-review.googlesource.com/c/go/+/317299
Trust: Bryan C. Mills <bcmills@google.com>
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/317300 mentions this issue: cmd/go: include packages with InvalidGoFiles when filtering main packages

gopherbot pushed a commit that referenced this issue May 10, 2021
go/build.ImportDir returns a *build.Package with various lists of
files. If a file is invalid for some reason, for example, because it
has a different package name than other files, it's added to
InvalidGoFiles in addition to GoFiles, TestGoFiles, or other lists.

Previously, files with parse errors or build constraint errors were
not included in these lists, which causes problems for tools that use
'go list' since InvalidGoFiles is not printed. With this change, files
with any kind of error are added to one of the GoFiles lists.

Fixes #39986

Change-Id: Iee007b5092293eb4420c8a39ce731805fe32135f
Reviewed-on: https://go-review.googlesource.com/c/go/+/241577
Trust: Jay Conrod <jayconrod@google.com>
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators May 10, 2022
@rsc rsc unassigned bcmills Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants