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 non-workspace packages #48738

Closed
goblinfactory opened this issue Oct 1, 2021 · 4 comments
Closed
Assignees
Labels
FrozenDueToAge gopls/analysis Issues related to running analysis in gopls gopls/performance Issues related to gopls performance (CPU, memory, etc). gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@goblinfactory
Copy link

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 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

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 Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Oct 1, 2021
@gopherbot gopherbot added this to the Unreleased milestone Oct 1, 2021
@findleyr
Copy link
Contributor

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 modified the milestones: Unreleased, gopls/unplanned Oct 4, 2021
@hyangah hyangah modified the milestones: gopls/unplanned, gopls/on-deck Oct 8, 2021
@findleyr findleyr added the gopls/performance Issues related to gopls performance (CPU, memory, etc). label Oct 8, 2021
@findleyr findleyr added the gopls/analysis Issues related to running analysis in gopls label Apr 11, 2022
@findleyr findleyr changed the title x/tools/gopls: compute analysis facts for stdlib x/tools/gopls: compute analysis facts for non-workspace packages May 10, 2022
@adonovan
Copy link
Member

This was fixed at head by https://go.dev/cl/443099. I have verified that the original test case is now working.

@hyangah hyangah modified the milestones: gopls/later, gopls/v0.12.0 Jan 4, 2023
@golang golang locked and limited conversation to collaborators Jan 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls/analysis Issues related to running analysis in gopls gopls/performance Issues related to gopls performance (CPU, memory, etc). gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants