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: strange attribution of missing imports #65335

Open
aclements opened this issue Jan 28, 2024 · 3 comments
Open

cmd/go: strange attribution of missing imports #65335

aclements opened this issue Jan 28, 2024 · 3 comments
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@aclements
Copy link
Member

Go version

go version go1.21.6 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/austin/.cache/go-build'
GOENV='/home/austin/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/austin/r/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/austin/r/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/home/austin/sdk/go1.21.6'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/home/austin/sdk/go1.21.6/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.21.6'
GCCGO='/usr/bin/gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/tmp/m/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2463259172=/tmp/go-build -gno-record-gcc-switches'

What did you do?

go install golang.org/x/exp/cmd/txtar@latest
mkdir m
cd m
txtar -x <<EOF
-- go.mod --
module m
-- a/a.go --
package a

import _ "x"
-- b/b.go --
package b

import _ "x"
EOF
go list -deps -json ./a ./b

What did you see happen?

Here's the full output:

a/a.go:3:8: package x is not in std (/home/austin/sdk/go1.21.6/src/x)
{
	"ImportPath": "x",
	"DepOnly": true,
	"Incomplete": true,
	"Stale": true,
	"StaleReason": "build ID mismatch",
	"Error": {
		"ImportStack": [
			"m/a"
		],
		"Pos": "a/a.go:3:8",
		"Err": "package x is not in std (/home/austin/sdk/go1.21.6/src/x)"
	}
}
{
	"Dir": "/tmp/m/a",
	"ImportPath": "m/a",
	"Name": "a",
	"Root": "/tmp/m",
	"Module": {
		"Path": "m",
		"Main": true,
		"Dir": "/tmp/m",
		"GoMod": "/tmp/m/go.mod",
		"GoVersion": "1.16"
	},
	"Match": [
		"./a"
	],
	"Incomplete": true,
	"Stale": true,
	"StaleReason": "stale dependency: x",
	"GoFiles": [
		"a.go"
	],
	"Imports": [
		"x"
	],
	"Deps": [
		"x"
	],
	"DepsErrors": [
		{
			"ImportStack": [
				"m/a"
			],
			"Pos": "a/a.go:3:8",
			"Err": "package x is not in std (/home/austin/sdk/go1.21.6/src/x)"
		}
	]
}
{
	"Dir": "/tmp/m/b",
	"ImportPath": "m/b",
	"Name": "b",
	"Root": "/tmp/m",
	"Module": {
		"Path": "m",
		"Main": true,
		"Dir": "/tmp/m",
		"GoMod": "/tmp/m/go.mod",
		"GoVersion": "1.16"
	},
	"Match": [
		"./b"
	],
	"Incomplete": true,
	"Stale": true,
	"StaleReason": "stale dependency: x",
	"GoFiles": [
		"b.go"
	],
	"Imports": [
		"x"
	],
	"Deps": [
		"x"
	],
	"DepsErrors": [
		{
			"ImportStack": [
				"m/a"
			],
			"Pos": "a/a.go:3:8",
			"Err": "package x is not in std (/home/austin/sdk/go1.21.6/src/x)"
		}
	]
}

And just the bits I found surprising:

a/a.go:3:8: package x is not in std (/home/austin/sdk/go1.21.6/src/x)
{
	"ImportPath": "x",
	...
	"Error": {
		"ImportStack": [
			"m/a"
		],
		"Pos": "a/a.go:3:8",
		"Err": "package x is not in std (/home/austin/sdk/go1.21.6/src/x)"
	}
}
{
	"ImportPath": "m/a",
	...
	"DepsErrors": [
		{
			"ImportStack": [
				"m/a"
			],
			"Pos": "a/a.go:3:8",
			"Err": "package x is not in std (/home/austin/sdk/go1.21.6/src/x)"
		}
	]
}
{
	"ImportPath": "m/b",
	...
	"DepsErrors": [
		{
			"ImportStack": [
				"m/a"
			],
			"Pos": "a/a.go:3:8",
			"Err": "package x is not in std (/home/austin/sdk/go1.21.6/src/x)"
		}
	]
}

Notably:

  1. Only the error in a is reported in the text output, not the bad import in b.
  2. The errors are reported as DepsErrors on packages a and b, rather than just Errors
  3. The error directly attributed to "package" x reports a position in package a (of the import of x)
  4. We see the same error in package b, attributed to the import line in a, even though a and b have no relationship

What did you expect to see?

I expected go list to report errors on packages a and b attributed to the bad import statements. That is, one error on package a at a/a.go:3 and another on package b at b/b.go:3. Something like:

a/a.go:3:8: package x is not in std (/home/austin/sdk/go1.21.6/src/x)
b/b.go:3:8: package x is not in std (/home/austin/sdk/go1.21.6/src/x)
{
	"ImportPath": "m/a",
	...
	"Error": {
		"ImportStack": null,
		"Pos": "a/a.go:3:8",
		"Err": "package x is not in std (/home/austin/sdk/go1.21.6/src/x)"
	}
}
{
	"ImportPath": "m/b",
	...
	"Error": {
		"ImportStack": null,
		"Pos": "b/b.go:3:8",
		"Err": "package x is not in std (/home/austin/sdk/go1.21.6/src/x)"
	}
}

I'm not sure whether I expected it to report a package for "x" at all. If it did, that would presumably also need an error. The "x" package it currently reports is pretty weird because it's missing so many fields (like Dir). If it didn't report a package "x", it would probably have to omit "x" from the Imports and Deps lists, which I think would be fine.

@aclements
Copy link
Member Author

I spent quite a while figuring out why this happens this way. Clearly, cmd/go represents this as a single error object attached to package x, and thus can only attach a single line number to it (and "a" happens to win).

In the modload layer, modload.(*loader).resolveMissingImports calls modload.queryImport("x"), which returns a modload.ImportMissingError, which resolveMissingImports saves to x's modload.loadPkg.err field. This error is the one grain of sand that eventually turns into the pearl reported on package "x" and as deps errors on "a" and "b" by go list. Package "x"'s modload.loadPkg eventually gets stored in the modload.loaded.pkgCache.

In the load layer, load.PackagesAndErrors -> load.loadImport(..., "./a", ...) -> ... -> load.loadImport(..., "x", ...). This calls load.loadPackageData -> modload.Lookup(..., "x"), which fetches "x"'s load.loadPkg from the load.loaded.pkgCache and returns the err field. load.loadImport passes this error into load.(*Package).load -> load.setLoadPackageDataError, which finds the position of the import of "x" via the importPos stack, and wraps the modload.ImportMissingError in a load.PackageError. In this example, since we first came to "x" via "a", setLoadPackageDataError sets the load.PackageError.Pos field to "a"'s import, even though the PackageError itself is attached to x's load.Package object.

Later, we see load.loadImport(..., "./b", ...) -> ... -> load.loadImport(..., "x", ...), which finds "x"'s load.Package in load.packageCache, and thus doesn't enter load.(*Package).load, so the position in "a" remains attached to "x"'s load.Package.

This was all sufficiently complicated that I wasn't really sure where to change how the error gets attached to packages. If we don't want "x" to be represented at all in the go list output, then I think the modload layer should attach the modload.ImportMissingError to "a"'s and "b"'s modload.loadPkg and not create a loadPkg for "x" at all. Then I think the rest would "just work."

@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go labels Jan 29, 2024
@bcmills bcmills added this to the Backlog milestone Jan 29, 2024
@aclements
Copy link
Member Author

I missed a step before modload.(*loader).resolveMissingImports above. During modload.loadFromRoots, modload.(*loader).pkg and modload.(*loader).load together walk the import graph. Eventually, pkg is called for "x", which constructs a loadPkg for "x" and calls load. load calls importFromModules, and at the end of that function is a if mg != nil check that actually constructs and returns the ImportMissingError for x. This is the ImportMissingError that resolveMissingImports eventually picks up on.

@bcmills
Copy link
Contributor

bcmills commented Jan 30, 2024

This might be a duplicate of #26909.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

2 participants