Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/gopls: edit the go.mod with go commands whenever possible #38209

Closed
stamblerre opened this issue Apr 1, 2020 · 27 comments
Closed

x/tools/gopls: edit the go.mod with go commands whenever possible #38209

stamblerre opened this issue Apr 1, 2020 · 27 comments
Assignees
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 1, 2020

We maintain a temporary go.sum file that is updated along with the temporary go.mod, but we never write it out. We should update the project's go.sum file whenever the go.mod file is saved.

/cc @ridersofrohan to confirm that this approach sounds reasonable
/cc @ardan-bkennedy

@stamblerre stamblerre added this to the gopls/v0.4.0 milestone Apr 1, 2020
@stamblerre stamblerre modified the milestones: gopls/v0.4.0, gopls/v0.5.0 Apr 2, 2020
@hyangah
Copy link
Contributor

@hyangah hyangah commented Apr 2, 2020

What does LSP flow look like? Will gopls send a save command to the client?

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Apr 2, 2020

I think we can just write the file to disk directly. Or, I guess there's also https://microsoft.github.io/language-server-protocol/specifications/specification-3-15/#workspace_applyEdit, but I'm not sure if the client will leave the file saved in that case. We want to make sure that the edit to the go.sum happens silently, without the user interacting with it.

@hyangah
Copy link
Contributor

@hyangah hyangah commented Apr 2, 2020

My concern around having gopls directly write the file to disk is that usually the file saving is supposed to be done from the editor side (client) - so, WillSave* and DidSave notifications.

It's possible that the client may cancel saving for some reason after WillSave, so I guess it should be done upon DidSave notification (and expect the following didChangeWatchedFiles notification if the client is watching the file system).

Still there could be rare cases where file write access is not available (vscode live if we ever find a way to support it in the future?).

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Apr 2, 2020

I agree it's not ideal, but I think it would be a bit strange for the user if they have a "tidied" go.mod file, but a messy go.sum. We'll have to see how it works in practice.

@stamblerre stamblerre modified the milestones: gopls/v0.4.0, gopls/v0.5.0 Apr 3, 2020
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Apr 16, 2020

Thinking about this a bit more...the main issue is that we don't necessarily have the "correct" go.sum for the given go.mod - we have the go.sum for the tidied go.mod.

@jayconrod: Is there some way we can update the go.sum in the same fine-grained way that we update the go.mod? We use the functions in https://pkg.go.dev/golang.org/x/mod/modfile - is there anything analogous for sum files?

@ardan-bkennedy
Copy link

@ardan-bkennedy ardan-bkennedy commented Apr 16, 2020

What if we provided a way from inside the editor to run go mod tidy / vendor? That is what a developer really needs at the end of the day. I need gopls to know about it.

I'm finding that I am doing that instead of using the in-editor support you added for updating go.mod.

What has been beautiful is, I don't need to run go mod tidy right away because gopls just starts working with the import. That is the big win here.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Apr 16, 2020

You could just add missing lines from the temporary go.sum. There's no programmatic interface in x/mod for doing that, but the format is very simple.

Having a way to run go mod tidy would be useful, but it has to be an explicit action since it can delete requirements that might still be needed.

@ardan-bkennedy
Copy link

@ardan-bkennedy ardan-bkennedy commented Apr 16, 2020

Yes explicit from within the editor environment.

Is adding a command to the context menu for the go.mod file an option?

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Apr 16, 2020

What if we provided a way from inside the editor to run go mod tidy / vendor? That is what a developer really needs at the end of the day. I need gopls to know about it.

This has been available for some time.

Screen Shot 2020-04-16 at 11 00 02 AM

You can even configure it to run on save:

"[go.mod]": {
	"editor.codeActionsOnSave": {
		"source.organizeImports": true,
	},
}

@jayconrod: Thanks for the information. I'll play around with it and see how far I get.

@ardan-bkennedy
Copy link

@ardan-bkennedy ardan-bkennedy commented Apr 16, 2020

WHAT!!!!!

I don't see that?

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Apr 16, 2020

It will only show up if your go.mod isn't tidied, I believe.

@ardan-bkennedy
Copy link

@ardan-bkennedy ardan-bkennedy commented Apr 16, 2020

I am removing items from go.mod and I get this message

Failed to save 'go.mod': The content of the file is newer. Please compare your version with the file contents or overwrite the content of the file with your changes.

That option is not showing up at all. However, eventually a save will bring those modules back in.

@ardan-bkennedy
Copy link

@ardan-bkennedy ardan-bkennedy commented Apr 16, 2020

However, I need to run a vendor because I use vendoring. This is all hidden :(

It would be so much better as an option on the context menu if this is an option

Tidy
Vendor

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Apr 16, 2020

We could have it show up like the code lenses for upgrading dependencies, but then it wouldn't tidy on save, so we'd have the same command available in two places. Maybe we can still do that though.

Vendoring we haven't really added any special support for. Is there any other behavior you'd like to have with vendoring? If there are more things maybe we can start a separate discussion for that on a new issue.

@ardan-bkennedy
Copy link

@ardan-bkennedy ardan-bkennedy commented Apr 16, 2020

I removed those settings because I think they are making things more confusing.

If I had wish...

The ability to right click on the go.mod file and have the option for go mod tidy and go mod vendor. That seems like the most obvious place to put it.

If that is not possible, the ability to see a squiggly line on the module keyword in go.mod so a context box can be presented with this options.

I think the goal is to present to the developer these options to tidy and vendor when they need to. Then they don't need to leave the editor to do it. Then gopls can know this operating is being executed explicitly by the developer.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Jul 27, 2020

@jayconrod: Going back to the discussion of keeping the go.mod and go.sum in sync, is there any way to tell which dependency caused which lines to be added to the go.sum? We offer the user each go mod tidy diagnostic and suggested fix separately, so we need to keep a mapping of the go.sum lines that correspond to each missing/unused dependency.

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Aug 3, 2020

Is there any way to tell which dependency caused which lines to be added to the go.sum?

@stamblerre If you're talking about direct dependencies, then in general, this is not possible. go.sum contains sums for all transitive dependencies. A transitive dependency may be reachable from multiple direct dependencies, so there's not a unique answer. go mod graph might help though when there is.

This may be moot though after #36460 is implemented. All modules needed to build and test packages in the main module will be listed in go.mod, not just transitive dependencies, and fewer transitive go.mod files will be loaded, so the contents of go.mod and go.sum should be similar.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Aug 4, 2020

I see, thanks for the clarification. I guess another way of implementing this would be some way to say "given this go.mod, what is the go.sum. I know I can generate it by running go mod tidy, but I don't want to tidy the whole module. Is there a way to do that?

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Aug 12, 2020

@heschik has suggested that we can do this with go list -m all in the main module.

@heschik
Copy link
Contributor

@heschik heschik commented Aug 13, 2020

go list all, actually; with -m it only fetches go.mod files and doesn't fill in checksums for the zip files :-/

@jayconrod
Copy link
Contributor

@jayconrod jayconrod commented Aug 13, 2020

go list all may fetch zip files for modules that don't provide any packages needed to build the main module (e.g., test dependencies of other modules' tests), so it's probably more than what you need.

go list -test ./... from the root of the main module would be better. That's very close to the set of packages loaded by go mod tidy. I think the only difference is that go list applies build constraints and go mod tidy does not.

You could also copy go.mod and go.sum, then use go mod tidy -modfile=copy.mod, then copy back the copy.sum file. That would do exactly what go mod tidy would do without updating the original go.mod or go.sum.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Aug 13, 2020

go list -test ./... from the root of the main module would be better. That's very close to the set of packages loaded by go mod tidy. I think the only difference is that go list applies build constraints and go mod tidy does not.

We do this when we first need to initialize the workspace, but I don't think we can pay the cost of running this every time we want to update the go.sum. If go list all is equally expensive, we can consider using ./..., but we'd just need to very careful about updating the go.sum (right now, I was thinking we could do it when save the go.mod).

Edit: I guess we do this with -compiled, which is probably what makes it so expensive. I'll try this out.

You could also copy go.mod and go.sum, then use go mod tidy -modfile=copy.mod, then copy back the copy.sum file. That would do exactly what go mod tidy would do without updating the original go.mod or go.sum.

The issue is we want to avoid go mod tidy here - we want the content of the go.sum file for a given go.mod, not for a tidied go.mod. The use case here is that, a user has accepted one of our suggested fixes, but not all, and wants that single fix reflected in their go.sum.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Aug 13, 2020

After some more discussion offline, we've concluded that it's too expensive to run these go lists on changes to the go.mod. As a compromise, we will:

  • Add a go mod tidy code lens will which make it easier to keep the two files in sync (#40422)
  • Suggest edits to the go.sum when we detect that the go.mod is tidy
@stamblerre stamblerre changed the title x/tools/gopls: update go.sum with go.mod x/tools/gopls: update go.sum when go.mod is tidy Sep 9, 2020
@stamblerre stamblerre changed the title x/tools/gopls: update go.sum when go.mod is tidy x/tools/gopls: edit the go.mod with go commands whenever possible Sep 17, 2020
@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Sep 17, 2020

An alternative to textual edits of the go.mod is running go get to add dependencies, etc. We should do this whenever possible, as it will also update the go.sum.

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 28, 2020

Change https://golang.org/cl/265981 mentions this issue: internal/lsp: use the go command to fix go.mod files

@heschik
Copy link
Contributor

@heschik heschik commented Oct 30, 2020

Missed one, the source.OrganizeImports action for mod files still does text edits.

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 4, 2020

Change https://golang.org/cl/267682 mentions this issue: internal/lsp: remove organize imports action for go.mod

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.