search

golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
122.82k stars 17.51k forks source link

cmd/go: flags to control changes to go.mod, go.sum #34506

Closed jayconrod closed 4 years ago

jayconrod commented 4 years ago

Proposed changes

I propose we add three new flags to go subcommands that deal with modules.

These flags would be allowed when GO111MODULE is set to on or auto (the current default) and rejected when GO111MODULE is set to off. -modfile and -sumfile both require an actual go.mod to be present, so modules must be enabled when they're used. -g does not require an actual go.mod to be present, and in auto mode, it implies that modules are enabled.

Background

The go command updates go.mod and go.sum after any command that needs to find a module for a package not provided by any module currently in the build list. This ensures reproducibility: if you run the same command twice, it should build (or list or test) the same packages at the same versions, even if new versions have been published since the first invocation.

For example, if you run go build ., and a .go file in the current directory imports example.com/m/pkg which is not provided by any known module, the go command will add a requirement on the latest version of the module example.com/m to go.mod. Future runs of go build . will produce the same result.

While these updates are usually helpful, there are many situations where they're not desirable.

Selected issues

gopls (various issues)

gopls loads information about source files in a workspace using golang.org/x/tools/go/packages, which invokes go list. gopls may also run go list directly. In either case, gopls may trigger changes to go.mod and go.sum. This may be caused by user actions that seem unrelated to building anything, for example, opening a file. go.mod appears to change mysteriously on its own, and users don't realize gopls is triggering it.

It's not usually important that the information gopls loads is reproducible; files it operates on are frequently changing. However, it is important that when it resolves an unknown import path to a module, it doesn't need to do so repeatedly since this can add a lot of latency, especially on slow connections.

gopls could set -modfile and -sumfile to temporary copies of the original go.mod and go.sum. The original go.mod and go.sum would not be modified (until the user explicitly runs a command like go build). Resolved module requirements would stay in the temporary files so they would not need to be resolved again.

25922 - clarify best practice for tool dependencies

Developers need a way to express module requirements that aren't implied by package imports. This is especially useful for tools invoked by go generate. Authors can add tool requirements to go.mod manually or with go get, but these requirements are erased by go mod tidy.

The current recommendation is to create a tools.go file, tag it with // +build tools, then import main packages of needed tools. tools.go will never be built because of the tag, but go mod tidy will read the imports and preserve the requirements. This feels like a hacky workaround rather than a best practice. It also pushes requirements which may not otherwise be needed on downstream modules.

A better solution would be to keep a separate go.tools.mod file with tool requirements, then point to that with -modfile=go.tools.mod when running commands that require tools.

26640 - allow go.mod.local to contain replace/exclude lines

This is a feature request to keep some go.mod statements out of source control. It's frequently useful for module authors to check out dependencies and point to them with replace statements for local development and debugging. These statements shouldn't necessarily be exposed to users or other developers on the same project though.

Setting -modfile=go.local.mod and -sumfile=go.local.sum would solve this problem, at least partially. The two files could be copied from the regular go.mod and go.sum files and added to .gitignore. Note however, that these local files are used instead of the regular files, not in addition to, so some synchronization might be required.

30515 - offer a consistent "global install" command

Developers want to be able to install tools from any directory, regardless of the requirements of the current module. go get tool@version may update the current go.mod, so users need to change to a temporary directory without a go.mod file to run commands like this. Tool authors need to be careful when writing installation instructions because of this.

The -g flag would address this issue. It would tell the go command to run as if it were outside any module. Tool authors could write go get -g tool@latest in their installation instructions: this would install the latest version of the tool, regardless of the current directory.

Note that "missing go.mod" is being reconsidered (#32027), so the actual semantics of -g may change: this issue is just about ignoring the current module.

33710 - module mode removes concept of global docs

In module mode, go doc example.com/pkg prints documentation for the packages named on the command line at the same version they would be built with. Like go build, go doc may add or update requirements in go.mod. This may be undesirable, especially if you're using the documentation to decide whether you want to depend on a package that is not currently imported.

The -g flag would partially solve this. The current module would be ignored, and "global" documentation would be shown.

Note that go doc does not currently work in "missing go.mod" for packages outside std. #33710 would need to be fixed, but -g would provide a useful way to access that mode.

Other related issues

There are a large number of open issues about unexpected and unwanted go.mod changes. The flags suggested here won't solve all these problems, but they provide useful context.

jayconrod commented 4 years ago

cc @bcmills @rsc @stamblerre @ianthehat

bcmills commented 4 years ago

-g - "global mode" - the go command would behave as if no go.mod file were present.

Per https://github.com/golang/go/issues/32027#issuecomment-533273609, we'll probably want to make the go command less willing to resolve dependencies when no go.mod file is present.

If we do that, presumably the -g flag will allow the go command to resolve the transitive dependencies of the modules containing the packages listed on the command line. Should -g also allow the go command to resolve missing dependencies found in the import statements of those packages?

bcmills commented 4 years ago

The original go.mod would still determine the module root directory,

but

These flags would be accepted when GO111MODULE is set to on and rejected when GO111MODULE is set to off.

If GO111MODULE is on but there is no go.mod file above the current working directory, what would we use as the module root?

jayconrod commented 4 years ago

Per #32027 (comment), we'll probably want to make the go command less willing to resolve dependencies when no go.mod file is present.

If we do that, presumably the -g flag will allow the go command to resolve the transitive dependencies of the modules containing the packages listed on the command line. Should -g also allow the go command to resolve missing dependencies found in the import statements of those packages?

I think -g should just make the go command do whatever it would do without a go.mod file. So if go run x.go resolves transitively imports and succeeds, go run -g x.go would do the same within a module. But if we change go run x.go to fail, then go run -g x.go would also fail.

jayconrod commented 4 years ago

If GO111MODULE is on but there is no go.mod file above the current working directory, what would we use as the module root?

I contradicted myself a bit here. Updated the text. -modfile and -sumfile require an actual go.mod to be present in order to set the module root directory.

heschi commented 4 years ago

Regarding the file flags: I think having to specify both -modfile and -sumfile is cumbersome. I can't think of any compelling reason to want to share go.sum with any other module, since go mod tidy will throw away the unnecessary lines anyway. So I would suggest that at a minimum, -sumfile be derived from -modfile if it's not set. A more extreme option would be to specify a single prefix, say -modprefix, and add .mod and .sum to it as needed. That may be too strange, though.

This is also a good precedent to set in case there's ever a third file, since nobody will know to override its location before it exists.

myitcv commented 4 years ago

@jayconrod regarding the "gopls (various issues)" motivation. I'm not entirely clear on what the issue here is and hence why -g/-modfile/other is a solution.

Is the problem that people are not used to cmd/go having side effects in module mode?

Because for me, I would expect that anything I do inside my editor, e.g. adding an import for a package whose module is not in my go.mod, could have side effects on the main module.

Could you give a bit more background?

mvdan commented 4 years ago

30515 - offer a consistent "global install" command

While not part of the original issue, the consensus for a while seemed to be that a "global" install should also obey replace directives. At least, this is what @ianthehat said in https://github.com/golang/go/issues/30515#issuecomment-469544933. Has the team's position generally changed to not obeying replace directives at all?

I'm also a bit confused by the multiple flags, like @heschik. How about a -modroot=path flag? It would roughly be equivalent to cd path && go <args>.

myitcv commented 4 years ago

Possibly worth reiterating that replace directives come in two forms:

~I think the consensus that @mvdan refers to above was on the latter being applied.~

See @mvdan's comment following this one.

mvdan commented 4 years ago

Sorry, I should have clarified. There were initially some comments about applying non-directory replace directives, but the consensus seemed that we either apply all of them or none of them - to not add a third build mode. @ianthehat's comment that I linked seemed to lean towards applying all replace directives.

rogpeppe commented 4 years ago

Personally, I'm in favour of applying directory replacements only when the source is in a user-controlled directory (as opposed to in global mode where the source is in the module cache). But I feel strongly that installs in global mode should respect other replacements, FWIW.

bcmills commented 4 years ago

@mvdan, @rogpeppe: see https://github.com/golang/go/issues/30515#issuecomment-469742759 and https://github.com/golang/go/issues/30515#issuecomment-470224733.

Furthermore, given that the proposed semantics of the -g flag are to do whatever we would do if outside of a module, the question of whether or how to apply replace directives seems orthogonal (and a bit off-topic). #31173 is probably a more appropriate venue for that discussion.

bcmills commented 4 years ago

It would be an error to use this flag when no actual go.mod file is present.

We should probably make that even stronger: the module directive in the replacement go.mod file should specify the same module path as the actual go.mod file. (Otherwise, if we're building packages within the module, we will end up resolving what should be module-local imports by looking for an external module.)

jayconrod commented 4 years ago

regarding the "gopls (various issues)" motivation. I'm not entirely clear on what the issue here is and hence why -g/-modfile/other is a solution.

Is the problem that people are not used to cmd/go having side effects in module mode?

Because for me, I would expect that anything I do inside my editor, e.g. adding an import for a package whose module is not in my go.mod, could have side effects on the main module.

@myitcv, @ianthehat and @stamblerre have told me there's no canonical issue for this, but they've had to close a lot of issues as "working as intended", pointing to #29452, and they explain this frequently on Slack.

They mentioned one egregious example where someone in a clean workspace switched branches, then tried to switch back but were unable to because go.mod had uncommitted changes. Their editor (and gopls) was open in the background. It had detected changes in open files, run go list indirectly, and modified go.mod as a result.

-modfile would have made these changes to a temporary go.mod file instead of the go.mod in the main repo. #31999 is about supporting go.mod files in gopls, and while there aren't any details there yet, part of the plan is to provide easy ways to make changes and upgrades. It would be difficult to do that without -modfile.

jayconrod commented 4 years ago

Regarding the file flags: I think having to specify both -modfile and -sumfile is cumbersome. I can't think of any compelling reason to want to share go.sum with any other module, since go mod tidy will throw away the unnecessary lines anyway. So I would suggest that at a minimum, -sumfile be derived from -modfile if it's not set. A more extreme option would be to specify a single prefix, say -modprefix, and add .mod and .sum to it as needed. That may be too strange, though.

This is also a good precedent to set in case there's ever a third file, since nobody will know to override its location before it exists.

I like the simplicity of just saying what both files should be, but I couldn't actually come up with a scenario where you'd want to set -modfile without -sumfile.

How about this: there's no -sumfile flag. If -modfile is set to M, then the sum file is strings.TrimPrefix(M, ".mod") + ".sum".

jayconrod commented 4 years ago

I'm also a bit confused by the multiple flags, like @heschik. How about a -modroot=path flag? It would roughly be equivalent to cd path && go .

I don't think that solves the same set of problems. -modfile would still use the location of actual go.mod file to set the module root directory, not its argument. -modfile just redirects reads and writes, providing a way to control changes.

ianthehat commented 4 years ago

While not part of the original issue, the consensus for a while seemed to be that a "global" install should also obey replace directives. At least, this is what @ianthehat said in #30515 (comment). Has the team's position generally changed to not obeying replace directives at all?

After thinking through all the weird edge cases and trying out a bunch of things, I came to the conclusion that applying replace directives is a confusing and dangerous ball of scary, and the only sane thing to do is not to apply them, in fact the only sane thing to do is to never ever check replace directives in to your repository in the first place. Part of the benefit of this proposal is it allows for a workflow that makes that a much easier goal, by allowing an alternate go.mod that has the replace directives in it but is not used by default.

I'm also a bit confused by the multiple flags, like @heschik. How about a -modroot=path flag? It would roughly be equivalent to cd path && go <args>.

That would not allow most of the useful patterns (having multiple .mod files in the same directory that you can choose between, or a cache directory with modified .mod files for a bunch of different modules etc)

It would also mean that you have to specify what happens in a bunch of interesting edge cases (thinks like relative paths, are they from the original module or the alternate root?)

I think that specifying the .sum file would/should be incredibly rare, leaving it as the original one would be fine for the majority of use cases.

jayconrod commented 4 years ago

@mvdan @rogpeppe

On replacements:

  1. We should apply all of them or none of them. A third mode would cause confusion. (https://github.com/golang/go/issues/30515#issuecomment-469742759).
  2. In order to apply file path replacements, we'd need to check out a whole repo. That's very different from what go get does now (especially when a proxy is in use), and there are many ways it could fail. We also won't get reproducible builds if replacement paths point outside the repository.

Personally, I think the downsides of (2) outweigh the benefits, and it's better to have something very simple like -g.

bcmills commented 4 years ago

How about this: there's no -sumfile flag. If -modfile is set to M, then the sum file is strings.TrimPrefix(M, ".mod") + ".sum".

Maybe split the difference? If the original is go.mod and -modfile is M.mod, first check whether M.sum exists: if so, use (and update) it, and otherwise use (and update) the original go.sum.

As far as I can tell, that handles both the tools.go.mod case and the /tmp/some-gopath-dir/go.{mod,sum} case.

jayconrod commented 4 years ago

Maybe split the difference? If the original is go.mod and -modfile is M.mod, first check whether M.sum exists: if so, use (and update) it, and otherwise use (and update) the original go.sum.

As far as I can tell, that handles both the tools.go.mod case and the /tmp/some-gopath-dir/go.{mod,sum} case.

@bcmils Could work, but it seems a little subtle. If a module only depends on std and has no go.sum file, a naïve tool might copy go.mod to /tmp without creating an empty go.sum file. It would then unexpectedly modify the temp go.mod and the original go.sum.

myitcv commented 4 years ago

@jayconrod re https://github.com/golang/go/issues/34506#issuecomment-535040055

-modfile would have made these changes to a temporary go.mod file instead of the go.mod in the main repo

But what about the situations where you do want gopls to have these side effects? i.e. adding an import for a package whose module is not in my go.mod.

bcmills commented 4 years ago

I think -g should just make the go command do whatever it would do without a go.mod file.

Without a go.mod file, go get example.com/some/really/old/tool (that is, some tool without its own go.mod file) should probably fail, rather than re-resolving the latest transitive imports of that tool and discarding the result.

On the other hand, I think it is probably reasonable to expect go get -g example.com/some/really/old/tool to succeed, even if it is consistently slow.

jayconrod commented 4 years ago

But what about the situations where you do want gopls to have these side effects? i.e. adding an import for a package whose module is not in my go.mod.

@myitcv It would be up to gopls and the editor to provide a sensible way to do that. Perhaps it could show a warning that go.mod is incomplete and provide a quick fix to add the missing requirement. Or it could update go.mod when a file is saved if it adds new imports.

There are situations where modifying go.mod is not wanted, and -modfile provides gopls with a knob that controls when those modifications happen.

jayconrod commented 4 years ago

Without a go.mod file, go get example.com/some/really/old/tool (that is, some tool without its own go.mod file) should probably fail, rather than re-resolving the latest transitive imports of that tool and discarding the result.

On the other hand, I think it is probably reasonable to expect go get -g example.com/some/really/old/tool to succeed, even if it is consistently slow.

@bcmills There's a lot of nuance here. Assuming no go.mod is present, should go get example.com/tool when example.com/tool does have a go.mod file? What if the go.mod file is missing some requirements?

Should -g have this variation in behavior for go get only or for other commands, too?

bcmills commented 4 years ago

Assuming no go.mod is present, should go get example.com/tool when example.com/tool does have a go.mod file? What if the go.mod file is missing some requirements?

I'm not sure. Probably:

bcmills commented 4 years ago

Should -g have this variation in behavior for go get only or for other commands, too?

Do we need to support the -g flag at all for other commands? I was assuming it would be a get-specific flag.

myitcv commented 4 years ago

It would be up to gopls and the editor to provide a sensible way to do that. Perhaps it could show a warning that go.mod is incomplete and provide a quick fix to add the missing requirement. Or it could update go.mod when a file is saved if it adds new imports.

This creates too much of a disconnect with the other go commands to my mind. By way of analogy, this would be equivalent to go test failing because go.mod is incomplete, and then requiring the user to run some specific command to fix it before again running go test.

~Hence @bcmills's comment makes sense to me:~

~Do we need to support the -g flag at all for other commands? I was assuming it would be a get-specific flag.~

Updated: @bcmills was talking about -g and @jayconrod was referring to -modfile

rodrigc commented 4 years ago

@jayconrod will your proposed change still be compatible with the advice given at:

https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module

(I'm guessing yes, but just want to make sure).

Some projects have migrated to that approach such as this:

https://github.com/atlassian/smith/blob/master/tools.go

But there are other projects which still do go get in a Makefile inside the module directory to fetch tools.

ianthehat commented 4 years ago

The proposal has no effect on that approach, but...

That approach is a horrible hack with all sorts of horrendous caveats and problems. I would argue strongly that it was okay as a temporary measure in go1.11 but it is already bad advice today and would become much worse advice when the features in this proposal are available.

bcmills commented 4 years ago

That approach is a horrible hack with all sorts of horrendous caveats and problems.

[citation needed]

33926 is the only issue I'm aware of describing a concrete problem with the // +build tools approach, and the concrete problem cited there (https://github.com/golang/go/issues/33926#issuecomment-527946737, an incompatibility between gopls and one of its dependencies) was due to a breaking change made in one of the tool's non-semantically-versioned dependencies.

jayconrod commented 4 years ago

Some notes from a few out-of-band conversations with @bcmills @ianthehat and @myitcv:

-modfile

-g

eliasnaur commented 4 years ago

@myitcv

But what about the situations where you do want gopls to have these side effects? i.e. adding an import for a package whose module is not in my go.mod.

@jayconrod

Some notes from a few out-of-band conversations with @bcmills @ianthehat and @myitcv:

-modfile

* In evaluating this feature request, it would be helpful to have a better understanding of how gopls would use it. Clearly, there are situations where it should and should not modify go.mod. Some extra logic will be required to manage and synchronize both the actual and temporary go.mod files.

  * @ianthehat @stamblerre Could you comment here with ideas on when this feature would be used and how that would work in gopls? What would the user experience look like, ideally? Is any of this a v1.0.0 blocker?

I don't think it's obvious gopls should ever modify my go.mod and go.sum. To me, gopls is an extension of my editor, and my editor should not change any files I didn't ask it to. To me, it's tedious to constantly have to keep an eye out for irrelevant go.mod/go.sum changes before checking in new code.

Of course, if I run go commands from inside my editor (either directly or indirectly through a "run", "build" or similar editor command), then my go.mod/go.sum files are subject to change. But in those cases I'm no longer editing, I'm using the editor as a convenient command line shell.

So in conclusion, I see gopls always using -modfile pointing to its own go.mod file as the right decision.

myitcv commented 4 years ago

@eliasnaur so what about when go test modifies your go.{mod,sum} - is that also an error from your perspective? Because in that instance you haven't specifically asked it to modify your go.{mod,sum}, you've simply asked to run some tests. go mod edit is the specific command for editing go.mod. If go test did not operate in this way, having to run an additional command (which would require some manual effort) to add the relevant require directives would be tiresome to my mind because that's almost always what I want to do.

Per @jayconrod's request to @ianthehat and @stamblerre above, I think we need to establish what the workflow here would be from a user's perspective before doing something that to my mind goes against what the go tool is already doing.

josharian commented 4 years ago

Another use case for this is go-fuzz. go-fuzz has to augment the source tree with its own dependencies during compilation, but there is ~zero value to the user in having the module containing those dependencies be present in their go.mod/go.sum. (cc @thepudds)

zikaeroh commented 4 years ago

To offer some input from a heavy gopls user: I'm very happy to let gopls modify my go.mod file as it pleases, since that means I can paste in import paths and have them actually resolve, even if I have to do cleanup duty later if go.mod contains something I didn't expect.

If it didn't have this ability, they'd break, and then more things end up breaking when I have to go run go in my terminal and gopls doesn't see that something has changed (leading to some bad state only a reload can fix, which is already far too common). I can see a future where gopls wouldn't have the ability to modify go.mod, but it'd really need to better handle watching for changes, and would be kinda disappointing to go from the workflow of "paste in an import and it just works" back to the old workflow of needing to make sure that everything is there before trying to use it.

eliasnaur commented 4 years ago

@eliasnaur so what about when go test modifies your go.{mod,sum} - is that also an error from your perspective?

No. I meant to cover go test by:

Of course, if I run go commands from inside my editor (either directly or indirectly through a "run", "build" or similar editor command), then my go.mod/go.sum files are subject to change. But in those cases I'm no longer editing, I'm using the editor as a convenient command line shell.

In other words, go commands modify my go.* files, as they should. I'm only talking about gopls and editing in general, which shouldn't.

To offer some input from a heavy gopls user: I'm very happy to let gopls modify my go.mod file as it pleases, since that means I can paste in import paths and have them actually resolve, even if I have to do cleanup duty later if go.mod contains something I didn't expect.

I agree, and that's why I haven't complained about gopls before this proposal. With -modfile, gopls can keep a private go.mod file and operate exactly as if it had modified your go.mod file.

myitcv commented 4 years ago

In other words, go commands modify my go.* files, as they should

My point was that for some people go test and other build commands (e.g. go list, go build etc) should not have these side effects: https://github.com/golang/go/issues/29452. i.e. an explicit go mod edit commands should be required before running go test etc.

I'm not arguing for that position, just using it to point out that many people have normalised the fact that cmd/go build commands modify go.{mod,sum} in just the same way that I've normalised gopls modifying go.{mod,sum}.

With -modfile, gopls can keep a private go.mod file and operate exactly as if it had modified your go.mod file.

What would the workflow look like when I do want gopls to modify my go.{mod,sum}?

Having a second go.{mod,sum} introduces a second source of truth; the "real" one is used by cmd/go, the "fake" is used by gopls. How do we ensure they remain in sync? How do we point out conflicts?

eliasnaur commented 4 years ago

In other words, go commands modify my go.* files, as they should

My point was that for some people go test and other build commands (e.g. go list, go build etc) should not have these side effects: #29452. i.e. an explicit go mod edit commands should be required before running go test etc.

I'm not arguing for that position, just using it to point out that many people have normalised the fact that cmd/go build commands modify go.{mod,sum} in just the same way that I've normalised gopls modifying go.{mod,sum}.

Sure, I've also normalized that behaviour, by monitoring my go.* files like a hawk :)

Note that #29452 seems to make a distinction between building commands such as go test, go install, go build and query commands such as go list, go mod. So even with a hypothetical fix to #29452, your go test would still do what you expect.

With -modfile, gopls can keep a private go.mod file and operate exactly as if it had modified your go.mod file.

What would the workflow look like when I do want gopls to modify my go.{mod,sum}?

May I ask when and why you want gopls to modify you go. files? And for each of the cases you do want its modifications, why aren't your inevitable go test, go run, go install or even go mod tidy good enough checkpoinst for actually changing your go. files?

The only legitimate case I can see is for adding imports that you're going to keep, but how would gopls know the keepers from your spelling mistakes? I can't count the number of times gopls has added irrelevant dependencies to my go.* files just because I code completed or misspelled a package.

Having a second go.{mod,sum} introduces a second source of truth; the "real" one is used by cmd/go, the "fake" is used by gopls. How do we ensure they remain in sync? How do we point out conflicts?

My go. files are the source of truth, while gopls' own files are for caching queries and pleasing its underlying go commands. If conflicts occur, its conflicting (or all?) changes to its internal go. files should be wiped out.

myitcv commented 4 years ago

Note that #29452 seems to make a distinction between building commands such as go test, go install, go build and query commands such as go list, go mod. So even with a hypothetical fix to #29452, your go test would still do what you expect.

It's worth noting that go list is a build command; it takes build flags. go mod edit is I think the only go mod subcommand that is literally concerned with mechanically editing go.mod. All the other subcommands will cause a full resolution of dependencies that might result in changes to go.{mod,sum}.

The reason I bring up the go test example is because in another world we could easily have decided that go test should fail if go.{mod,sum} didn't contain the relevant entries for go test to proceed. In such a scenario, a go mod edit or similar mechanical update would have been required as a separate step before running go test again. As it is, go test does as little as it needs to to go.{mod,sum} for the build configuration under test. This feels natural to me, because it avoids the painful error message "you need to run X before continuing".

May I ask when and why you want gopls to modify you go.* files?

Because I want the type checking and analysis that gopls carries out to proceed in much the same way that go test does without me needing to intervene. Similarly, when gopls gains the power to help with things like code generation or other commands, I don't want to be having to manually intervene to run go mod edit to fix my go.mod or click "accept" on a code suggestion to add something to my go.mod.

The only legitimate case I can see is for adding imports that you're going to keep, but how would gopls know the keepers from your spelling mistakes?

I notice you skilfully sidestepped my question on the workflow of when I do want gopls to update my go.{mod,sum} 😉 I'm guessing however you want to manually "copy" the "right" changes from gopls's copy to the real copy?

I can't count the number of times gopls has added irrelevant dependencies to my go.* files just because I code completed or misspelled a package.

This could happen in any scenario: even with go test for example. go mod tidy is your friend here, and I think Rebecca added support for a formatting of go.mod files (which amounts to a go mod tidy from memory). I don't think manually copying/accepting the "right" changes is necessary here, the tools can do the work for us.

If conflicts occur, its conflicting (or all?) changes to its internal go.* files should be wiped out.

I'm certainly not clear what this conflict resolution algorithm would look like: can you sketch it out ?

eliasnaur commented 4 years ago

Note that #29452 seems to make a distinction between building commands such as go test, go install, go build and query commands such as go list, go mod. So even with a hypothetical fix to #29452, your go test would still do what you expect.

It's worth noting that go list is a build command; it takes build flags. go mod edit is I think the only go mod subcommand that is literally concerned with mechanically editing go.mod. All the other subcommands will cause a full resolution of dependencies that might result in changes to go.{mod,sum}.

I suppose that's what #29452 is about: go list is a build command, but feels like a query command.

However, in the context of gopls its use of go list is an implementation detail.

May I ask when and why you want gopls to modify you go.* files?

Because I want the type checking and analysis that gopls carries out to proceed in much the same way that go test does without me needing to intervene. Similarly, when gopls gains the power to help with things like code generation or other commands, I don't want to be having to manually intervene to run go mod edit to fix my go.mod or click "accept" on a code suggestion to add something to my go.mod.

That's a great example. I want to consent to go.mod changes because adding a dependency is not something I do lightly. From Rus Cox' Our Software Dependency Problem:

"A package, for this discussion, is code you download from the internet. Adding a package as a dependency outsources the work of developing that code—designing, writing, testing, debugging, and maintaining—to someone else on the internet, someone you often don’t know. By using that code, you are exposing your own program to all the failures and flaws in the dependency. Your program’s execution now literally depends on code downloaded from this stranger on the internet. Presented this way, it sounds incredibly unsafe. Why would anyone do this?"

In other words, completing or auto-generating code for me is ok to do more or less automatically, but or adding or changing my dependencies is not.

I notice you skilfully sidestepped my question on the workflow of when I do want gopls to update my go.{mod,sum} wink I'm guessing however you want to manually "copy" the "right" changes from gopls's copy to the real copy?

Whenever you consent to the changes. Whether that is a go build, go test on the command line, or clicking "run" or "accept" in a IDE. And perhaps there is a setting for "I accept all changes" if you really want to accept whatever gopls gives you during edits.

I can't count the number of times gopls has added irrelevant dependencies to my go.* files just because I code completed or misspelled a package.

This could happen in any scenario: even with go test for example. go mod tidy is your friend here, and I think Rebecca added support for a formatting of go.mod files (which amounts to a go mod tidy from memory). I don't think manually copying/accepting the "right" changes is necessary here, the tools can do the work for us.

Yes, but I consider go test and friends consenting to changes. It's not perfect (#29452) and I would personally have GOFLAGS=-mod=readonly if govim supported it.

If conflicts occur, its conflicting (or all?) changes to its internal go.* files should be wiped out.

I'm certainly not clear what this conflict resolution algorithm would look like: can you sketch it out ?

Keep an internal copy of go. and use -modfile to point to them. Whenever the original go. files changes, replace the copy.

bcmills commented 4 years ago

I'm curious as to whether #34829 would satisfy the gopls use-case less invasively. How important is it to resolve dependencies in uncommitted code, vs. tolerating (and possibly suppressing) errors for unresolved dependencies?

jayconrod commented 4 years ago

Contemporaneous notes from a conversation with @ianthehat and @stamblerre:

myitcv commented 4 years ago

@eliasnaur

go list is a build command, but feels like a query command.

I can sympathise with that point of view: it's certainly not obvious.

However, in the context of gopls its use of go list is an implementation detail.

This is true but it's a critical implementation detail. Whilst go/packages is the abstraction, go list is the very means by which the concept of modules, packages etc are defined for cmd/go-based build systems.

(That's not to say however that we're bound only by what cmd/go offers today, indeed that's what we're discussing here.)

I would personally have GOFLAGS=-mod=readonly if govim supported it.

I think this is a great suggestion for an option in govim. Indeed I just did a bit of experimentation and it already works if GOFLAGS=-mod=readonly is passed to the environment of gopls by govim: you get no modifications to go.{mod,sum} as a result of using Vim/govim, and changes made via cmd/go are correctly picked up by gopls by virtue of govim simulating file watch changes (whilst we wait for full gopls file watching support). We could add this today with a bit of a hack on the govim side, pending support for a workspace config option in gopls at a later date. I've created https://github.com/govim/govim/issues/555 to track this for govim.

Whenever you consent to the changes. Whether that is a go build, go test on the command line, or clicking "run" or "accept" in a IDE.

I'm not clear how go build or go test is any more "safe" than things happening via gopls/go/packages/go list - they will end up having exactly the same side effects (if we consider them being run at the same point in time) and you're still blind to them unless you check your go.{mod,sum}.

Keep an internal copy of go. and use -modfile to point to them. Whenever the original go. files changes, replace the copy.

As you mention above, you'd either look to "accept" the changes by:

  1. explicitly running go build, go test and friends
  2. prompting the user

For option 1, this would mean that there is a possibility the subsequent go build/go test will result in a different changes to the go.{mod,sum} than those made in the -modfile copy. Because none of these commands (with the exception of go get) specifies a version of a dependency. This doesn't seem ideal.

For option 2, presumably you'd prompt for every change that gets made to the -modfile go.mod to "sync" it back to the original if the user accepts? If so, this feels like a very noisy workflow to me, especially when I don't get any such prompts when using go test etc.

Per @jayconrod's request to Rebecca and Ian (https://github.com/golang/go/issues/34506#issuecomment-536024494) and subsequent follow up in https://github.com/golang/go/issues/34506#issuecomment-541165801, I think it's worth looking at the scenarios when a go.{mod,sum} change can occur:

And also the use cases that motivated this discussion in the first place:

UC3 is covered above and and in https://github.com/govim/govim/issues/555.

UC1 requires a significantly broader solution to my understanding, because changing files like this "underneath" the editor has all sorts of problems.

UC2 is totally valid to my mind, and building on the suggestion discussed between Jay, Rebecca and Ian, it might make sense (assuming the user is not interested in UC3) to not make any changes to go.{mod,sum} until the first user-initiated change is made to a file in the main module. This however creates a weird UX, because all type check errors that might show would then disappear with the addition, say, of a space to a comment. Alternatively, if it's detected the go.{mod,sum} is not tidy, then the user could be prompted to fix this as the workspace is opened.

Just to note, I'm very much in support of fixing workflow issues like this. My reservations are around a lack of clarity on the UI/UX, use cases that we're looking to fix, whether we will end up actually fixing those use cases, and potential issues with conflicts if we were to use -modfile.

josharian commented 4 years ago

scenarios when a go.{mod,sum} change can occur

As I mentioned above, this happens with go-fuzz. We inject a dependency and execute a build, which then changes go.mod in a way that the user doesn't care about at all.

myitcv commented 4 years ago

@josharian - sorry, I totally missed that. My comments were more focussed on use cases from a gopls/editor perspective. Although I'll admit I don't really know whether there is overlap or not with go-fuzz in that respect.

eliasnaur commented 4 years ago

On Fri Oct 11, 2019 at 1:52 PM Paul Jolly wrote:

Keep an internal copy of go. and use -modfile to point to them. Whenever the original go. files changes, replace the copy.

For option 2, presumably you'd prompt for every change that gets made to the -modfile go.mod to "sync" it back to the original if the user accepts? If so, this feels like a very noisy workflow to me, especially when I don't get any such prompts when using go test etc.

Do you mean that accepting every change is noisy? I read @jayconrod's comment

-modfile would let us add the module requirement to a separate go.mod, then suggest a change with the difference. We could type check everything.

as there would be a summary of all the changes which I could accept in bulk if/when I pleased. If I waited to accept changes, or (more likely) never accepted them, gopls would hopefully keep working.

josharian commented 4 years ago

Here's another use case for this flag.

As part of #34867, I want to use a small Go program to do some fairly trivial json parsing. At the moment that I need that Go program to run, the local go.mod file is in a bad state. (I'm running the Go program as part of a script to make the go.mod file functional.) As a result, I can't use go run. If I could do go run -modfile=/sometempfile, then I could temporarily get past the broken go.mod and run the file.

(Yes, there are other ways to accomplish this, like setting GO111MODULE=off, or copying the Go program into a temp dir, manufacturing a trivial go.mod for it, building it, and running it. But this flag also provides a nice, easy simple workaround.)

gopherbot commented 4 years ago

Change https://golang.org/cl/202564 mentions this issue: cmd/go: add -modfile flag that sets go.mod file to read/write

jayconrod commented 4 years ago

A quick update:

I've implemented the -modfile flag in CL 202564. There will be no -sumfile flag. Instead, the name of the go.sum file will be derived from the name of the go.mod file by trimming the .mod suffix (if present) and add .sum, as @heschik suggested.

I'm planning to add a -g flag that would apply to go get. It will cause go get to behave as if it were outside a module. After changes made in #32027, it doesn't really make sense for -g to apply to other commands.

This interpretation of -g means there would be no main module, and replace and exclude directives will be ignored. We've discussed the tradeoffs of doing it this way here and in #30515. I'm starting to think that we should support both approaches. Perhaps it should be possible to pass a -g=main flag. This would restrict command line arguments to one main package (or perhaps packages from one "main" module) and would apply replace and exclude directives from that module. File replace directives would be rejected. WDYT?

myitcv commented 4 years ago

@jayconrod just so I'm clear, what decision on replace/exclude directives was taken in https://go-review.googlesource.com/c/go/+/203279?