x/mod/modfile: File.Cleanup breaks references so future edits do nothing

[jameshartig]

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

$ go version
go version go1.16.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.16.1"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/app/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 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3582639095=/tmp/go-build -gno-record-gcc-switches"

What did you do?

https://play.golang.org/p/PQwdaFv3Pgp

When calling *File.Cleanup it ends up causing future edits to do nothing. If this is intended the method description should be updated to make it apparent that you shouldn't do any other edits after calling it.

We have a script that parses the go.mod in a project and updates all of the requires and then updates the replacements. If a requires ends up invalidating a replacement then we drop the replacement and then need to call Cleanup before looping over *File.Replace since there might be empty entries in there now.

What did you expect to see?

I expected to see

=================== runTest(withMiddleCleanup = true) ===================
contents okay
=================== runTest(withMiddleCleanup = false) ===================
contents okay

Which means that Cleanup does not break future edits.

What did you see instead?

Instead I saw:

=================== runTest(withMiddleCleanup = true) ===================
unexpected contents
module test

go 1.16

require test.com/test v1.0.0

replace test.com/test => test.com/test2 v1.0.1

=================== runTest(withMiddleCleanup = false) ===================
contents okay

[cherrymui]

cc @bcmills @jayconrod @matloob

[bcmills]

If this is intended the method description should be updated to make it apparent that you shouldn't do any other edits after calling it.

IMO it is not (or shouldn't be) the intended behavior — that seems like it would be too much of a trap, for no clear benefit.

I think this just needs to be fixed. Want to send a CL?

[jameshartig]

I've narrowed it down to *FileSyntax.Cleanup and only when it hits the collasping case:

if ww == 1 {
	// Collapse block into single line.
	line := &Line{
		Comments: Comments{
			Before: commentsAdd(stmt.Before, stmt.Line[0].Before),
			Suffix: commentsAdd(stmt.Line[0].Suffix, stmt.Suffix),
			After:  commentsAdd(stmt.Line[0].After, stmt.After),
		},
		Token: stringsAdd(stmt.Token, stmt.Line[0].Token),
	}
	x.Stmt[w] = line
	w++
	continue
}

Just modifying stmt.Line[0] should fix this. I'll work on a CL

[gopherbot]

Change https://golang.org/cl/303234 mentions this issue: modfile: fix Cleanup clobbering Line reference

[jameshartig]

@bcmills can I get a review on https://golang.org/cl/303234? Thanks!