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.

Sign up for GitHub

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: embed errors don't always appear #44342

Closed
heschi opened this issue Feb 17, 2021 · 6 comments
Closed

x/tools/gopls: embed errors don't always appear #44342

heschi opened this issue Feb 17, 2021 · 6 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.7.2

Comments

@heschi
Copy link
Contributor

heschi commented Feb 17, 2021

This program shows no error on the embed pattern:

package embedtest

import (
	_ "embed"
)

//go:embed NONEXISTANT
var foo string

When I add an import, the error appears, but only if it's not in the stdlib:

package embedtest

import (
	_ "embed"

	_ "golang.org/x/tools/imports"
)

//go:embed NONEXISTANT
var foo string

At first I thought this was because we should invalidate metadata when a //go:embed comment changes, but even when I restart gopls the IWL doesn't pick up the error. So there's something weird going on here that I didn't dig into. (We should probably still invalidate metadata on embed comment changes, or implement embed pattern checking ourselves.)

Reported by @myitcv in Slack.
@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 Feb 17, 2021
@gopherbot gopherbot added this to the Unreleased milestone Feb 17, 2021
@stamblerre stamblerre modified the milestones: Unreleased, gopls/v1.0.0 Feb 17, 2021
@heschi
Copy link
Contributor Author

heschi commented Feb 23, 2021

go/src/go/build/read.go

Lines 174 to 273 in c73232d

// findEmbed advances the input reader to the next //go:embed comment.
// It reports whether it found a comment.
// (Otherwise it found an error or EOF.)
func (r *importReader) findEmbed(first bool) bool {
// The import block scan stopped after a non-space character,
// so the reader is not at the start of a line on the first call.
// After that, each //go:embed extraction leaves the reader
// at the end of a line.
startLine := !first
var c byte
for r.err == nil && !r.eof {
c = r.readByteNoBuf()
Reswitch:
switch c {
default:
startLine = false
case '\n':
startLine = true
case ' ', '\t':
// leave startLine alone
case '"':
startLine = false
for r.err == nil {
if r.eof {
r.syntaxError()
}
c = r.readByteNoBuf()
if c == '\\' {
r.readByteNoBuf()
if r.err != nil {
r.syntaxError()
return false
}
continue
}
if c == '"' {
c = r.readByteNoBuf()
goto Reswitch
}
}
goto Reswitch
case '`':
startLine = false
for r.err == nil {
if r.eof {
r.syntaxError()
}
c = r.readByteNoBuf()
if c == '`' {
c = r.readByteNoBuf()
goto Reswitch
}
}
case '/':
c = r.readByteNoBuf()
switch c {
default:
startLine = false
goto Reswitch
case '*':
var c1 byte
for (c != '*' || c1 != '/') && r.err == nil {
if r.eof {
r.syntaxError()
}
c, c1 = c1, r.readByteNoBuf()
}
startLine = false
case '/':
if startLine {
// Try to read this as a //go:embed comment.
for i := range goEmbed {
c = r.readByteNoBuf()
if c != goEmbed[i] {
goto SkipSlashSlash
}
}
c = r.readByteNoBuf()
if c == ' ' || c == '\t' {
// Found one!
return true
}
}
SkipSlashSlash:
for c != '\n' && r.err == nil && !r.eof {
c = r.readByteNoBuf()
}
startLine = true
}
}
}
return false
}
is the code that reads embed comments if we want to track them.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/295415 mentions this issue: gopls/internal/regtest: add test for bad embed rules

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/295412 mentions this issue: internal/lsp/cache: parse filenames from go list errors correctly

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/296549 mentions this issue: internal/lsp/cache: invalidate metadata on magic comment changes

gopherbot pushed a commit to golang/tools that referenced this issue Mar 2, 2021
@heschi
internal/lsp/cache: parse filenames from go list errors correctly
89a9cb6 
go/packages.Error has filenames relative to the go command's working
directory. We need to interpret them as such.

This would perhaps be better done in go/packages but with no release
process in place I'm leery of making changes to it.

Updates golang/go#44342.

Change-Id: I95bcdff0368efe09ec7059394e59a39bf195310b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/295412
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Mar 2, 2021
@heschi
gopls/internal/regtest: add test for bad embed rules
f9c628b 
Updates golang/go#44342.

Change-Id: I8518874fb20ae0a95f6fb2b62033e70a7d057067
Reviewed-on: https://go-review.googlesource.com/c/tools/+/295415
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
gopherbot pushed a commit to golang/tools that referenced this issue Mar 2, 2021
@heschi
internal/lsp/cache: invalidate metadata on magic comment changes
6422c5c 
When a //go:embed or //go:build (//+build) line changes, we need to
invalidate metadata. Do so. It might be preferable to only invalidate on
save, but we don't currently have an approach for doing that. So for now
we'll load on each keystroke in a magic comment.

Fixes golang/go#38732, golang/go#44342.

Change-Id: Id05fb84f44215ea6242a7cf8b2bca4e85f74680e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/296549
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@ShoshinNikita
Copy link

I think this issue should be closed according to the commit golang/tools@6422c5c:

Fixes golang/go#38732, golang/go#44342

@stamblerre
Copy link
Contributor

Oh good catch, thanks @ShoshinNikita! Closing.

@stamblerre stamblerre modified the milestones: gopls/v1.0.0, gopls/v0.7.2 Aug 6, 2021
@golang golang locked and limited conversation to collaborators Aug 6, 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. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.7.2
Development

No branches or pull requests
4 participants
@stamblerre @gopherbot @heschi @ShoshinNikita