Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

optimize godef-gomod cache, better support one workspace for multi go… #2216

Closed
wants to merge 5 commits into from

Conversation

sunliver
Copy link

@sunliver sunliver commented Dec 27, 2018

see this issue

ChangeLog

  1. Now workspaceModCache stands for a workspace's go.mod's path.
  2. Rm pkgModCache, use folderModCache instead.
  3. Add function getModPath to detect file's mod path. If getModPath returns none empty string, then isModSupported returns true.

Known Problems

  1. If you are working with GO111MODULE projects and legacy GOPATH projects at the same time, you'd better set GO111MODULE to auto. Because we use go env GOMOD to find go.mod, if may fail when you're under GOPATH project.
  2. Godef for gomod is slower than expect now.

@msftclas
Copy link

msftclas commented Dec 27, 2018

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sunliver Looks like this is your first PR to this project, Thanks & Welcome!

The working directory used to run godef was intentionally kept to match the current workspace to support the below case:

  • Use Go to definition on a symbol from an external package
  • When the file opens (from the module cache) for the above symbol, run Go to definition again.

For the second run, godef expects the working directory to be that of the original file and not of the file from the module cache.

Can you update this PR to ensure the above still holds true?

@@ -80,7 +82,7 @@ export function adjustWordPosition(document: vscode.TextDocument, position: vsco

const godefImportDefinitionRegex = /^import \(.* ".*"\)$/;
function definitionLocation_godef(input: GoDefinitionInput, token: vscode.CancellationToken): Promise<GoDefinitionInformation> {
let godefTool = input.isMod ? 'godef-gomod' : 'godef';
let godefTool = 'godef';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not change this in this PR. I have a separate issue tracking the need to use godef instead of godef-mod, now that the changes from the latter have been merged to the former


return new Promise<GoDefinitionInformation>((resolve, reject) => {
// Spawn `godef` process
p = cp.execFile(godefPath, ['-t', '-i', '-f', input.document.fileName, '-o', offset.toString()], { env, cwd }, (err, stdout, stderr) => {
p = cp.execFile(godefPath, ['-t', '-f', input.document.fileName, '-o', offset.toString()], { env, cwd }, (err, stdout, stderr) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for removing the -i flag?
We had that in place to support the Go to definition feature on unsaved changes in the file

workspaceModCache.forEach((v, k) => {
if (folderPath.startsWith(k)) {
folderModCache.set(folderPath, k);
return Promise.resolve(k);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return applies just to the callback passed to the forEach call. If you want to exit the getModPath function when you folderPath.startsWith(k) is true, then you need to use a simple for loop

@sunliver
Copy link
Author

sunliver commented Jan 9, 2019

@ramya-rao-a Thanks for you code review! Sorry for some incorrect changes.

If I recall right,

  • the first run is, for example, my project uses module A, and I run go to definition in opened files of A
  • the second run is, I just open a file from local mod cache, and I run go to definition?

@ramya-rao-a
Copy link
Contributor

The first run is when you run go to definition on a symbol from module A used in one of the files in your project.
This will open the file from module A from the module cache (I think this would be from GOPATH/pkg/mod)
The second run is when you run go to definition on any symbol in this file that got opened from module A

@sunliver
Copy link
Author

Actually it runs as you think.
I did not change the behavior of old GOPATH package, just add support for mod package.

If you try to go to definition under a package,

  • first use go list -m -f {{.GoMod}} to figure out whether it is a mod package or not.
  • if not, run godef under workspace dir
  • otherwise run godef under mod path

Why change go env GOMOD to go list -m -f {{.GoMod}}?
golang/go#26500 (comment)

@ramya-rao-a
Copy link
Contributor

Why change go env GOMOD to go list -m -f {{.GoMod}}?

Are you asking why we are using go env GOMOD instead of go list -m -f {{.GoMod}}?
We are doing this as go list has been known to be much slower.

@sunliver
Copy link
Author

But go env GOMOD may fail sometimes.
As I've tested in this issue, golang/go#29433, though it is not a mod package , go env GOMOD still get the go.mod file. But it is not actually likely to occur.

@ramya-rao-a
Copy link
Contributor

@sunliver I have pushed a commit that gets the latest from the master branch. There was a bug where errors from godef resulted in an infinite loop which is now fixed with 8c6f035

Now going back to the case I was describing in #2216 (comment), lets see the below example

package main

import "rsc.io/quote"

func main() {
	quote.Hello()
}

Here if you run Go to definition on the Hello() function, the file $GOPATH/pkg/mod/rsc.io/quote@v1.5.2/quote.go opens. This file uses another package called sampler. Run Go to definition on sampler.Hello().

Using the code in this PR, this Go to definition operation fails.
You can see the error in the output channel -> Log (Extension Host)

Command failed: C:\GoTools\bin\godef.exe -t -i -f c:\GoCode\pkg\mod\rsc.io\quote@v1.5.2\quote.go -o 335 godef: no object

This is because we are using the folder $GOPATH/pkg/mod as current working directory.

@ramya-rao-a
Copy link
Contributor

@sunliver What do you think of #2262?

It is basically the same approach you have taken, plus some simplification, but with not changing the logic for files under the module cache.

@ramya-rao-a
Copy link
Contributor

Closing this PR in favor of #2262 which does something similar and has other fixes

@sunliver
Copy link
Author

@sunliver I have pushed a commit that gets the latest from the master branch. There was a bug where errors from godef resulted in an infinite loop which is now fixed with 8c6f035

Now going back to the case I was describing in #2216 (comment), lets see the below example

package main

import "rsc.io/quote"

func main() {
	quote.Hello()
}

Here if you run Go to definition on the Hello() function, the file $GOPATH/pkg/mod/rsc.io/quote@v1.5.2/quote.go opens. This file uses another package called sampler. Run Go to definition on sampler.Hello().

Using the code in this PR, this Go to definition operation fails.
You can see the error in the output channel -> Log (Extension Host)

Command failed: C:\GoTools\bin\godef.exe -t -i -f c:\GoCode\pkg\mod\rsc.io\quote@v1.5.2\quote.go -o 335 godef: no object

This is because we are using the folder $GOPATH/pkg/mod as current working directory.

@ramya-rao-a
I've found this case a few days ago. Because rsc.io/quote use sampler , and quote does not add go.sum into the repo. And currently x/package requires go.sum go work, actually go list requires go.sum to work. So the go tool try to generate the go.sum file to work. But mod path is read only, and it fails.

@sunliver
Copy link
Author

sunliver commented Jan 22, 2019

@ramya-rao-a

cwd: (modFolderPath && modFolderPath !== getModuleCache())
? modFolderPath : (getWorkspaceFolderPath(document.uri) || path.dirname(document.fileName))

It seems that you may fail back to use workspace folder in some cases.
It works in most situations. Here I provide a corner case: go to definition on a module file, which is not directly used in our project.
I've found that, for example, the folders organize as follows:

├── test    =>   A
│   ├── go.mod    => B
│   └── main.go
  • if you run godef -f another-third-party-from-pkg-mod -o xxx under B, and the test module doesn't require the module, it fails
    godef: There must be at least one package that contains the file
  • if you run under the third_party pkg mod path, it successes.

Now comes back on #2216 (comment), the second run.
Before go1.11, if we run go to definition on a file, which is not used in our project, it will fail.
But now comes go1.11 and x/package, if we run go to definition under the same situation, we can make it if we set the correct cwd.

However, godef's behavior is not clearly enough for me now, later I will dig into it.

Thus #2216 (comment), go tool may not work as we expected, I still think #2262 it is too complicated, although you mentioned that,

Not using the right package import path for packages in module cache, results in no documentation > to show on hover for their symbols too. So we update the getCurrentPackage function to return the right import path for such packages.

Maybe ask go team for some help.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants