cmd/go: strange attribution of missing imports

[aclements]

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:

Details

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]

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."

[aclements]

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]

This might be a duplicate of #26909.