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.

Sign up for GitHub

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

Jump to bottom

x/tools/godoc/vcs/zipfs: New can't be called with a *zip.Reader #66117

Open
riconnon opened this issue Mar 5, 2024 · 3 comments
Open

x/tools/godoc/vcs/zipfs: New can't be called with a *zip.Reader #66117

riconnon opened this issue Mar 5, 2024 · 3 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@riconnon
Copy link

riconnon commented Mar 5, 2024

Go version

go version go1.22.0 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE='auto'
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/richardcrovern/Library/Caches/go-build'
GOENV='/Users/richardcrovern/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/richardcrovern/pkg/mod'
GONOPROXY='github.com/monzo'
GONOSUMDB='github.com/monzo'
GOOS='darwin'
GOPATH='/Users/richardcrovern'
GOPRIVATE='github.com/monzo'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.22.0/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.22.0/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD=''
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 -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/ds/9nyxj91138s74kvd12pgpjbc0000gn/T/go-build3692657355=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Construct a zip.ReadCloser from an io.ReaderAt suitable for use with zipfs.New from golang.org/x/tools/godoc/vfs/zipfs as follows:

zip, err := zip.NewReader(reader, size)
if err != nil {
  panic(err)
}
closableZip := &zip.ReadCloser{Reader: *zip}

See https://go.dev/play/p/hFPUG1CYhdH for a complete program example

What did you see happen?

go vet reports the following error:

./prog.go:20:40: literal copies lock value from *zipReader: archive/zip.Reader contains sync.Once contains sync/atomic.Uint32 contains sync/atomic.noCopy

What did you expect to see?

The finding by go vet is legitimate, in that a sync.Once is copied here, but I don't believe there is any way to construct the zip.ReadCloser and not get this error.
@bcmills
Copy link
Contributor

bcmills commented Mar 5, 2024

The Reader returned by zip.NewReader does not need to be closed (and cannot be closed): it is assumed that the caller will close the underlying io.ReaderAt when it is no longer needed. If you need to add a no-op Close method, you can either use io.NopCloser or embed the type in a struct that provides the Close method: https://go.dev/play/p/ypFIx7bRucQ

In this case, it seems that the root of the problem is an API defect in zipfs.New: it hard-codes the *zip.ReadCloser type when it really ought to accept only a *zip.Reader and leave the Close operation up to the caller. (Notably, the zipfs package only calls the Close method in its own zipFS.Close method, which is on an unexported type!)

That could probably be remedied by adding a second variant of the zipfs.New function that accepts a *zip.Reader instead of a *zip.ReadCloser.

@bcmills bcmills changed the title archive/zip: Can't construct a zip.ReadCloser without tripping over go vet x/tools/godoc/vcs/zipfs: New can't be called with a *zip.Reader Mar 5, 2024
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Mar 5, 2024
@gopherbot gopherbot added this to the Unreleased milestone Mar 5, 2024
@bcmills
Copy link
Contributor

bcmills commented Mar 5, 2024

In the meantime, the only workaround I can see is to write the contents of your zipfile to a temporary file so that you can open that using zip.OpenReader. It's awkward, but it would at least get you unblocked.

@riconnon
Copy link
Author

riconnon commented Mar 5, 2024

Thanks for the info @bcmills

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@mknyszek @riconnon @bcmills @gopherbot