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

Jump to bottom

x/tools/gopls: stale embed diagnostics when preceded by const block #47436

Closed
myitcv opened this issue Jul 28, 2021 · 6 comments
Closed

x/tools/gopls: stale embed diagnostics when preceded by const block #47436

myitcv opened this issue Jul 28, 2021 · 6 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.7.2

Comments

@myitcv
Copy link
Member

myitcv commented Jul 28, 2021

What version of Go are you using (go version)?

$ go version
go version devel go1.17-849b791129 Sun Jul 25 17:16:20 2021 +0000 linux/amd64
$ go list -m golang.org/x/tools
golang.org/x/tools v0.1.5-0.20210708231608-69948257bde7
$ go list -m golang.org/x/tools/gopls
golang.org/x/tools/gopls v0.0.0-20210708231608-69948257bde7

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/myitcv/.cache/go-build"
GOENV="/home/myitcv/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/myitcv/gostuff/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/myitcv/gostuff"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/myitcv/gos"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/myitcv/gos/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="devel go1.17-849b791129 Sun Jul 25 17:16:20 2021 +0000"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/myitcv/gostuff/src/github.com/myitcv/govim/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1114119742=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Firstly, a demonstration of a working setup:

-- dir/file.txt --
-- dir/other.go --
-- main.go --
package p

import "embed"

//go:embed main.go *.txt
var myFS embed.FS

Sequence is:

  • Open main.go
  • See diagnostic for bad go:embed directive
  • Fix it, see diagnostic disappear
  • Remove fix, see diagnostic return

The corresponding log file: working.log

Things go wrong however if there is a const declaration list before the go:embed directive (at least my hacking seems to demonstrate this as the problem):

-- dir/file.txt --
-- dir/other.go --
-- main.go --
package p

import "embed"

const (
	Hello = "world"
)

//go:embed main.go *.txt
var myFS embed.FS

Sequence is:

  • Open main.go
  • See diagnostic for bad go:embed directive
  • Fix it; diagnostic does not disappear but it should (bad_part1.log)
  • Close govim + gopls, because it is "stuck" at this point, and re-open.
  • Notice that now there is no diagnostic
  • Remove the fix, diagnostic does not return as it should (bad_part2.log)

What did you expect to see?

Consistent behaviour with respect to go:embed error diagnostics.

What did you see instead?

As above.

cc @stamblerre

FYI @leitzler
@myitcv myitcv added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. gopls Issues related to the Go language server, gopls. labels Jul 28, 2021
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jul 28, 2021
@gopherbot gopherbot added this to the Unreleased milestone Jul 28, 2021
@hyangah hyangah modified the milestones: Unreleased, gopls/v0.7.2 Jul 28, 2021
@hyangah
Copy link
Contributor

hyangah commented Jul 28, 2021

Interesting. I could reproduce this in vscode too.
I observed this stale diagnostics issue if go:embed is preceded by any code; the code doesn't have to be const block. I could repro the issue with func F() {} before the go:embed line. If the same code is placed after the go:embed directive, it doesn't affect and diagnostics work as expected.

@hyangah hyangah added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 30, 2021
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 30, 2021
ShoshinNikita added a commit to ShoshinNikita/tools that referenced this issue Aug 3, 2021
@ShoshinNikita
internal/lsp/cache: parse files with ParseFull mode to check if metad…
64d606c 
…ata reload is required

ParseHeader mode is used to parse only the package and import declarations.
However, change of go:embed directive should also invalidate metadata.
So, we must use ParseFull mode to get all file comments to compare
old and new go:embed directives.

Fixes golang/go#47436
@ShoshinNikita ShoshinNikita mentioned this issue Aug 3, 2021
Closed
@gopherbot
Copy link

Change https://golang.org/cl/339469 mentions this issue: internal/lsp/cache: parse files with ParseFull mode to check if metadata reload is required

@ShoshinNikita
Copy link

I could find the reason why the diagnostic works if go:embed is not preceded by any code. parser.ParseFile returns *ast.File with one extra comment if the mode is set to parser.ImportsOnly | parser.ParseComments:

main.go:

package main

import (
	"fmt"
	"go/parser"
	"go/token"
)

func main() {
	const (
		mode = parser.ImportsOnly | parser.ParseComments
		src  = `
// Package main ...
package main

import (
	// fmt import ...
	_ "fmt"
	_ "github.com/pkg/errors" // errors import ...
)

// F ...
func F() {}

// B ...
func B() {}

// Hello ...
const Hello = "word"
`
	)

	f, err := parser.ParseFile(token.NewFileSet(), "x.go", src, mode)
	if err != nil {
		panic(err)
	}
	for _, c := range f.Comments {
		fmt.Printf("%q\n", c.Text())
	}
}

Output:

"Package main ...\n"
"fmt import ...\n"
"errors import ...\n"
"F ...\n"

"F ...\n" should not be listed here because the comment for parser.ImportsOnly says stop parsing after import declarations. So, I think there's a bug in parser.ParseFile function. What do you think?

@myitcv
Copy link
Member Author

myitcv commented Aug 8, 2021

Thanks for the fix, @ShoshinNikita!

@ShoshinNikita
Copy link

cc @stamblerre - #47436 (comment)

@stamblerre
Copy link
Contributor

@ShoshinNikita: That definitely seems like a bug! If you can, please open a new issue in this repo with the prefix "go/parser: " and include a playground link to your repro case.

@ShoshinNikita ShoshinNikita mentioned this issue Aug 18, 2021
Open
@golang golang locked and limited conversation to collaborators Aug 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.7.2
5 participants
@myitcv @hyangah @stamblerre @gopherbot @ShoshinNikita