tools: merge gomodifytags functionality into gopls and use gopls

[hyangah]

From #1652

Function: modify tags on structs (go.add.tags, go.remove.tags commands)
Proposal:

• Option 1: do nothing and keep using gomodifytags.
• Option 2: implement from gopls: can we refactor gomodifytags and reuse it from gopls? (cc @fatih)

[fatih]

@hyangah Hi 👋🏼

When I created gomodifytags years ago, I made sure to write it so other applications could import it in the future. Here is a PR for the initial stab on putting it into a package that can be imported (not now, but later): fatih/gomodifytags#89

It's currently inside an internal folder as I want to make sure it works well for gopls before making it part of the public package. For example, the error message contains references to flags, even though in gopls there will be no flags as I know.

Let me know what you think and how you think we should proceed. For example, do we want to make gomodifytags part of gopls by copying the code into the codebase of gopls? Or do we want to import it and keep it under fatih/gomodifytags? I worked years on this project, it's battle tested and has hundreds of regression tests, so I think it's in a perfect position to be either imported or copied into gopls. Again, just let me think what is best for gopls.

[hyangah]

Hi! Thanks a lot @fatih!

cc @findleyr for API & gopls integration advice:

Skimming through your PR, the new API will be like

type Config struct {
	fset *token.FileSet

	File     string
	Output   string
	Quiet    bool
	Write    bool
	Modified io.Reader

	Offset     int
	StructName string
	FieldName  string
	Line       string
	Start, End int
	All        bool

	Remove        []string
	RemoveOptions []string

	Add                  []string
	AddOptions           []string
	Override             bool
	SkipUnexportedFields bool

	Transform   string
	Sort        bool
	ValueFormat string
	Clear       bool
	ClearOption bool
}

// Run runs gomodifytags with the config.
func (c *Config) Run() error {...}

Currently Config specifies the input (io.Reader) and the output is stdout, in addition to the flags that determine the logic of gomodifytags.

Gopls does the custom command integration within the x/tools/internal/lsp/command framework. Gopls manages the snapshot of the parsed file including the overlay treatment, and the framework allows the integrated tool to access data available in source.Snapshot. The output should be communicated back to the client (editor) using LSP messages.

So, I think it would allow us more efficient integration if the API simply lets the caller handle read/parse/output part, takes ast.Node(and the gomodifytags-specific flags) and returns a new ast.Node or the edit. Then, gopls can process the edit or diff and translate it to LSP TextDocumentEdit message. What do you think, (@fatih @findleyr )?

For example, gopls is implementing VS Code's add_import feature like this https://cs.opensource.google/go/x/tools/+/refs/tags/v0.1.8:internal/lsp/command.go;l=684-701.

I think it's better to keep it under fatih/gomodifytags. Exposing the final API as a publicly available package will make our job easier, bug keeping it internal is also ok for short term - we can automate copying if necessary.

[findleyr]

Thank you @fatih!

I also only skimmed the PR, but have a couple thoughts:

• We need to be able to pass our view of the overlay. It looks like this may be possible via the Config.Modified field? That may work, though being able to pass an *ast.File or *ast.Node directly could be better, as Hana suggests.
• I agree with @hyangah that we'd rather call a library than maintain our own copy. However, we can't do this until we refactor to allow adding custom commands from the golang.org/x/tools/gopls module (we can't currently add third-party module imports to x/tools). We'll look into this.

[findleyr]

@fatih we're looking at this now, as gomodifytags is one of our final remaining third-party tool dependency in VS Code (and we'd like to expose its functionality to all gopls-backed editors).

Are you still interested in landing that PR? If not, we can fork the gomodifytags codebase inside gopls.

[fatih]

Hi @findleyr,

Unfortunately I don't have the bandwidth to work on this. Feel free to fork or take the relevant functions from the project.

[findleyr]

@fatih we totally understand. Thanks for the quick response.

[findleyr]

@madelinekalil the current gomodifytags integration is documented here: https://github.com/golang/vscode-go/blob/master/docs/features.md#add-or-remove-struct-tags

And configuration is here: https://github.com/golang/vscode-go/blob/master/docs/settings.md#goaddtags

Since we are not exposing all of the gomodifytags command line in VS Code settings, I think this will be perhaps easier than first thought. To migrate gomodifytags to gopls, we need to:

• Selectively fork the gomodifytags logic into gopls. We put forks in the third_party directory with their license (see examples here: https://cs.opensource.google/search?q=f:third.party&ss=go). We should also include the gomodifytags license in the gopls licenses subcommand output.
• Add gopls commands to add and remove tags. Arguments to adding tags should correspond to the 'go.addTags' configuration above.
• Migrate the VS Code commands to invoke gopls.
• Also surface these gopls commands in refactoring code actions, when the cursor is in a struct, field, or tag. (Let's discuss the details of how this would work).

(EDIT: whoops, fixed link to gomodifytags feature documentation)

[findleyr]

@fatih we're encountering some friction with different ways of vendoring in or depending upon a fork of gomodifytags (e.g. doing so can cause more work for linux package managers that bundle gopls). Would you be amenable to us contributing a refactoring that allows gomodifytags to be called as a library? This would be similar to your CL above, but we'd handle the rebasing and would do the first pass at review ourselves, just needing your final sign-off.

Alternatively, we can rewrite most of the functionality without reference to the original source, which would avoid some of the hassle, but I'd prefer not to operate in such gray areas.

CC @adonovan @stapelberg with whom I'd discussed various options.

[fatih]

Would you be amenable to us contributing a refactoring that allows gomodifytags to be called as a library? This would be similar to your CL above, but we'd handle the rebasing and would do the first pass at review ourselves, just needing your final sign-off.

Yeap that sounds perfect. I'm more than happy to make it easy for you folks to include it. If the library doesn't work out, you can always rewrite and I would be OK with it. I believe the license I chose is the same we use for Go, so they are compatible. If you open a PR, I can easily merge it after you review it, just let me know once that is done.

[madelinekalil]

What do you think about us adding another entrypoint, so in addition to:
func (c *Config) Run() error
we would also have:
func (c *Config) Apply(f *ast.File) []protocol.TextEdit, error
We could avoid duplicating the work in cfg.parse() since gopls has already parsed the file, and generate a list of TextEdits based on the output from cfg.format()
cc @findleyr

[findleyr]

@madelinekalil the refactored gomodifytags package won't have access to our edit types. Perhaps we could mutate the given *ast.File in place?, and manage the formatting ourselves?

[madelinekalil]

@findleyr Another idea is to use this output type specified by gomodifytags:

type output struct {
	Start  int      `json:"start"`
	End    int      `json:"end"`
	Lines  []string `json:"lines"`
	Errors []string `json:"errors,omitempty"`
}

and then gopls can transform this into []protocol.TextEdit and make a call to protocol.Client.ApplyEdit

We could also mutate the file in place and do the formatting separately as you suggest. I think this option would involve the least number of changes to gomodifytags, but I'm not sure how important that is

[findleyr]

@madelinekalil I think having an API that mutates the AST could be most useful, as it allows the operation to be easily composable with other mutations.

How about

// Apply applies the struct tag modifications of the receiver to all
// structs contained within the given node, mutating its input.
func (c *Config) Apply(n ast.Node, start, end token.Pos) error {

}

This is very similar to the existing rewrite, API, except that it doesn't return the node. But returning the mutated node makes it less apparent that the input has been mutated. I think it is therefore best to leave of the node return.

[adonovan]

Config.Apply seems like the right existing seam to expose, but I will note that we still don't really have a good way to compose two operations that mutate syntax trees. If they are purely syntactic operations (as Config.Apply is), there's no problem, but if the second operation needs type information, there is no way to obtain type information for the mutated tree resulting from the first operation short of running the type checker again, which is expensive and may require careful coordination with the rest of the application.

Also, analyzers using the go/analysis framework and gopls code actions merely borrow the syntax tree that belongs to the framework, so they must not mutate it; instead they must clone the tree and return a mutated copy. However, type information is unavailable for the mutated copy, so the analysis must do its computation on the original.

In summary, most analyses must be lent the original, typed syntax tree, must not mutate it, and must express transformations by returning a mutated copy (or a textual diff).