Open mvdan opened 1 year ago
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 require X Y
and not just require (X Y; ...)
. It found one more, funnily enough, in gopls:
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.
I'm a bit skeptical that the problem will go away with time, as
1.17
was released over a year ago and the extra sections still pop up.
Note that the awkward transition happens when the go
directive in your go.mod
file crosses that boundary; it's more-or-less independent of upgrading the toolchain. (Current versions of go
still try to tidy according to the old format when in a module with an older go
version.)
Right. Note that the vocdoni example had been on go 1.17
for nearly a year by the time the duplicate section was introduced, so I think the duplication is an ongoing inconvenience regardless.
For what it's worth, it happened again in one of the projects mentioned above :) https://github.com/vocdoni/vocdoni-node/commit/ed5fb167e1a658b63318ec13b06fd65f14ca1aa5#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6
Can't share the specifics, but I just had an internal module end up with 5 require
blocks. I'm using Go 1.19.3 and the module has go 1.18
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.
After https://github.com/golang/go/issues/51983 I've just hit this once more, this time only using go get
and not go mod edit
in https://github.com/evcc-io/evcc/pull/5342/commits/c906d1dd48d92841fb3bcf9f0bcc1005f96cd3d4. We're already on Go 1.18.
+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 require
section, which presumably is a bug :) Sharing the reproducer script with the output below, hidden away as it's a bit long.
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, go mod tidy
will once more break it up into multiple sections. It does not only not merge them, it actually separates them.
I ran into this as well at work on a proprietary code base. We're in a situation where running go mod tidy
adds a third section and only when it is manually merged with the existing // indirect
section does go mod tidy
do the right thing.
It sounds like this is probably demonstrated by @mvdan's https://github.com/golang/go/issues/56471#issuecomment-1340095845 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 go mod tidy
should join multiple sections but that seems less important and more controversial than fixing this buggy behavior. After all, my workflow (and probably most others') mainly uses go mod tidy
to edit go.mod
, so if go mod tidy
isn't introducing new extraneous require
blocks to the file then there won't be multiple sections to merge in the first place.
I was a little bothered by this issue so until go mod tidy
does this by default a used the official modfile
parser to make my own tool: https://github.com/abhijit-hota/modfmt
I think opening a separate issue for go mod tidy
adding a third section is a good idea. I think it's a question whether we should preserve more than two require blocks in a file that already has more than two require blocks, but it's clearly a bug if go mod tidy
is adding an extra require block to a file that only has two require blocks.
I opened #67948 for the bug.
@mvdan I tried using your reproducer from the vocdoni/vocdoni-node repo but it doesn't seem to repro on master, and your branch go-mod-tidy-bug
seems to have been deleted. Fortunately I was able to find a public repro case among the issues linked to this one.
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 go mod tidy
on this results in three blocked, its not clear why but this is clearly broken.
Since https://github.com/golang/go/issues/45965,
go.mod
files now generally consist of two sections: onerequire (...)
section with direct module dependencies, and one for// indirect
module dependencies.However, if a
go.mod
file somehow ends up with more than two of those sections, thengo mod tidy
does 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; done
with 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:
1) Git merges. If two branches make changes to
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. 2) Manual editing. I've seen some users not fully grasp howgo get
works, and resorting to editinggo.mod
directly to update or addrequire
lines. When doing it quickly or copy-pasting, I imagine it's tempting to just add arequire some-module v1.2.3
at 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
require
sections 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 tidy
upgrade fromgo 1.16
togo 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.17
was released over a year ago and the extra sections still pop up. For example, that vocdoni PR was finalized in April 2022, and thego.mod
file in master was already ongo 1.17
since September 2021.I think
go mod tidy
should join these extra sections. The only reason I see that as potentially risky is if, in the future, another proposal like https://github.com/golang/go/issues/45965 comes along and we want more than two sections. But presumably that shouldn't be a problem, becausego mod tidy
already complains if ago.mod
file has ago X.Y
version that's too new.cc @bcmills @matloob @leitzler @seankhliao @dylan-bourque