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: compute analysis facts for stdlib #48738

Open
goblinfactory opened this issue Oct 1, 2021 · 3 comments
Open

x/tools/gopls: compute analysis facts for stdlib #48738

goblinfactory opened this issue Oct 1, 2021 · 3 comments

Comments

@goblinfactory
Copy link

@goblinfactory goblinfactory commented Oct 1, 2021

What version of Go, VS Code & VS Code Go extension are you using?

Version Information
  • Run go version to get version of Go from the VS Code integrated terminal.
    • go1.17 darwin/amd64
  • Run gopls -v version to get version of Gopls from the VS Code integrated terminal.
    • golang.org/x/tools/gopls v0.7.2
  • Run code -v or code-insiders -v to get version of VS Code or VS Code Insiders.
    • 1.60.2
  • Check your installed extensions to get the version of the VS Code Go extension
    • v0.28.1
  • Run Ctrl+Shift+P (Cmd+Shift+P on Mac OS) > Go: Locate Configured Go Tools command.
    • gopkgs: /Users/{username}/go/bin/gopkgs: go1.17
    • go-outline: /Users/{username}/go/bin/go-outline: go1.17
    • gotests: /Users/{username}/go/bin/gotests: go1.17
    • gomodifytags: /Users/{username}/go/bin/gomodifytags: go1.17
    • impl: /Users/{username}/go/bin/impl: go1.17
    • goplay: /Users/{username}/go/bin/goplay: go1.17
    • dlv: /Users/{username}/go/bin/dlv: go1.17
    • dlv-dap: /Users/{username}/go/bin/dlv-dap: go1.17
    • golint: /Users/{username}/go/bin/golint: go1.17
    • gopls: /Users/{username}/go/bin/gopls: go1.17

Share the Go related settings you have added/edited

Run Preferences: Open Settings (JSON) command to open your settings.json file.
Share all the settings with the go. or ["go"] or gopls prefixes.

{
    "go.toolsManagement.autoUpdate": true,
    "go.lintTool": "golint",
    "go.vetOnSave": "workspace"
}

Describe the bug

When "Go:vet On Save is enabled in settings; and a go file is edited and saved (containing an error that go vet ./... does pick up), then no error squiggles appear in the editor to show that there is a vet error.

Required behavior

When "Go:vet On Save is enabled in settings; and a go file is edited and saved (containing an error that go vet ./... does pick up), then error squiggles MUST appear in the editor to show that there is a vet error.

Steps to reproduce the behavior:

  1. given a go file testvet.go with the following code
package testvet

import (
	"fmt"
	"sync"
)

// TestThatVetRunsOnSave minimal code to show vet not running on save in VS code
func TestThatVetRunsOnSave() {

	ch := make(chan string)
	var wg sync.WaitGroup
	wg.Add(1)
	go printit(wg, ch)

	ch <- "one"
	ch <- "two"
	close(ch)
	wg.Wait()
	fmt.Println("done.")

}

func printit(wg sync.WaitGroup, ch chan string) {
	defer wg.Done()
	for t := range ch {
		fmt.Println(t)
	}
}
  1. When I edit any text in this file that changes something, for example a space or comment
  2. Then the editor should highlight the same lines of code containing vet errors, that the command line go vet ./... picks up as having an error.
pkg/testvet/testvet.go:14:13: call of printit copies lock value: sync.WaitGroup contains sync.noCopy
pkg/testvet/testvet.go:24:17: printit passes lock by value: sync.WaitGroup contains sync.noCopy
  1. Manually running the command , Go:vet workspace does work and causes the editor to report the correct errors and squigglies. (see screenshot below)

Screenshots or recordings

To be clear, the screenshot below is the behaviour that we WANT to happen and be triggered on save, this is currently not happening and only happens when you manually run the command Go;vet workspace

Screenshot 2021-10-01 at 19 33 36

In addition to this functionality not working, the user interface helptext in VSCode settings does not make sense. The setting helptext has this text, which refers to functionality that I believe is not longer valid.

Screenshot 2021-10-01 at 19 45 06

I have tried all variations of this setting value; both package and workspace. Neither works.

@heschi
Copy link
Contributor

@heschi heschi commented Oct 1, 2021

I suspect this is because gopls doesn't calculate Facts for non-workspace packages. (https://cs.opensource.google/go/x/tools/+/master:internal/lsp/cache/analysis.go;l=122;drc=c8db76165785717f38f6e82eb659e5aae5e991da) Perhaps it could produce them just for the stdlib somehow?

@goblinfactory
Copy link
Author

@goblinfactory goblinfactory commented Oct 1, 2021

Just for the standard lib would be a great work around.
The reason this particular issue is a thorny one for me, as a new engineer coming to the language for the first time, is that there's no compiler or language support for ensuring that some types cannot be passed by value, and when learning Go for the first time, beginners spend a lot of time writing small applications with goroutines and channels, with sync.WaitGroup. These can quickly become quite complex to debug for a new developer, and having the IDE give you a warning you've passed a waitGroup by value, can often save a whole evening of debugging.

Having the facts produced for the stdlib would solve this. (I'm assuming that would include sync.Waitgroup?)

@stamblerre stamblerre transferred this issue from golang/vscode-go Oct 1, 2021
@stamblerre stamblerre changed the title Go: Vet On Save (not working) x/tools/gopls: compute analysis facts for stdlib Oct 1, 2021
@gopherbot gopherbot added this to the Unreleased milestone Oct 1, 2021
@findleyr
Copy link
Contributor

@findleyr findleyr commented Oct 4, 2021

If we do this naively, computing analysis facts for the standard library will be very costly.

We want to do this, but my guess is that any solution that works for the standard library will work more generally.

@findleyr findleyr removed this from the Unreleased milestone Oct 4, 2021
@findleyr findleyr added this to the gopls/unplanned milestone Oct 4, 2021
@hyangah hyangah removed this from the gopls/unplanned milestone Oct 8, 2021
@hyangah hyangah added this to the gopls/on-deck milestone Oct 8, 2021
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
5 participants