gopls/internal/lsp/cache: add support for loading standalone main files
Add support in gopls for working on "standalone main files", which are
Go source files that should be treated as standalone packages.
Standalone files are identified by a specific build tag, which may be
configured via the new standaloneTags setting. For example, it is common
to use the directive "//go:build ignore" to colocate standalone files
with other package files.
Specifically,
- add a new loadScope interface for use in snapshot.load, to add a bit
of type safety
- add a new standaloneTags setting to allow configuring the set of build
constraints that define standalone main files
- add an isStandaloneFile function that detects standalone files based
on build constraints
- implement the loading of standalone files, by querying go/packages for
the standalone file path
- rewrite getOrLoadIDsForURI, which had inconsistent behavior with
respect to error handling and the experimentalUseInvalidMetadata
setting
- update the WorkspaceSymbols handler to properly format
command-line-arguments packages
- add regression tests for LSP behavior with standalone files, and for
dynamic configuration of standalone files
Fixes golang/go#49657
Change-Id: I7b79257a984a87b67e476c32dec3c122f9bbc636
Reviewed-on: https://go-review.googlesource.com/c/tools/+/441877
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go
index c33561f..e930eb4 100644
--- a/gopls/internal/lsp/cache/load.go
+++ b/gopls/internal/lsp/cache/load.go
@@ -7,6 +7,7 @@
import (
"bytes"
"context"
+ "errors"
"fmt"
"io/ioutil"
"os"
@@ -28,12 +29,15 @@
var loadID uint64 // atomic identifier for loads
+// errNoPackages indicates that a load query matched no packages.
+var errNoPackages = errors.New("no packages returned")
+
// load calls packages.Load for the given scopes, updating package metadata,
// import graph, and mapped files with the result.
//
// The resulting error may wrap the moduleErrorMap error type, representing
// errors associated with specific modules.
-func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interface{}) (err error) {
+func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadScope) (err error) {
id := atomic.AddUint64(&loadID, 1)
eventName := fmt.Sprintf("go/packages.Load #%d", id) // unique name for logging
@@ -45,7 +49,7 @@
moduleQueries := make(map[string]string)
for _, scope := range scopes {
switch scope := scope.(type) {
- case PackagePath:
+ case packageLoadScope:
if source.IsCommandLineArguments(string(scope)) {
panic("attempted to load command-line-arguments")
}
@@ -53,14 +57,24 @@
// partial workspace load. In those cases, the paths came back from
// go list and should already be GOPATH-vendorized when appropriate.
query = append(query, string(scope))
- case fileURI:
+
+ case fileLoadScope:
uri := span.URI(scope)
- // Don't try to load a file that doesn't exist.
fh := s.FindFile(uri)
if fh == nil || s.View().FileKind(fh) != source.Go {
+ // Don't try to load a file that doesn't exist, or isn't a go file.
continue
}
- query = append(query, fmt.Sprintf("file=%s", uri.Filename()))
+ contents, err := fh.Read()
+ if err != nil {
+ continue
+ }
+ if isStandaloneFile(contents, s.view.Options().StandaloneTags) {
+ query = append(query, uri.Filename())
+ } else {
+ query = append(query, fmt.Sprintf("file=%s", uri.Filename()))
+ }
+
case moduleLoadScope:
switch scope {
case "std", "cmd":
@@ -70,6 +84,7 @@
query = append(query, modQuery)
moduleQueries[modQuery] = string(scope)
}
+
case viewLoadScope:
// If we are outside of GOPATH, a module, or some other known
// build system, don't load subdirectories.
@@ -78,6 +93,7 @@
} else {
query = append(query, "./...")
}
+
default:
panic(fmt.Sprintf("unknown scope type %T", scope))
}
@@ -136,9 +152,9 @@
if len(pkgs) == 0 {
if err == nil {
- err = fmt.Errorf("no packages returned")
+ err = errNoPackages
}
- return fmt.Errorf("%v: %w", err, source.PackagesLoadError)
+ return fmt.Errorf("packages.Load error: %w", err)
}
moduleErrs := make(map[string][]packages.Error) // module path -> errors
diff --git a/gopls/internal/lsp/cache/metadata.go b/gopls/internal/lsp/cache/metadata.go
index 66c679b..2fa87eb 100644
--- a/gopls/internal/lsp/cache/metadata.go
+++ b/gopls/internal/lsp/cache/metadata.go
@@ -41,6 +41,11 @@
Config *packages.Config
}
+// PackageID implements the source.Metadata interface.
+func (m *Metadata) PackageID() string {
+ return string(m.ID)
+}
+
// Name implements the source.Metadata interface.
func (m *Metadata) PackageName() string {
return string(m.Name)
diff --git a/gopls/internal/lsp/cache/pkg.go b/gopls/internal/lsp/cache/pkg.go
index 76e29ee..44fe855 100644
--- a/gopls/internal/lsp/cache/pkg.go
+++ b/gopls/internal/lsp/cache/pkg.go
@@ -32,13 +32,24 @@
hasFixedFiles bool // if true, AST was sufficiently mangled that we should hide type errors
}
-// Declare explicit types for files and directories to distinguish between the two.
+// A loadScope defines a package loading scope for use with go/packages.
+type loadScope interface {
+ aScope()
+}
+
type (
- fileURI span.URI
- moduleLoadScope string
- viewLoadScope span.URI
+ fileLoadScope span.URI // load packages containing a file (including command-line-arguments)
+ packageLoadScope string // load a specific package
+ moduleLoadScope string // load packages in a specific module
+ viewLoadScope span.URI // load the workspace
)
+// Implement the loadScope interface.
+func (fileLoadScope) aScope() {}
+func (packageLoadScope) aScope() {}
+func (moduleLoadScope) aScope() {}
+func (viewLoadScope) aScope() {}
+
func (p *pkg) ID() string {
return string(p.m.ID)
}
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 1587a5d..f070fe3 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -712,52 +712,95 @@
return phs, nil
}
+// getOrLoadIDsForURI returns package IDs associated with the file uri. If no
+// such packages exist or if they are known to be stale, it reloads the file.
+//
+// If experimentalUseInvalidMetadata is set, this function may return package
+// IDs with invalid metadata.
func (s *snapshot) getOrLoadIDsForURI(ctx context.Context, uri span.URI) ([]PackageID, error) {
+ useInvalidMetadata := s.useInvalidMetadata()
+
s.mu.Lock()
+
+ // Start with the set of package associations derived from the last load.
ids := s.meta.ids[uri]
- reload := len(ids) == 0
+
+ hasValidID := false // whether we have any valid package metadata containing uri
+ shouldLoad := false // whether any packages containing uri are marked 'shouldLoad'
for _, id := range ids {
- // If the file is part of a package that needs reloading, reload it now to
- // improve our responsiveness.
- if len(s.shouldLoad[id]) > 0 {
- reload = true
- break
+ // TODO(rfindley): remove the defensiveness here. s.meta.metadata[id] must
+ // exist.
+ if m, ok := s.meta.metadata[id]; ok && m.Valid {
+ hasValidID = true
}
- // TODO(golang/go#36918): Previously, we would reload any package with
- // missing dependencies. This is expensive and results in too many
- // calls to packages.Load. Determine what we should do instead.
+ if len(s.shouldLoad[id]) > 0 {
+ shouldLoad = true
+ }
}
+
+ // Check if uri is known to be unloadable.
+ //
+ // TODO(rfindley): shouldn't we also mark uri as unloadable if the load below
+ // fails? Otherwise we endlessly load files with no packages.
+ _, unloadable := s.unloadableFiles[uri]
+
s.mu.Unlock()
- if reload {
- scope := fileURI(uri)
+ // Special case: if experimentalUseInvalidMetadata is set and we have any
+ // ids, just return them.
+ //
+ // This is arguably wrong: if the metadata is invalid we should try reloading
+ // it. However, this was the pre-existing behavior, and
+ // experimentalUseInvalidMetadata will be removed in a future release.
+ if !shouldLoad && useInvalidMetadata && len(ids) > 0 {
+ return ids, nil
+ }
+
+ // Reload if loading is likely to improve the package associations for uri:
+ // - uri is not contained in any valid packages
+ // - ...or one of the packages containing uri is marked 'shouldLoad'
+ // - ...but uri is not unloadable
+ if (shouldLoad || !hasValidID) && !unloadable {
+ scope := fileLoadScope(uri)
err := s.load(ctx, false, scope)
- // As in reloadWorkspace, we must clear scopes after loading.
+ // Guard against failed loads due to context cancellation.
//
- // TODO(rfindley): simply call reloadWorkspace here, first, to avoid this
- // duplication.
- if !errors.Is(err, context.Canceled) {
- s.clearShouldLoad(scope)
+ // Return the context error here as the current operation is no longer
+ // valid.
+ if ctxErr := ctx.Err(); ctxErr != nil {
+ return nil, ctxErr
}
- // TODO(rfindley): this doesn't look right. If we don't reload, we use
- // invalid metadata anyway, but if we DO reload and it fails, we don't?
- if !s.useInvalidMetadata() && err != nil {
- return nil, err
- }
+ // We must clear scopes after loading.
+ //
+ // TODO(rfindley): unlike reloadWorkspace, this is simply marking loaded
+ // packages as loaded. We could do this from snapshot.load and avoid
+ // raciness.
+ s.clearShouldLoad(scope)
- s.mu.Lock()
- ids = s.meta.ids[uri]
- s.mu.Unlock()
-
- // We've tried to reload and there are still no known IDs for the URI.
- // Return the load error, if there was one.
- if len(ids) == 0 {
- return nil, err
+ // Don't return an error here, as we may still return stale IDs.
+ // Furthermore, the result of getOrLoadIDsForURI should be consistent upon
+ // subsequent calls, even if the file is marked as unloadable.
+ if err != nil && !errors.Is(err, errNoPackages) {
+ event.Error(ctx, "getOrLoadIDsForURI", err)
}
}
+ s.mu.Lock()
+ ids = s.meta.ids[uri]
+ if !useInvalidMetadata {
+ var validIDs []PackageID
+ for _, id := range ids {
+ // TODO(rfindley): remove the defensiveness here as well.
+ if m, ok := s.meta.metadata[id]; ok && m.Valid {
+ validIDs = append(validIDs, id)
+ }
+ }
+ ids = validIDs
+ }
+ s.mu.Unlock()
+
return ids, nil
}
@@ -1206,17 +1249,18 @@
// clearShouldLoad clears package IDs that no longer need to be reloaded after
// scopes has been loaded.
-func (s *snapshot) clearShouldLoad(scopes ...interface{}) {
+func (s *snapshot) clearShouldLoad(scopes ...loadScope) {
s.mu.Lock()
defer s.mu.Unlock()
for _, scope := range scopes {
switch scope := scope.(type) {
- case PackagePath:
+ case packageLoadScope:
+ scopePath := PackagePath(scope)
var toDelete []PackageID
for id, pkgPaths := range s.shouldLoad {
for _, pkgPath := range pkgPaths {
- if pkgPath == scope {
+ if pkgPath == scopePath {
toDelete = append(toDelete, id)
}
}
@@ -1224,7 +1268,7 @@
for _, id := range toDelete {
delete(s.shouldLoad, id)
}
- case fileURI:
+ case fileLoadScope:
uri := span.URI(scope)
ids := s.meta.ids[uri]
for _, id := range ids {
@@ -1481,7 +1525,7 @@
// reloadWorkspace reloads the metadata for all invalidated workspace packages.
func (s *snapshot) reloadWorkspace(ctx context.Context) error {
- var scopes []interface{}
+ var scopes []loadScope
var seen map[PackagePath]bool
s.mu.Lock()
for _, pkgPaths := range s.shouldLoad {
@@ -1493,7 +1537,7 @@
continue
}
seen[pkgPath] = true
- scopes = append(scopes, pkgPath)
+ scopes = append(scopes, packageLoadScope(pkgPath))
}
}
s.mu.Unlock()
@@ -1505,7 +1549,7 @@
// If the view's build configuration is invalid, we cannot reload by
// package path. Just reload the directory instead.
if !s.ValidBuildConfiguration() {
- scopes = []interface{}{viewLoadScope("LOAD_INVALID_VIEW")}
+ scopes = []loadScope{viewLoadScope("LOAD_INVALID_VIEW")}
}
err := s.load(ctx, false, scopes...)
@@ -1527,7 +1571,7 @@
files := s.orphanedFiles()
// Files without a valid package declaration can't be loaded. Don't try.
- var scopes []interface{}
+ var scopes []loadScope
for _, file := range files {
pgf, err := s.ParseGo(ctx, file, source.ParseHeader)
if err != nil {
@@ -1536,7 +1580,8 @@
if !pgf.File.Package.IsValid() {
continue
}
- scopes = append(scopes, fileURI(file.URI()))
+
+ scopes = append(scopes, fileLoadScope(file.URI()))
}
if len(scopes) == 0 {
@@ -1560,7 +1605,7 @@
event.Error(ctx, "reloadOrphanedFiles: failed to load", err, tag.Query.Of(scopes))
s.mu.Lock()
for _, scope := range scopes {
- uri := span.URI(scope.(fileURI))
+ uri := span.URI(scope.(fileLoadScope))
if s.noValidMetadataForURILocked(uri) {
s.unloadableFiles[uri] = struct{}{}
}
diff --git a/gopls/internal/lsp/cache/standalone_go115.go b/gopls/internal/lsp/cache/standalone_go115.go
new file mode 100644
index 0000000..79569ae
--- /dev/null
+++ b/gopls/internal/lsp/cache/standalone_go115.go
@@ -0,0 +1,14 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build !go1.16
+// +build !go1.16
+
+package cache
+
+// isStandaloneFile returns false, as the 'standaloneTags' setting is
+// unsupported on Go 1.15 and earlier.
+func isStandaloneFile(src []byte, standaloneTags []string) bool {
+ return false
+}
diff --git a/gopls/internal/lsp/cache/standalone_go116.go b/gopls/internal/lsp/cache/standalone_go116.go
new file mode 100644
index 0000000..39e8864
--- /dev/null
+++ b/gopls/internal/lsp/cache/standalone_go116.go
@@ -0,0 +1,52 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build go1.16
+// +build go1.16
+
+package cache
+
+import (
+ "fmt"
+ "go/build/constraint"
+ "go/parser"
+ "go/token"
+)
+
+// isStandaloneFile reports whether a file with the given contents should be
+// considered a 'standalone main file', meaning a package that consists of only
+// a single file.
+func isStandaloneFile(src []byte, standaloneTags []string) bool {
+ f, err := parser.ParseFile(token.NewFileSet(), "", src, parser.PackageClauseOnly|parser.ParseComments)
+ if err != nil {
+ return false
+ }
+
+ if f.Name == nil || f.Name.Name != "main" {
+ return false
+ }
+
+ for _, cg := range f.Comments {
+ // Even with PackageClauseOnly the parser consumes the semicolon following
+ // the package clause, so we must guard against comments that come after
+ // the package name.
+ if cg.Pos() > f.Name.Pos() {
+ continue
+ }
+ for _, comment := range cg.List {
+ fmt.Println(comment.Text)
+ if c, err := constraint.Parse(comment.Text); err == nil {
+ if tag, ok := c.(*constraint.TagExpr); ok {
+ for _, t := range standaloneTags {
+ if t == tag.Tag {
+ return true
+ }
+ }
+ }
+ }
+ }
+ }
+
+ return false
+}
diff --git a/gopls/internal/lsp/cache/standalone_go116_test.go b/gopls/internal/lsp/cache/standalone_go116_test.go
new file mode 100644
index 0000000..5eb7ff0
--- /dev/null
+++ b/gopls/internal/lsp/cache/standalone_go116_test.go
@@ -0,0 +1,90 @@
+// Copyright 2022 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build go1.16
+// +build go1.16
+
+package cache
+
+import (
+ "testing"
+)
+
+func TestIsStandaloneFile(t *testing.T) {
+ tests := []struct {
+ desc string
+ contents string
+ standaloneTags []string
+ want bool
+ }{
+ {
+ "new syntax",
+ "//go:build ignore\n\npackage main\n",
+ []string{"ignore"},
+ true,
+ },
+ {
+ "legacy syntax",
+ "// +build ignore\n\npackage main\n",
+ []string{"ignore"},
+ true,
+ },
+ {
+ "invalid tag",
+ "// +build ignore\n\npackage main\n",
+ []string{"script"},
+ false,
+ },
+ {
+ "non-main package",
+ "//go:build ignore\n\npackage p\n",
+ []string{"ignore"},
+ false,
+ },
+ {
+ "alternate tag",
+ "// +build script\n\npackage main\n",
+ []string{"script"},
+ true,
+ },
+ {
+ "both syntax",
+ "//go:build ignore\n// +build ignore\n\npackage main\n",
+ []string{"ignore"},
+ true,
+ },
+ {
+ "after comments",
+ "// A non-directive comment\n//go:build ignore\n\npackage main\n",
+ []string{"ignore"},
+ true,
+ },
+ {
+ "after package decl",
+ "package main //go:build ignore\n",
+ []string{"ignore"},
+ false,
+ },
+ {
+ "on line after package decl",
+ "package main\n\n//go:build ignore\n",
+ []string{"ignore"},
+ false,
+ },
+ {
+ "combined with other expressions",
+ "\n\n//go:build ignore || darwin\n\npackage main\n",
+ []string{"ignore"},
+ false,
+ },
+ }
+
+ for _, test := range tests {
+ t.Run(test.desc, func(t *testing.T) {
+ if got := isStandaloneFile([]byte(test.contents), test.standaloneTags); got != test.want {
+ t.Errorf("isStandaloneFile(%q, %v) = %t, want %t", test.contents, test.standaloneTags, got, test.want)
+ }
+ })
+ }
+}
diff --git a/gopls/internal/lsp/cache/view.go b/gopls/internal/lsp/cache/view.go
index bdf2281..dec2cb0 100644
--- a/gopls/internal/lsp/cache/view.go
+++ b/gopls/internal/lsp/cache/view.go
@@ -291,6 +291,9 @@
if !reflect.DeepEqual(a.DirectoryFilters, b.DirectoryFilters) {
return false
}
+ if !reflect.DeepEqual(a.StandaloneTags, b.StandaloneTags) {
+ return false
+ }
if a.MemoryMode != b.MemoryMode {
return false
}
@@ -665,7 +668,7 @@
// Collect module paths to load by parsing go.mod files. If a module fails to
// parse, capture the parsing failure as a critical diagnostic.
- var scopes []interface{} // scopes to load
+ var scopes []loadScope // scopes to load
var modDiagnostics []*source.Diagnostic // diagnostics for broken go.mod files
addError := func(uri span.URI, err error) {
modDiagnostics = append(modDiagnostics, &source.Diagnostic{
@@ -709,7 +712,7 @@
// since it provides fake definitions (and documentation)
// for types like int that are used everywhere.
if len(scopes) > 0 {
- scopes = append(scopes, PackagePath("builtin"))
+ scopes = append(scopes, packageLoadScope("builtin"))
}
err := s.load(ctx, true, scopes...)
