cmd/go: mod tidy should join "require" sections if there are more than two #56471
Comments
|
A slightly better CS regular expression is https://cs.opensource.google/search?q=path%3Ago.mod+AND+pcre%3Ayes+AND+%28require%5C+%28.%7C%5Cn%29*require%5C+%28.%7C%5Cn%29*require%5C+%29+&sq=, which also matches |
|
see also #51983 |
|
Properly handling comments could indeed be some work. I'm honestly not worried about it, because in all the instances I've run into this problem, there were no custom comments in any of the require sections. So I'd be fine if we start by only joining require sections without any comments inside or between them, for example. |
heschi
added
the
NeedsDecision
Note that the awkward transition happens when the |
|
Right. Note that the vocdoni example had been on |
|
For what it's worth, it happened again in one of the projects mentioned above :) vocdoni/vocdoni-node@ed5fb16#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6 |
|
Can't share the specifics, but I just had an internal module end up with 5 |
After #51983 I've just hit this once more, this time only using +1 for automatic fix. When I call tidy I mean tidy ;) |
|
@bcmills I finally ran into a reproducible way to get cmd/go to add a third I'm sure the bug can be reproduced without Docker and with a smaller Go module, but I hope that's enough for you to identify the bug. I know this thread is about joining the sections, but this bug feels very much related, and perhaps it's another item to add to the list of reasons why we should join the sections :) |
|
Consider the attached go mod. Even when I manually join the three require sections, |
* run tf-sdk-migrator v2upgrade * schema.Removed removed in v2, change to deprecated https://developer.hashicorp.com/terraform/plugin/sdkv2/guides/v2-upgrade-guide#removal-of-helper-schema-schema-removed * SetPartial was removed in v2 https://developer.hashicorp.com/terraform/plugin/sdkv2/guides/v2-upgrade-guide#removal-of-helper-schema-resourcedata-setpartial * ResourceProvider removed in v2 https://developer.hashicorp.com/terraform/plugin/sdkv2/guides/v2-upgrade-guide#removal-of-the-terraform-resourceprovider-interface * go/storage@v1.27.0 was retracted go: warning: cloud.google.com/go/storage@v1.27.0: retracted by module author: due to googleapis/google-cloud-go#6857 go: to switch to the latest unretracted version, run: go get cloud.google.com/go/storage@latest * update plugin/v2 deps and manually merge require sections open bug about require sections: golang/go#56471 * publish image needs fields for computed resources * set default for file mode. ensure proper string-int conversion the lxd client api works in ints, but mode is a string for our users * use set-specific attribute test function * simplify provider tests
|
I ran into this as well at work on a proprietary code base. We're in a situation where running It sounds like this is probably demonstrated by @mvdan's #56471 (comment) so I won't try to construct my own reproducer for now. Should we open a separate issue for this since this is just some kind of straight-up bug? I agree with the proposal title that |
|
I was a little bothered by this issue so until |
|
I think opening a separate issue for |
|
I found this while trying to use golang.org/x/mod/modfile to add new requires to a go.mod file. It's behaviour is to always add requires to last block, which typically contains indirect requires, instead of adding to the correct block depending if it was an indirect or not. Running |
|
So we spent some time looking at #67948, which provided a case that I think could result in a lot of confusion:
I think we need to figure out if there are any reasons why users would want three sections, and if there aren't, we can proceed with this proposal. I'm not sure how to prove there aren't reasons users would want three sections, but we could perhaps do an analysis of |
|
Confirmed the above go mod init github.com/stevenh/mod-test
# Result: Basic go.mod
go mod edit -require github.com/stretchr/testify@v1.9.0
# Result: Single block with direct
cat > mod_test.go << EOF
package main
import _ "github.com/stretchr/testify/require"
EOF
go mod tidy
# Result: Two blocks first directs and second for indirect's.
go mod edit -require github.com/testcontainers/testcontainers-go@v0.33.0
# Result: direct added to second block (now mixed)
cat > mod_test.go << EOF
package main
import (
_ "github.com/stretchr/testify/require"
_ "github.com/testcontainers/testcontainers-go"
)
EOF
go mod tidy
# Result: Second block is now mixed
go mod tidy
# Result: Three blocks, directs, indirects and mixed. |
Since #45965,
go.modfiles now generally consist of two sections: onerequire (...)section with direct module dependencies, and one for// indirectmodule dependencies.However, if a
go.modfile somehow ends up with more than two of those sections, thengo mod tidydoes not join them back. I think it should. For example, if I join the normal two sections into one,go mod tidy, splits them again.Here are two instances I recently found of such an unintentional "split" of sections:
Both were spotted manually and fixed, as they were indeed unintentional. Locally, I used
for f in **/go.mod; do n=$(grep -F 'require (' $f | wc -l); if [[ $n -gt 2 ]]; then echo $n $f; fi; donewith Bash to find any others. I couldn't find any.Globally, I tried using cs.github.com, but it doesn't appear to support matching regular expressions across multiple lines, like PCRE. But Googe's code search does; https://cs.opensource.google/search?q=path%3Ago.mod+AND+pcre%3Ayes+AND+%28require%5C+%5C%28%28.%7C%5Cn%29*require%5C+%5C%28%28.%7C%5Cn%29*require%29+ found:
None of those four appear to be on purpose. I think they could have appeared unintentionally due to a number of reasons, such as:
go.mod, depending on how the user resolves the conflicts manually, they could end up with more than two sections. I'm fairly sure that this is what happened in vocdoni-node, as the PR in question lived for over a month and had conflicts to be resolved.go getworks, and resorting to editinggo.moddirectly to update or addrequirelines. When doing it quickly or copy-pasting, I imagine it's tempting to just add arequire some-module v1.2.3at the end of the file, which will work regardless of what the file looks like.I personally can't think why anyone would want more than two
requiresections today. The fact that I could only find four examples today in ten minutes of research is a double-edged sword. On one hand it's proof that basically noone wants more than two sections. On the other, it also means that this problem appears rarely, so it might not be a high priority as an improvement forgo mod tidy.Still, I have encountered this problem myself twice now, and that's the magic number that tells me I should file a bug and propose an automated fix. This topic has been brought up a number of times on Slack (January 2022, February 2022, October 2022), so there are at least a couple of other people experiencing the problem and fixing it manually.
In the second of those Slack threads, @bcmills mentions that this might be a quirk with the
go mod tidyupgrade fromgo 1.16togo 1.17. It could be that some of the third sections came about that way; it's hard to tell for sure. I'm a bit skeptical that the problem will go away with time, as1.17was released over a year ago and the extra sections still pop up. For example, that vocdoni PR was finalized in April 2022, and thego.modfile in master was already ongo 1.17since September 2021.I think
go mod tidyshould join these extra sections. The only reason I see that as potentially risky is if, in the future, another proposal like #45965 comes along and we want more than two sections. But presumably that shouldn't be a problem, becausego mod tidyalready complains if ago.modfile has ago X.Yversion that's too new.cc @bcmills @matloob @leitzler @seankhliao @dylan-bourque
The text was updated successfully, but these errors were encountered: