x/mod/modfile: AddNewRequire doesn't put direct dependencies in the first block

[stevenh]

Go version

go version go1.23.0 linux/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/steveh/.cache/go-build'
GOENV='/home/steveh/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/steveh/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/home/steveh/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.23.0'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/home/steveh/.config/go/telemetry'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/steveh/code/github.com/rocketsciencegg/congestion-control/tools/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-build155249492=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Use AddNewRequire to add new requires.

package main

import (
    "testing"

    "github.com/stretchr/testify/require"
    "golang.org/x/mod/modfile"
    "golang.org/x/mod/module"
)

var testFile = `module test

go 1.23.0

require (
    github.com/test/test1 v0.1.0
    github.com/test/test2 v0.1.0
)

require (
    github.com/test/test3 v0.1.0 // indirect
)
`

var expected = `module test

go 1.23.0

require (
    github.com/foo/direct1 v0.1.1
    github.com/foo/direct2 v0.1.2
    github.com/test/test1 v0.1.0
    github.com/test/test2 v0.1.0
)

require (
    github.com/foo/indirect1 v0.2.1 // indirect
    github.com/foo/indirect2 v0.2.2 // indirect
    github.com/test/test3 v0.1.0 // indirect
)
`

func TestAddRequire(t *testing.T) {
    file, err := modfile.Parse("go.mod", []byte(testFile), nil)
    require.NoError(t, err)

    file.AddNewRequire("github.com/foo/indirect2", "v0.2.2", true)
    file.AddNewRequire("github.com/foo/direct2", "v0.1.2", false)
    file.AddNewRequire("github.com/foo/direct1", "v0.1.1", false)
    file.AddNewRequire("github.com/foo/indirect1", "v0.2.1", true)

    file.Cleanup()
    file.SortBlocks()

    data, err := file.Format()
    require.NoError(t, err)
    require.Equal(t, expected, string(data))
}

What did you see happen?

Direct requires should be added to first require block, indirect requires should be added to second block.

What did you expect to see?

When using AddNewRequire, requires are added to the last block. This is the documented behaviour, but other go tools such as go mod tidy maintain two blocks, so this use the same approach.

It seems possible to use SetRequireSeparateIndirect to replicate the desired behaviour but that was far from obvious.

If nothing else reference in AddNewRequire and Addequire to SetRequireSeparateIndirect would help users fine the right functionality, but ideally AddNewRequire and AddRequire should function as expected.

If a user isn't aware of the two block rule then using this will result in a file that go mod tidy handles badly, in some cases resulting in three blocks instead of two.

[gabyhelp]

Related Issues and Documentation

• cmd/go: `mod tidy -go=1.17` should move indirect dependencies to the second require part #47733 (closed)
• cmd/go: 'mod tidy' sometimes adds new, unnecessary require sections #67948
• cmd/go: separate direct and indirect dependencies in go.mod #27887 (closed)
• cmd/go: 'go mod tidy' may delete comments on require blocks #47563 (closed)
• cmd/go: mod tidy should join "require" sections if there are more than two #56471
• import/path: issue title more than require blocks in go.mod #68303 (closed)
• cmd/go: mod edit doesn't respect direct/indirect blocks in go.mod #51983 (closed)
• cmd/go: go directive is not only added during go mod init, but also under other conditions that are hard to deduce #30790 (closed)
• cmd/go: 'mod tidy' chooses points in requirement cycles that differ from other subcommands #34086 (closed)
• x/tools/gopls: missing direct requirements also listed in go.mod diagnostics #40470 (closed)

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[seankhliao]

See #68593, just parsing the modfile doesn't provide enough information for if a module should be direct or indirect, and the default addition is to mark it as indirect.

[stevenh]

See #68593, just parsing the modfile doesn't provide enough information for if a module should be direct or indirect, and the default addition is to mark it as indirect.

I'm not sure you correctly understood what I reported, AddNewRequire takes the indirect parameter to specify if the caller wants a direct or indirect, which it does correctly.

What it's not doing is adding it to the correct block.

[seankhliao]

Ah sorry.

[stevenh]

FYI I have some code which I believe fixes the behaviour, just need to get a PR created from it.

[gopherbot]

Change https://go.dev/cl/608056 mentions this issue: modfile: fix AddNewRequire block ordering

[matloob]

AddNewRequire is documented to add the require to the last block and I think we should be careful when making changes to behavior like this.

The go command only creates the two blocks for go modules declaring go 1.17 or later. We should make sure that we don't change that behavior.

Take a look at the code here: src/cmd/go/internal/modload/init.go#1846 The go command decides to call SetRequire or SetRequireSeparateIndirect based on the module's goVersion.

[stevenh]

Thanks for the link, to clarify are users of modfile expected to use SetRequireSeparateIndirect to add requires instead of AddRequire / AddNewRequire @matloob?

[matloob]

SetRequire and SetRequireSeparateIndirect are meant to be used by the go command.

I think users that need to manually edit a go.mod file should use AddRequire but then run go mod tidy to fix the go.mod file. The direct and indirect values aren't always updated until a tidy is done.

We should fix the problems with tidy: Tidy shouldn't split a two-group go.mod file into three.

[stevenh]

To confirm my understanding, AddRequire and AddNewRequire should ideally maintain the status?

If so this is what exactly https://github.com/golang/mod/pull/21 targets to do. However it does always look to create two groups so if we want it to only create one per 1.17 then it would need tweaking.

[matloob]

I don't think AddRequire should maintain the // indirect blocks. go mod tidy should do that.

It should be okay to had AddRequire add an // indirect requirement to the third block and then have go mod tidy figure out where it belongs.

[stevenh]

The problem with that is it means you add a dependency on the go binary, the reason for using modfile is to avoid that, otherwise you could just exec go get, go mod edit --replace.

If modfile.Cleanup() corrected the order that would be fine though.

For reference the use case I'm working with is automating the correction of modules which create plugins, which have the requirement that they use identical module versions, specifically for krakend, which is typically used in a docker container that doesn't have go.

[matloob]

We expect the go command to be used to tidy a go.mod file before it's released. That's the main supported use case. We provide the x/mod library to allow programatic modification of the go.mod file, but it isn't meant to produce tidy go.mod files.

If the edited modules are being released I'd strongly recommend running go mod tidy before releasing them.

I think for what you're trying to your best bet is probably to use SetRequireSeparateIndirect to set the requires in the file and sort the requires into direct and indirect blocks. But you should be aware that there may be more work required to produce a tidy go.mod file`.

[stevenh]

Totally agree that go mod tidy is still needed, in our use case this is done as a separate step outside of the container that is making the initial changes. The problem is go mod tidy doesn't correct the output from AddNewRequire.

We have already made the switch to SetRequireSeparateIndirect for now but I think this should still be fixed.

From a users perspective the AddNewRequire is the most obvious way add a require, and it should behave correctly, specifically it should leave the file in a state that go mod tidy can correct, which is not currently the case. Having two ways to do things is fine, but they should both have a valid output.

If SetRequireSeparateIndirect is the new way, deprecating AddNewRequire would at least make it obvious to the user, but as AddNewRequire is an better match for the typical use case of "adding a specific require" I would suggest the ideal solution would be to make AddNewRequire and SetRequire result in the expected layout, and deprecate SetRequireSeparateIndirect as it would become unnecessary.

In short all methods should result in a file that would require minimal changes that go mod tidy will handle.

Thoughts?

[matloob]

I agree that the methods should result in a file that go mod tidy will handle. We should fix go mod tidy to move direct dependencies that appear in the indirect block back into the same block as the other dependencies.

But I think it's okay that AddNewRequire places direct requirements in the second "indirect" block.

SetRequireSeparateIndirect essentially exists to be used by go mod tidy, so it's not something we want to push people to. And we don't want to deprecate AddNewRequire.

We do plan to fix the go mod tidy issue where it doesn't keep the two blocks.