cmd/go: 'go mod tidy' may delete comments on require blocks

[jayconrod]

What version of Go are you using (go version)?

$ go version
go version go1.17rc2 darwin/amd64

Does this issue reproduce with the latest release?

1.17+ only, in a module with go 1.17 or higher

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/jayconrod/Library/Caches/go-build"
GOENV="/Users/jayconrod/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/jayconrod/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/jayconrod/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/go/installed"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/go/installed/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.17rc2"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/jayconrod/Code/test/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/rq/x0692kqj6ml8cvrhcqh5bswc008xj1/T/go-build2401327528=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I have a go.mod file with some manual edits. A require block contains requirements are all either lacking an // indirect comment when they should have it, or vice versa. The require block itself has a comment.

go mod tidy moves all the requirements to a new block that does not have the comment above it.

go mod tidy
grep 'don''t delete me' go.mod

-- go.mod --
module test

go 1.17

// don't delete me
require (
	example.com/m v0.0.0 // indirect
)

replace example.com/m v0.0.0 => ./m
-- use.go --
package use

import _ "example.com/m"
-- m/go.mod --
module example.com/m

go 1.17
-- m/m.go --
package m

What did you expect to see?

In the example above, the // don't delete me comment is preserved.

What did you see instead?

The new go.mod file does not contain the // don't delete me comment.

module test

go 1.17

require example.com/m v0.0.0

replace example.com/m v0.0.0 => ./m

This happens in modfile.SetRequireSeparateIndirect.

I think it makes sense to delete the block-level comments if the require directives are moved to other existing blocks. However, if all of the requirements in a block are moved to a new block, and the old block is delete, the new block should inherit the comments from the old block.

cc @bcmills @matloob

[gopherbot]

Change https://golang.org/cl/340349 mentions this issue: mod/modfile: 'go mod tidy' may delete comments on require blocks

[bcmills]

However, if all of the requirements in a block are moved to a new block, and the old block is [deleted], the new block should inherit the comments from the old block.

What happens if we also add other new modules in the same block, or delete some of the modules from the original block and retain the rest? If we add other new ones, we have no way to know whether the comment applies to them. I guess deletion should theoretically be ok, though.

But what if the comment says something like these indirect dependencies are only needed for example.com/bar, and they're being moved from the indirect section to the direct one? Then preserving the comment on the new block would be equally wrong.

Honestly, I would rather we just retain the empty require block and its comment and let the user decide what to do about it when they read the diff (https://play.golang.org/p/-jeTQAvJ9Ll).

module test

go 1.17

// don't delete me
require (
)

require example.com/m v0.0.0

replace example.com/m v0.0.0 => ./m

[bcmills]

Or, another option might be to avoid moving a dependency between blocks if its existing require block has a block-level comment.

That would potentially introduce mixed-dependency blocks where they were previously single-kind, but would always preserve the block comments that apply to a given dependency — even if its classification changes, and even if other requirements are added or removed in the same block at the same time.

[bcmills]

On balance, I think I prefer the latter approach. Let's just keep a dependency in the same require block it was already in if that block has a comment.

[bcmills]

Similarly, perhaps we shouldn't add new dependencies into an existing block with a block comment.

[jayconrod]

I like the simplicity of not moving directives into or out of require blocks with comments and not deleting blocks with comments that become empty. These are all going to be custom situations with hand edits, and it's hard to pick a behavior that will work everywhere, so let's go with something simple.

[korzhao]

@bcmills In this case,What do we expect to see?

module m
// save
require (
    x.y/a v1.2.3 
)
require (
    x.y/a v1.2.3 // comments
)

A:

module m
// save
require (
)
require x.y/a v1.2.3 // comments

B:

module m
// save
require x.y/a v1.2.3

C:

module m
// save
require x.y/a v1.2.3 // comments

D:

module m
require x.y/a v1.2.3 // comments

[gopherbot]

Change https://golang.org/cl/343431 mentions this issue: modfile: in SetRequireSeparateIndirect, arrange requirements consistently

[gopherbot]

Change https://golang.org/cl/343432 mentions this issue: cmd/go: write go.mod requirements more consistently for go 1.17+