proposal: cmd/go: unused (yet) dependencies added by go get should not be indirect #68593
Comments
seankhliao
changed the title
go get on go.mod should match user intent
|
I've seen projects on Github where the dependencies are incorrectly marked as indirect, presumably for this reason. |
|
I'm not sure what all the implications of doing this are. You're right that since the I would like to see if we can first try to find an alternate solution to the bzlmod the right set of direct dependencies. For example, if it's too expensive to run I'm curious to know how long |
A tool that is based on this assumption would seem very brittle: Since I nonetheless agree that we should take the potential implications on backwards compatibility seriously. If we otherwise arrive at the conclusion that this behavior would be beneficial to users in general (not just users of rules_go), could gating this behind
Could you elaborate on the
In a large monorepo with >2,000 Go module deps, the logic that translates |
I think we would use those if we wanted to experiment with the behavior change without committing to it. But I think what we'd end up doing would be to keep the old behavior for modules declaring, say, go version 1.23 or older, and only change the behavior for 1.24 or later. I think we'd ultimately get the same level of safety, but any people or tools depending on the old behavior would have to adjust their expectations for (say) 1.24 or later modules.
The Take the following directory as an example (it's in txtar format: use golang.org/x/exp/cmd/txtar --extract to write the files to disk so you can try it out) If you run So
I see. I can understand the need for something faster. I'd like to see first if we can find a solution without changing the behavior of |
Thanks for the explanation and the example! This could be a fast replacement for
We could possibly use this for a workaround that is similar to what Downsides would be that our "transparent wrapper around |
|
They are indirect because there are no references to them yet. This is an accurate recording of the facts. If you want them to not be indirect, add the imports first and then use 'go get' with no arguments to scan the current directory's source code and resolve its imports. Then it will see that the imports are used and mark them direct. Or else run 'go mod tidy' after writing your code. Both of those will also end up with an accurate recording of the facts. This is not something we should change. |
I think a better description of the state of the world than the import being "indirect" is that it was "manual". It's not a normal indirect import where A imports B. It's C was added manually but isn't needed (yet). |
If we follow this line of argument to the end, wouldn't that mean that
Adding just the imports is tricky since IDEs (both VSCode with gopls and GoLand) tend to remove imports that aren't used in a file on save (or explicit format action). So this would require adding both an
Compared to that, it would be more convenient to just type out the Which leads to my most important question: Why should a user interactively run the
I like this point of view. If adding the new dep as a direct require seems inappropriate, perhaps it could be marked as |
|
I still don't think we should make changes here. Manually added dependencies can be considered indirect, meaning they are not directly used by the source code. If you want to specify a specific version, you can still add the import where you plan to use it and then run |
|
This proposal has been added to the active column of the proposals project |
|
Based on the discussion above, this proposal seems like a likely decline. |
|
No change in consensus, so declined. |
**What type of PR is this?** Feature **What does this PR do? Why is it needed?** When running `go get example.com/foo@v1.2.3`, the module `example.com/foo` will be added to `go.mod` with an `// indirect` comment, just like its transitive dependencies. This results in `go_deps` not being able to distinguish this explicitly requested new dep from other indirect deps that shouldn't be imported by the root module. Running `go mod tidy` or `go get` without arguments is required to have the comment removed after adding a reference to the new module in code. This change makes it so that `bazel run @rules_go//go get example.com/foo@v1.2.3` marks the requested module as a direct dependency. This realizes golang/go#68593 in our custom wrapper, thus making this command the only one needed to add a new module dependency and have it work with a subsequent Bazel build (assuming that also updates the BUILD files with Gazelle). This [matches the behavior of `gopls` when adding a new dependency](https://github.com/golang/tools/blob/ec1a81bfec7cc6563474b7aa160db30df9bfce6b/gopls/internal/server/command.go#L805-L809). --------- Co-authored-by: Son Luong Ngoc <sluongng@gmail.com>
Proposal Details
(In the following,
$packagestands for some package in a Go module$moduleat version$version.)When a developer runs
go get $package@$versionto add a new dependency to their module, the result is that the module'sgo.modfile is updated withrequiredirectives for$module@$versionas well as the transitive dependencies of the given package, with all of theserequires being marked with// indirect.While this seems appropriate for the transitive dependencies, the outcome for the
require $module $versionis not what the user intended:$packagein their code, they will have introduced a direct dependency on$module@$versionwithout this being reflected ingo.mod.$package(or any other package in$module), therequiredirectives for$module@$versionand all its new transitive dependencies are unnecessary.In either case, a command such as
go mod tidyorgo get -t ./...is necessary to get thego.modfile into a consistent state. In fact, thego.modfile after runninggo get $package@$versionis, due to the// indirectcomment on the newrequiredirective for$module@$version, usually not in a state that could ever be produced bygo mod tidy.This is problematic for external tooling that relies on the information in
go.modto determine the set of dependencies for a module in a lightweight way (compared to the more heavyweight and over-approximatinggo list -m all).goplsexplicitly works around this behavior by manually runninggo mod edit -require $module@$versionto ensure that therequiredirective for$module@$versionis not marked as// indirect.Proposal
When a user runs
go get $package@$version(or any other package pattern), therequiredirective for$module@$versionshould not be marked as// indirect. Since adding a usage of the package to the code and runninggo mod tidywould have the same effect, this change should not break any existing workflows or tooling.A more conservative approach would be to gate this behavior behind a flag (say
-rfor "require").Context
While I believe that this issue is generic to all users of
go get, especially those that aren't also usinggopls, there is a specific use case that motivated me to write it up.The
rules_goBazel ruleset uses Bazel's new dependency management system Bzlmod to directly translate the information in a project'sgo.modandgo.sumfiles into Bazel dependencies, with each Go module corresponding to a Bazel repository.Since external dependencies are subject to "strict visibility", each Bazel module needs to declare all its direct dependencies. For this reason, the
// indirectcomment ingo.modis used as a signal torules_gothat a dependency is not direct, avoiding build breakages caused by changes to transitive dependencies of updated direct dependencies. Due to the particular behavior ofgo get, this makes it very difficult for users to add new dependencies to their Bazel-managed Go project without also adding a usage in code and runninggo mod tidy, which can be slow in large monorepos.If
go getdidn't mark the explicitly requested modules as// indirect, this setup would "just work".The text was updated successfully, but these errors were encountered: