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,internal/goroot: testdata is surprisingly treated as a standard-library package #65406

Closed
adonovan opened this issue Jan 31, 2024 · 14 comments
Assignees
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@adonovan
Copy link
Member

adonovan commented Jan 31, 2024

This marker test (of hover) fails spuriously because the name of the package is incorrect:

-- go.mod --
module testdata

go 1.18

-- p.go --
package p

type P struct{}

var p P //@hover("P", "P", P)

-- @P --
[`p.P` on pkg.go.dev](https://pkg.go.dev/testdata/p.go#P)

The actual value is command-line-arguments/var/folders/fy/dn6v01n16zjdwsqy_8qfbbxr000_9w/T/Testhoverembed.txt4282490610/001/work, not testdata. I think this is a bug in the metadata. Moving p.go to a subdirectory p fixes the problem. Renaming testdata to example.com makes no difference is an effective workaround.

@adonovan adonovan added the gopls/metadata Issues related to metadata loading in gopls label Jan 31, 2024
@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 31, 2024
@gopherbot gopherbot added this to the Unreleased milestone Jan 31, 2024
@findleyr findleyr modified the milestones: Unreleased, gopls/v0.15.0 Jan 31, 2024
@findleyr
Copy link
Contributor

Looks like this is due to the special name testdata. Any other module name works.

@findleyr
Copy link
Contributor

@bcmills @matloob I think this behavior may be a bug, or should be documented at https://go.dev/ref/mod, which appears to mention only testdata directories, not module names. It's particularly surprising that e.g. module foo.com/testdata seems to work. It's really just the module name testdata that does not work.

Repurposing this issue to mitigate the surprise.

@findleyr findleyr changed the title x/tools/gopls: spurious commaand-line-arguments package in marker test cmd/go: surprising behavior with modules named 'testdata' Jan 31, 2024
@findleyr findleyr added GoCommand cmd/go and removed gopls Issues related to the Go language server, gopls. gopls/metadata Issues related to metadata loading in gopls labels Jan 31, 2024
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 31, 2024
@mknyszek mknyszek modified the milestones: Unreleased, Backlog Jan 31, 2024
@adonovan adonovan self-assigned this Jan 31, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/559457 mentions this issue: gopls/internal/test/marker: reject "module testdata"

@seankhliao
Copy link
Member

supposedly almost all undotted names are considered reserved #37641 ?

@findleyr
Copy link
Contributor

@seankhliao yes, if that were uniformly enforced I think that would be a fine solution. But any other undotted name works in this case -- testdata is unique.

@adonovan
Copy link
Member Author

I assigned it to myself for the gopls CL above; reassigning to bcmills for the go command behavior.

@bcmills
Copy link
Contributor

bcmills commented Jan 31, 2024

The go list behavior looks correct to me:

$ go version
go version devel go1.23-d278d5bb Fri Jan 26 04:31:42 2024 +0000 linux/amd64

$ go mod init testdata
go: creating new go.mod: module testdata

$ cat > main.go
package main

func main() {}

$ go list .
ambiguous import: found package testdata in multiple modules:
        testdata (/tmp/tmp.tYGTQN1PjH)
         (/usr/local/google/home/bcmills/sdk/gotip/src/testdata)

@adonovan, can you give more concrete steps to reproduce the behavior seen in the marker tests?

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 31, 2024
@bcmills
Copy link
Contributor

bcmills commented Jan 31, 2024

Hmm. I guess in theory this shouldn't be ambiguous, because the testdata directory in the Go standard library doesn't include any .go source files:
https://cs.opensource.google/go/go/+/master:src/testdata/

So I think this is arguably a bug in internal/goroot.IsStandardPackage, which (as an optimization) only checks for the existence of the directory, not source files within that directory. (That is in contrast to the logic for resolving modules, which requires at least one .go source file to be present in order to locate a package.)

@bcmills bcmills changed the title cmd/go: surprising behavior with modules named 'testdata' cmd/go,internal/goroot: testdata is surprisingly treated as a standard-library package Jan 31, 2024
@adonovan
Copy link
Member Author

adonovan commented Jan 31, 2024

This session reduces the problem to the two packages.Load calls made by gopls.

First we use the name testdata.com:

$ cd /tmp/xx
$ cat go.mod
$ echo "module testdata.com" > go.mod
$ echo "package p" > p.go
$ gopackages $(pwd)/... builtin
Go package "testdata.com":
	package p
	has no exported type info
	file /tmp/xx/p.go

Go package "builtin":
	package builtin
	has no exported type info
	file /Users/adonovan/w/goroot/src/builtin/builtin.go
	import "cmp"

$ gopackages file=$(pwd)/p.go
Go package "command-line-arguments":
	package p
	has no exported type info
	file /tmp/xx/p.go

Now we use testdata:

$ echo "module testdata" > go.mod
$ gopackages $(pwd)/... builtin
Go package "testdata":
	package 
	has no exported type info
	-: ambiguous import: found package testdata in multiple modules:
	testdata (/tmp/xx)
	 (/Users/adonovan/w/goroot/src/testdata)

Go package "builtin":
	package builtin
	has no exported type info
	file /Users/adonovan/w/goroot/src/builtin/builtin.go
	import "cmp"

$ gopackages file=$(pwd)/p.go
Go package "command-line-arguments":
	package p
	has no exported type info
	file /tmp/xx/p.go

The difference is that the dir/... query in the second case matches the testdata directory in the standard library, making it ambiguous. This causes gopls to issue the second query using file=, which creates the ad hoc package.

So this reduces to:

$ go list testdata
ambiguous import: found package testdata in multiple modules:
	testdata (/tmp/xx)
	 (/Users/adonovan/w/goroot/src/testdata)

So the question is: is testdata really a standard package?

The same behavior occurs when the go.mod name conflicts with any other standard library directory name, such as encoding or fmt, even if it has no .go files, like testdata or text. Is that a bug? Not sure.

gopherbot pushed a commit to golang/tools that referenced this issue Feb 1, 2024
Updates golang/go#65406

Change-Id: I1ce4e4d94f118c4b51dc52d624d90e2f7e05f048
Reviewed-on: https://go-review.googlesource.com/c/tools/+/559457
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@matloob
Copy link
Contributor

matloob commented Feb 5, 2024

@bcmills. Do you think we should change IsStandardImportPath (and the moduleindex variant of it) to check to see that there's at least one .go file in the directory?

@bcmills
Copy link
Contributor

bcmills commented Feb 5, 2024

We probably should, yeah. We can probably get rid of the call to os.Stat and use ReadDir instead, although it may be somewhat more expensive. I wonder if we should also cache the results using a sync.Map. 🤔

@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Feb 5, 2024
@bcmills bcmills removed their assignment Feb 5, 2024
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 5, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/561338 mentions this issue: internal/goroot: in IsStandardPackage check for go source files

@matloob
Copy link
Contributor

matloob commented Feb 5, 2024

Usually we're using the module index, so we probably won't hit the goroot.IsStandardPackage function so caching it probably wouldn't make a big difference.

@bcmills
Copy link
Contributor

bcmills commented Feb 5, 2024

Oh! I just remembered https://go.dev/cl/504063, which seems very closely related.

I wonder if it would be less error-prone for future maintenance to establish the invariant that modindex.GetPackage always returns a non-nil error for a directory that does not contain a Go package, similar to go/build.ImportDir.

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Be more strict in IsStandardPackage: before this change we'd just
check for the existence of the directory, but now we check to see that
there's at least one .go file in the directory.

Also update some comments in the modindex package to reflect the fact
that an IndexPackage might represent a directory that does not contain
any source files.

Fixes golang#65406
Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest,gotip-windows-amd64-longtest

Change-Id: I82f0c0e7bfcd5bb4df0195c4c8c7fc7c67fae53e
Reviewed-on: https://go-review.googlesource.com/c/go/+/561338
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

7 participants