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: list -export -e sets the Error field but keeps Incomplete false #57724

Closed
mvdan opened this issue Jan 10, 2023 · 3 comments
Closed

cmd/go: list -export -e sets the Error field but keeps Incomplete false #57724

mvdan opened this issue Jan 10, 2023 · 3 comments
Labels
GoCommand cmd/go help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mvdan
Copy link
Member

mvdan commented Jan 10, 2023

I tracked this down after a tool of mine (which calls go list) was choking on https://github.com/bytedance/sonic.

With that repo cloned, here's the reproducer:

$ go version
go version devel go1.20-0202ad0b3a Mon Jan 9 22:50:08 2023 +0000 linux/amd64
$ git switch -d 67cffb15bd786099af68ac82e545f12f2eedfb16
HEAD is now at 67cffb1 feat:(ast) add  fallback api on `not-amd64` env (#341)
$ go build ./internal/loader
# github.com/bytedance/sonic/internal/loader
internal/loader/funcdata.go:27:22: undefined: _ModuleData
internal/loader/funcdata.go:30:27: undefined: _ModuleData
internal/loader/funcdata.go:41:6: undefined: _Func
internal/loader/funcdata.go:42:12: undefined: _ModuleData
internal/loader/funcdata.go:53:16: undefined: _ModuleData
internal/loader/funcdata.go:79:26: undefined: _ModuleData
internal/loader/funcdata_invalid.go:23:6: panic("Unsupported Go version. Supported versions are: 1.15, 1.16, 1.17, 1.18, 1.19") (no value) used as value
internal/loader/loader.go:47:50: too many arguments in call to registerFunction
	have (string, uintptr, uintptr, int, int, uintptr, uintptr, uintptr)
	want (string, uintptr, int, int, uintptr)
$ go list -export -e -f '{{.ImportPath}} -- {{.Incomplete}} -- {{.Error}}' ./internal/loader
github.com/bytedance/sonic/internal/loader -- false -- # github.com/bytedance/sonic/internal/loader
internal/loader/funcdata.go:27:22: undefined: _ModuleData
internal/loader/funcdata.go:30:27: undefined: _ModuleData
internal/loader/funcdata.go:41:6: undefined: _Func
internal/loader/funcdata.go:42:12: undefined: _ModuleData
internal/loader/funcdata.go:53:16: undefined: _ModuleData
internal/loader/funcdata.go:79:26: undefined: _ModuleData
internal/loader/funcdata_invalid.go:23:6: panic("Unsupported Go version. Supported versions are: 1.15, 1.16, 1.17, 1.18, 1.19") (no value) used as value
internal/loader/loader.go:47:50: too many arguments in call to registerFunction
	have (string, uintptr, uintptr, int, int, uintptr, uintptr, uintptr)
	want (string, uintptr, int, int, uintptr)

Note how the package fails to build (they don't support tip, aka 1.20), and the Error field is set to the error message, but Incomplete is still false.

From the docs:

        // Error information
        Incomplete bool            // this package or a dependency has an error
        Error      *PackageError   // error loading package
        DepsErrors []*PackageError // errors loading dependencies

Unless I'm misreading the docs, Incomplete needs to be true in this case.

I see that, in cmd/go/internal/load, there are many places where Error and Incomplete are set. It sounds very likely to me that one of those set Error while forgetting to set Incomplete. Could we not set Incomplete at the very end of loading, based on whether Error or DepsErrors are not empty? That would help ensure the documented invariant, reduce code, and avoid bugs like these.

cc @bcmills @matloob

@bcmills bcmills added GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. help wanted labels Jan 10, 2023
@bcmills bcmills added this to the Backlog milestone Jan 10, 2023
@bcmills
Copy link
Contributor

bcmills commented Jan 10, 2023

Agreed, Error !=nil || len(DepsErrors) > 0 should imply Incomplete == true.

Is this a recent regression, or a long-standing bug?

@mvdan
Copy link
Member Author

mvdan commented Jan 10, 2023

The particular package only fails to load with Go master, so it's not trivial to reproduce on 1.19.4 :) I haven't reduced this to an input that (presumably) fails on stable.

@gopherbot
Copy link

Change https://go.dev/cl/536418 mentions this issue: cmd/go/internal/work: set Incomplete to true if there is an error

@dmitshur dmitshur modified the milestones: Backlog, Go1.22 Oct 20, 2023
@dmitshur dmitshur 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 Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants