[tools] gopls: improve diagnostics for orphaned files
523 views
Skip to first unread message
Robert Findley (Gerrit)
May 12, 2023, 6:29:57 PM5/12/23
to goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, kokoro, Gopher Robot, Alan Donovan, golang-co...@googlegroups.com
Robert Findley submitted this change.
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: gopls/internal/lsp/regtest/marker.go
Insertions: 2, Deletions: 0.
@@ -108,6 +108,8 @@
// in these directories before running the test.
// -skip_goos=a,b,c instructs the test runner to skip the test for the
// listed GOOS values.
+// TODO(rfindley): using build constraint expressions for -skip_goos would
+// be clearer.
// TODO(rfindley): support flag values containing whitespace.
// - "settings.json": this file is parsed as JSON, and used as the
// session configuration (see gopls/doc/settings.md)
```
```
The name of the file: gopls/internal/lsp/cache/snapshot.go
Insertions: 5, Deletions: 2.
@@ -1771,6 +1771,8 @@
}
if msg == "" && ignoredFiles[fh.URI()] {
+ // TODO(rfindley): use the constraint package to check if the file
+ // _actually_ satisfies the current build context.
hasConstraint := false
walkConstraints(pgf.File, func(constraint.Expr) bool {
hasConstraint = true
@@ -1781,8 +1783,10 @@
fix = `This file may be excluded due to its build tags; try adding "-tags=<build tag>" to your gopls "buildFlags" configuration
See the documentation for more information on working with build tags:
https://github.com/golang/tools/blob/master/gopls/doc/settings.md#buildflags-string.`
- } else {
+ } else if strings.Contains(filepath.Base(fh.URI().Filename()), "_") {
fix = `This file may be excluded due to its GOOS/GOARCH, or other build constraints.`
+ } else {
+ fix = `This file is ignored by your gopls build.` // we don't know why
}
msg = fmt.Sprintf("No packages found for open file %s.\n%s", fh.URI().Filename(), fix)
}
@@ -1797,7 +1801,6 @@
Message: msg,
}
}
-
}
return diagnostics
```
Approvals:
Gopher Robot: TryBots succeeded
Robert Findley: Run TryBots
kokoro: gopls CI succeeded
Alan Donovan: Looks good to me, approved
gopls: improve diagnostics for orphaned files
Fix some of the very misleading errors gopls produces when it can't find
packages for a file. Try to diagnose _why_ a file is orphaned by the
workspace, rather than just guess that it may be due to build
constraints. Only put diagnostics on files that we can prove are
excluded in some way.
To achieve this, we need to be able to differentiate
command-line-arguments packages that are standalone packages, so add a
corresponding field on source.Metadata.
Refactor/simplify some functions that operate on open files. In the
future, we really should track overlays separately in the snapshot, but
that is out-of-scope for now.
Also make a minor fix for TestImplementationsOfError: I use $HOME/src as
my development directory, so GOROOT/src is $HOME/src/go/src.
For golang/go#53880
Change-Id: I8e9fa7d4f2c03ce3daaab7c6d119b4276ec6da79
Reviewed-on: https://go-review.googlesource.com/c/tools/+/494675
Run-TryBot: Robert Findley <rfin...@google.com>
Reviewed-by: Alan Donovan <adon...@google.com>
gopls-CI: kokoro <noreply...@google.com>
TryBot-Result: Gopher Robot <go...@golang.org>
---
M gopls/internal/lsp/cache/check.go
R gopls/internal/lsp/cache/constraints.go
R gopls/internal/lsp/cache/constraints_test.go
M gopls/internal/lsp/cache/graph.go
M gopls/internal/lsp/cache/load.go
M gopls/internal/lsp/cache/session.go
M gopls/internal/lsp/cache/snapshot.go
D gopls/internal/lsp/cache/standalone_go115.go
M gopls/internal/lsp/cache/view.go
M gopls/internal/lsp/diagnostics.go
M gopls/internal/lsp/regtest/marker.go
M gopls/internal/lsp/source/view.go
A gopls/internal/regtest/marker/testdata/diagnostics/excludedfile.txt
A gopls/internal/regtest/marker/testdata/quickfix/addgowork.txt
A gopls/internal/regtest/marker/testdata/quickfix/usemodule.txt
M gopls/internal/regtest/misc/references_test.go
M gopls/internal/regtest/workspace/metadata_test.go
17 files changed, 437 insertions(+), 194 deletions(-)
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index ce84487..83ea177 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -1440,7 +1440,7 @@
depPH := b.handles[id]
if depPH == nil {
// e.g. missing metadata for dependencies in buildPackageHandle
- return nil, missingPkgError(path, inputs.moduleMode)
+ return nil, missingPkgError(inputs.id, path, inputs.moduleMode)
}
if !source.IsValidImport(inputs.pkgPath, depPH.m.PkgPath) {
return nil, fmt.Errorf("invalid use of internal package %q", path)
@@ -1601,13 +1601,17 @@
// missingPkgError returns an error message for a missing package that varies
// based on the user's workspace mode.
-func missingPkgError(pkgPath string, moduleMode bool) error {
+func missingPkgError(from PackageID, pkgPath string, moduleMode bool) error {
// TODO(rfindley): improve this error. Previous versions of this error had
// access to the full snapshot, and could provide more information (such as
// the initialization error).
if moduleMode {
- // Previously, we would present the initialization error here.
- return fmt.Errorf("no required module provides package %q", pkgPath)
+ if source.IsCommandLineArguments(from) {
+ return fmt.Errorf("current file is not included in a workspace module")
+ } else {
+ // Previously, we would present the initialization error here.
+ return fmt.Errorf("no required module provides package %q", pkgPath)
+ }
} else {
// Previously, we would list the directories in GOROOT and GOPATH here.
return fmt.Errorf("cannot find package %q in GOROOT or GOPATH", pkgPath)
diff --git a/gopls/internal/lsp/cache/standalone_go116.go b/gopls/internal/lsp/cache/constraints.go
similarity index 63%
rename from gopls/internal/lsp/cache/standalone_go116.go
rename to gopls/internal/lsp/cache/constraints.go
index 2f72d5f..9503abc 100644
--- a/gopls/internal/lsp/cache/standalone_go116.go
+++ b/gopls/internal/lsp/cache/constraints.go
@@ -2,12 +2,10 @@
// 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 (
+ "go/ast"
"go/build/constraint"
"go/parser"
"go/token"
@@ -26,25 +24,38 @@
return false
}
- for _, cg := range f.Comments {
+ found := false
+ walkConstraints(f, func(c constraint.Expr) bool {
+ if tag, ok := c.(*constraint.TagExpr); ok {
+ for _, t := range standaloneTags {
+ if t == tag.Tag {
+ found = true
+ return false
+ }
+ }
+ }
+ return true
+ })
+
+ return found
+}
+
+// walkConstraints calls f for each constraint expression in the file, until
+// all constraints are exhausted or f returns false.
+func walkConstraints(file *ast.File, f func(constraint.Expr) bool) {
+ for _, cg := range file.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() {
+ if cg.Pos() > file.Name.Pos() {
continue
}
for _, comment := range cg.List {
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
- }
- }
+ if !f(c) {
+ return
}
}
}
}
-
- return false
}
diff --git a/gopls/internal/lsp/cache/standalone_go116_test.go b/gopls/internal/lsp/cache/constraints_test.go
similarity index 100%
rename from gopls/internal/lsp/cache/standalone_go116_test.go
rename to gopls/internal/lsp/cache/constraints_test.go
diff --git a/gopls/internal/lsp/cache/graph.go b/gopls/internal/lsp/cache/graph.go
index 3d1d0dd..684bdab 100644
--- a/gopls/internal/lsp/cache/graph.go
+++ b/gopls/internal/lsp/cache/graph.go
@@ -24,6 +24,8 @@
// ids maps file URIs to package IDs, sorted by (!valid, cli, packageID).
// A single file may belong to multiple packages due to tests packages.
+ //
+ // Invariant: all IDs present in the ids map exist in the metadata map.
ids map[span.URI][]PackageID
}
diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go
index d932c95..6f60c3b 100644
--- a/gopls/internal/lsp/cache/load.go
+++ b/gopls/internal/lsp/cache/load.go
@@ -36,12 +36,15 @@
//
// The resulting error may wrap the moduleErrorMap error type, representing
// errors associated with specific modules.
+//
+// If scopes contains a file scope there must be exactly one scope.
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
var query []string
var containsDir bool // for logging
+ var standalone bool // whether this is a load of a standalone file
// Keep track of module query -> module path so that we can later correlate query
// errors with errors.
@@ -55,7 +58,14 @@
query = append(query, string(scope))
case fileLoadScope:
+ // Given multiple scopes, the resulting load might contain inaccurate
+ // information. For example go/packages returns at most one command-line
+ // arguments package, and does not handle a combination of standalone
+ // files and packages.
uri := span.URI(scope)
+ if len(scopes) > 1 {
+ panic(fmt.Sprintf("internal error: load called with multiple scopes when a file scope is present (file: %s)", uri))
+ }
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.
@@ -66,6 +76,7 @@
continue
}
if isStandaloneFile(contents, s.view.Options().StandaloneTags) {
+ standalone = true
query = append(query, uri.Filename())
} else {
query = append(query, fmt.Sprintf("file=%s", uri.Filename()))
@@ -144,6 +155,10 @@
return fmt.Errorf("packages.Load error: %w", err)
}
+ if standalone && len(pkgs) > 1 {
+ return bug.Errorf("internal error: go/packages returned multiple packages for standalone file")
+ }
+
// Workaround for a bug (?) that has been in go/packages since
// the outset: Package("unsafe").GoFiles=[], whereas it should
// include unsafe/unsafe.go. Derive it from builtins.go.
@@ -156,9 +171,10 @@
{
var builtin, unsafe *packages.Package
for _, pkg := range pkgs {
- if pkg.ID == "unsafe" {
+ switch pkg.ID {
+ case "unsafe":
unsafe = pkg
- } else if pkg.ID == "builtin" {
+ case "builtin":
builtin = pkg
}
}
@@ -221,7 +237,7 @@
if allFilesExcluded(pkg.GoFiles, filterFunc) {
continue
}
- buildMetadata(newMetadata, pkg, cfg.Dir, query)
+ buildMetadata(newMetadata, pkg, cfg.Dir, standalone)
}
s.mu.Lock()
@@ -372,7 +388,7 @@
rootMod = uri.Filename()
}
rootDir := filepath.Dir(rootMod)
- nestedModules := make(map[string][]source.FileHandle)
+ nestedModules := make(map[string][]*Overlay)
for _, fh := range openFiles {
mod, err := findRootPattern(ctx, filepath.Dir(fh.URI().Filename()), "go.mod", s)
if err != nil {
@@ -401,9 +417,9 @@
// "orphaned". Don't show a general diagnostic in the progress bar,
// because the user may still want to edit a file in a nested module.
var srcDiags []*source.Diagnostic
- for modDir, uris := range nestedModules {
+ for modDir, files := range nestedModules {
msg := fmt.Sprintf("This file is in %s, which is a nested module in the %s module.\n%s", modDir, rootMod, multiModuleMsg)
- srcDiags = append(srcDiags, s.applyCriticalErrorToFiles(ctx, msg, uris)...)
+ srcDiags = append(srcDiags, s.applyCriticalErrorToFiles(ctx, msg, files)...)
}
if len(srcDiags) != 0 {
return fmt.Errorf("You have opened a nested module.\n%s", multiModuleMsg), srcDiags
@@ -412,7 +428,7 @@
return nil, nil
}
-func (s *snapshot) applyCriticalErrorToFiles(ctx context.Context, msg string, files []source.FileHandle) []*source.Diagnostic {
+func (s *snapshot) applyCriticalErrorToFiles(ctx context.Context, msg string, files []*Overlay) []*source.Diagnostic {
var srcDiags []*source.Diagnostic
for _, fh := range files {
// Place the diagnostics on the package or module declarations.
@@ -446,7 +462,7 @@
// buildMetadata populates the updates map with metadata updates to
// apply, based on the given pkg. It recurs through pkg.Imports to ensure that
// metadata exists for all dependencies.
-func buildMetadata(updates map[PackageID]*source.Metadata, pkg *packages.Package, loadDir string, query []string) {
+func buildMetadata(updates map[PackageID]*source.Metadata, pkg *packages.Package, loadDir string, standalone bool) {
// Allow for multiple ad-hoc packages in the workspace (see #47584).
pkgPath := PackagePath(pkg.PkgPath)
id := PackageID(pkg.ID)
@@ -482,6 +498,7 @@
Module: pkg.Module,
Errors: pkg.Errors,
DepsErrors: packagesinternal.GetDepsErrors(pkg),
+ Standalone: standalone,
}
updates[id] = m
@@ -494,6 +511,10 @@
uri := span.URIFromPath(filename)
m.GoFiles = append(m.GoFiles, uri)
}
+ for _, filename := range pkg.IgnoredFiles {
+ uri := span.URIFromPath(filename)
+ m.IgnoredFiles = append(m.IgnoredFiles, uri)
+ }
depsByImpPath := make(map[ImportPath]PackageID)
depsByPkgPath := make(map[PackagePath]PackageID)
@@ -582,7 +603,7 @@
depsByImpPath[importPath] = PackageID(imported.ID)
depsByPkgPath[PackagePath(imported.PkgPath)] = PackageID(imported.ID)
- buildMetadata(updates, imported, loadDir, query)
+ buildMetadata(updates, imported, loadDir, false) // only top level packages can be standalone
}
m.DepsByImpPath = depsByImpPath
m.DepsByPkgPath = depsByPkgPath
@@ -685,7 +706,8 @@
}
for uri := range uris {
- if s.isOpenLocked(uri) {
+ fh, _ := s.files.Get(uri)
+ if _, open := fh.(*Overlay); open {
return true
}
}
diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go
index d824f3c..17f9bdb 100644
--- a/gopls/internal/lsp/cache/session.go
+++ b/gopls/internal/lsp/cache/session.go
@@ -308,7 +308,16 @@
return nil, fmt.Errorf("view %q not found", view.id)
}
- v, _, release, err := s.createView(ctx, view.name, view.folder, options, seqID)
+ v, snapshot, release, err := s.createView(ctx, view.name, view.folder, options, seqID)
+ // The new snapshot has lost the history of the previous view. As a result,
+ // it may not see open files that aren't in its build configuration (as it
+ // would have done via didOpen notifications). This can lead to inconsistent
+ // behavior when configuration is changed mid-session.
+ //
+ // Ensure the new snapshot observes all open files.
+ for _, o := range v.fs.Overlays() {
+ _, _ = snapshot.ReadFile(ctx, o.URI())
+ }
release()
if err != nil {
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 71f6563..2035364 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -10,6 +10,7 @@
"errors"
"fmt"
"go/ast"
+ "go/build/constraint"
"go/token"
"go/types"
"io"
@@ -901,15 +902,9 @@
defer func() {
s.activePackages.Set(id, active, nil) // store the result either way: remember that pkg is not open
}()
- for _, cgf := range pkg.Metadata().GoFiles {
- if s.isOpenLocked(cgf) {
- return pkg
- }
- }
- for _, cgf := range pkg.Metadata().CompiledGoFiles {
- if s.isOpenLocked(cgf) {
- return pkg
- }
+
+ if containsOpenFileLocked(s, pkg.Metadata()) {
+ return pkg
}
return nil
}
@@ -1215,7 +1210,6 @@
//
// The given uri must be a file, not a directory.
func nearestModFile(ctx context.Context, uri span.URI, fs source.FileSource) (span.URI, error) {
- // TODO(rfindley)
dir := filepath.Dir(uri.Filename())
mod, err := findRootPattern(ctx, dir, "go.mod", fs)
if err != nil {
@@ -1261,11 +1255,11 @@
}
}
-// noValidMetadataForURILocked reports whether there is any valid metadata for
-// the given URI.
-func (s *snapshot) noValidMetadataForURILocked(uri span.URI) bool {
+// noRealPackagesForURILocked reports whether there are any
+// non-command-line-arguments packages containing the given URI.
+func (s *snapshot) noRealPackagesForURILocked(uri span.URI) bool {
for _, id := range s.meta.ids[uri] {
- if _, ok := s.meta.metadata[id]; ok {
+ if !source.IsCommandLineArguments(id) || s.meta.metadata[id].Standalone {
return false
}
}
@@ -1351,26 +1345,23 @@
func (s *snapshot) IsOpen(uri span.URI) bool {
s.mu.Lock()
defer s.mu.Unlock()
- return s.isOpenLocked(uri)
-}
-
-func (s *snapshot) openFiles() []source.FileHandle {
- s.mu.Lock()
- defer s.mu.Unlock()
-
- var open []source.FileHandle
- s.files.Range(func(uri span.URI, fh source.FileHandle) {
- if isFileOpen(fh) {
- open = append(open, fh)
- }
- })
+ fh, _ := s.files.Get(uri)
+ _, open := fh.(*Overlay)
return open
}
-func (s *snapshot) isOpenLocked(uri span.URI) bool {
- fh, _ := s.files.Get(uri)
- return isFileOpen(fh)
+func (s *snapshot) openFiles() []*Overlay {
+ s.mu.Lock()
+ defer s.mu.Unlock()
+
+ var open []*Overlay
+ s.files.Range(func(uri span.URI, fh source.FileHandle) {
+ if o, ok := fh.(*Overlay); ok {
+ open = append(open, o)
+ }
+ })
+ return open
}
func isFileOpen(fh source.FileHandle) bool {
@@ -1590,15 +1581,39 @@
//
// An error is returned if the load is canceled.
func (s *snapshot) reloadOrphanedOpenFiles(ctx context.Context) error {
+ s.mu.Lock()
+ meta := s.meta
+ s.mu.Unlock()
// When we load ./... or a package path directly, we may not get packages
// that exist only in overlays. As a workaround, we search all of the files
// available in the snapshot and reload their metadata individually using a
// file= query if the metadata is unavailable.
- files := s.orphanedOpenFiles()
+ open := s.openFiles()
+ var files []*Overlay
+ for _, o := range open {
+ uri := o.URI()
+ if s.IsBuiltin(uri) || s.view.FileKind(o) != source.Go {
+ continue
+ }
+ if len(meta.ids[uri]) == 0 {
+ files = append(files, o)
+ }
+ }
if len(files) == 0 {
return nil
}
+ // Filter to files that are not known to be unloadable.
+ s.mu.Lock()
+ loadable := files[:0]
+ for _, file := range files {
+ if _, unloadable := s.unloadableFiles[file.URI()]; !unloadable {
+ loadable = append(loadable, file)
+ }
+ }
+ files = loadable
+ s.mu.Unlock()
+
var uris []span.URI
for _, file := range files {
uris = append(uris, file.URI())
@@ -1654,7 +1669,7 @@
// TODO(rfindley): instead of locking here, we should have load return the
// metadata graph that resulted from loading.
uri := file.URI()
- if s.noValidMetadataForURILocked(uri) {
+ if len(s.meta.ids) == 0 {
s.unloadableFiles[uri] = struct{}{}
}
}
@@ -1662,34 +1677,133 @@
return nil
}
-func (s *snapshot) orphanedOpenFiles() []source.FileHandle {
+// OrphanedFileDiagnostics reports diagnostics describing why open files have
+// no packages or have only command-line-arguments packages.
+//
+// If the resulting diagnostic is nil, the file is either not orphaned or we
+// can't produce a good diagnostic.
+//
+// TODO(rfindley): reconcile the definition of "orphaned" here with
+// reloadOrphanedFiles. The latter does not include files with
+// command-line-arguments packages.
+func (s *snapshot) OrphanedFileDiagnostics(ctx context.Context) map[span.URI]*source.Diagnostic {
s.mu.Lock()
- defer s.mu.Unlock()
+ meta := s.meta
+ s.mu.Unlock()
- var files []source.FileHandle
- s.files.Range(func(uri span.URI, fh source.FileHandle) {
- // Only consider open files, which will be represented as overlays.
- if _, isOverlay := fh.(*Overlay); !isOverlay {
- return
+ var files []*Overlay
+
+searchOverlays:
+ for _, o := range s.overlays() {
+ uri := o.URI()
+ if s.IsBuiltin(uri) || s.view.FileKind(o) != source.Go {
+ continue
}
- // Don't try to reload metadata for go.mod files.
- if s.view.FileKind(fh) != source.Go {
- return
+ for _, id := range meta.ids[o.URI()] {
+ if !source.IsCommandLineArguments(id) || meta.metadata[id].Standalone {
+ continue searchOverlays
+ }
}
- // If the URI doesn't belong to this view, then it's not in a workspace
- // package and should not be reloaded directly.
- if !source.InDir(s.view.folder.Filename(), uri.Filename()) {
- return
+ files = append(files, o)
+ }
+ if len(files) == 0 {
+ return nil
+ }
+
+ loadedModFiles := make(map[span.URI]struct{})
+ ignoredFiles := make(map[span.URI]bool)
+ for _, meta := range meta.metadata {
+ if meta.Module != nil && meta.Module.GoMod != "" {
+ gomod := span.URIFromPath(meta.Module.GoMod)
+ loadedModFiles[gomod] = struct{}{}
}
- // Don't reload metadata for files we've already deemed unloadable.
- if _, ok := s.unloadableFiles[uri]; ok {
- return
+ for _, ignored := range meta.IgnoredFiles {
+ ignoredFiles[ignored] = true
}
- if s.noValidMetadataForURILocked(uri) {
- files = append(files, fh)
+ }
+
+ diagnostics := make(map[span.URI]*source.Diagnostic)
+ for _, fh := range files {
+ // Only warn about orphaned files if the file is well-formed enough to
+ // actually be part of a package.
+ //
+ // Use ParseGo as for open files this is likely to be a cache hit (we'll have )
+ pgf, err := s.ParseGo(ctx, fh, source.ParseHeader)
+ if err != nil {
+ continue
}
- })
- return files
+ if !pgf.File.Name.Pos().IsValid() {
+ continue
+ }
+ rng, err := pgf.PosRange(pgf.File.Name.Pos(), pgf.File.Name.End())
+ if err != nil {
+ continue
+ }
+
+ // If we have a relevant go.mod file, check whether the file is orphaned
+ // due to its go.mod file being inactive. We could also offer a
+ // prescriptive diagnostic in the case that there is no go.mod file, but it
+ // is harder to be precise in that case, and less important.
+ var msg string
+ if goMod, err := nearestModFile(ctx, fh.URI(), s); err == nil && goMod != "" {
+ if _, ok := loadedModFiles[goMod]; !ok {
+ modDir := filepath.Dir(goMod.Filename())
+ if rel, err := filepath.Rel(s.view.folder.Filename(), modDir); err == nil {
+ modDir = rel
+ }
+
+ var fix string
+ if s.view.goversion >= 18 {
+ if s.view.gowork != "" {
+ fix = fmt.Sprintf("To fix this problem, you can add this module to your go.work file (%s)", s.view.gowork)
+ } else {
+ fix = "To fix this problem, you can add a go.work file that uses this directory."
+ }
+ } else {
+ fix = `To work with multiple modules simultaneously, please upgrade to Go 1.18 or
+later, reinstall gopls, and use a go.work file.`
+ }
+ msg = fmt.Sprintf(`This file is in directory %q, which is not included in your workspace.
+%s
+See the documentation for more information on setting up your workspace:
+https://github.com/golang/tools/blob/master/gopls/doc/workspace.md.`, modDir, fix)
+ }
+ }
+
+ if msg == "" && ignoredFiles[fh.URI()] {
+ // TODO(rfindley): use the constraint package to check if the file
+ // _actually_ satisfies the current build context.
+ hasConstraint := false
+ walkConstraints(pgf.File, func(constraint.Expr) bool {
+ hasConstraint = true
+ return false
+ })
+ var fix string
+ if hasConstraint {
+ fix = `This file may be excluded due to its build tags; try adding "-tags=<build tag>" to your gopls "buildFlags" configuration
+See the documentation for more information on working with build tags:
+https://github.com/golang/tools/blob/master/gopls/doc/settings.md#buildflags-string.`
+ } else if strings.Contains(filepath.Base(fh.URI().Filename()), "_") {
+ fix = `This file may be excluded due to its GOOS/GOARCH, or other build constraints.`
+ } else {
+ fix = `This file is ignored by your gopls build.` // we don't know why
+ }
+ msg = fmt.Sprintf("No packages found for open file %s.\n%s", fh.URI().Filename(), fix)
+ }
+
+ if msg != "" {
+ // Only report diagnostics if we detect an actual exclusion.
+ diagnostics[fh.URI()] = &source.Diagnostic{
+ URI: fh.URI(),
+ Range: rng,
+ Severity: protocol.SeverityWarning,
+ Source: source.ListError,
+ Message: msg,
+ }
+ }
+ }
+
+ return diagnostics
}
// TODO(golang/go#53756): this function needs to consider more than just the
@@ -2347,7 +2461,7 @@
return pgfs[0], nil
}
-func (s *snapshot) IsBuiltin(ctx context.Context, uri span.URI) bool {
+func (s *snapshot) IsBuiltin(uri span.URI) bool {
s.mu.Lock()
defer s.mu.Unlock()
// We should always get the builtin URI in a canonical form, so use simple
diff --git a/gopls/internal/lsp/cache/standalone_go115.go b/gopls/internal/lsp/cache/standalone_go115.go
deleted file mode 100644
index 79569ae..0000000
--- a/gopls/internal/lsp/cache/standalone_go115.go
+++ /dev/null
@@ -1,14 +0,0 @@
-// 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/view.go b/gopls/internal/lsp/cache/view.go
index ae98836..884b0fc 100644
--- a/gopls/internal/lsp/cache/view.go
+++ b/gopls/internal/lsp/cache/view.go
@@ -68,7 +68,7 @@
vulns map[span.URI]*govulncheck.Result
// fs is the file source used to populate this view.
- fs source.FileSource
+ fs *overlayFS
// seenFiles tracks files that the view has accessed.
// TODO(golang/go#57558): this notion is fundamentally problematic, and
diff --git a/gopls/internal/lsp/diagnostics.go b/gopls/internal/lsp/diagnostics.go
index 520549d..26f2242 100644
--- a/gopls/internal/lsp/diagnostics.go
+++ b/gopls/internal/lsp/diagnostics.go
@@ -214,7 +214,7 @@
}
// Don't request type-checking for builtin.go: it's not a real package.
- if snapshot.IsBuiltin(ctx, uri) {
+ if snapshot.IsBuiltin(uri) {
continue
}
@@ -391,15 +391,8 @@
// Orphaned files.
// Confirm that every opened file belongs to a package (if any exist in
// the workspace). Otherwise, add a diagnostic to the file.
- for _, o := range s.session.Overlays() {
- if _, ok := seen[o.URI()]; ok {
- continue
- }
- diagnostic := s.checkForOrphanedFile(ctx, snapshot, o)
- if diagnostic == nil {
- continue
- }
- s.storeDiagnostics(snapshot, o.URI(), orphanedSource, []*source.Diagnostic{diagnostic}, true)
+ for uri, diag := range snapshot.OrphanedFileDiagnostics(ctx) {
+ s.storeDiagnostics(snapshot, uri, orphanedSource, []*source.Diagnostic{diag}, true)
}
}
@@ -475,7 +468,7 @@
// Merge analysis diagnostics with package diagnostics, and store the
// resulting analysis diagnostics.
for uri, adiags := range analysisDiags {
- if snapshot.IsBuiltin(ctx, uri) {
+ if snapshot.IsBuiltin(uri) {
bug.Reportf("go/analysis reported diagnostics for the builtin file: %v", adiags)
continue
}
@@ -506,7 +499,7 @@
}
// builtin.go exists only for documentation purposes, and is not valid Go code.
// Don't report distracting errors
- if snapshot.IsBuiltin(ctx, uri) {
+ if snapshot.IsBuiltin(uri) {
bug.Reportf("type checking reported diagnostics for the builtin file: %v", diags)
continue
}
@@ -668,66 +661,6 @@
}
}
-// checkForOrphanedFile checks that the given URIs can be mapped to packages.
-// If they cannot and the workspace is not otherwise unloaded, it also surfaces
-// a warning, suggesting that the user check the file for build tags.
-func (s *Server) checkForOrphanedFile(ctx context.Context, snapshot source.Snapshot, fh source.FileHandle) *source.Diagnostic {
- // TODO(rfindley): this function may fail to produce a diagnostic for a
- // variety of reasons, some of which should probably not be ignored. For
- // example, should this function be tolerant of the case where fh does not
- // exist, or does not have a package name?
- //
- // It would be better to panic or report a bug in several of the cases below,
- // so that we can move toward guaranteeing we show the user a meaningful
- // error whenever it makes sense.
- if snapshot.View().FileKind(fh) != source.Go {
- return nil
- }
- // builtin files won't have a package, but they are never orphaned.
- if snapshot.IsBuiltin(ctx, fh.URI()) {
- return nil
- }
-
- // This call has the effect of inserting fh into snapshot.files,
- // where for better or worse (actually: just worse) it influences
- // the sets of open, known, and orphaned files.
- snapshot.ReadFile(ctx, fh.URI())
-
- metas, _ := snapshot.MetadataForFile(ctx, fh.URI())
- if len(metas) > 0 || ctx.Err() != nil {
- return nil // file has a package (or cancelled)
- }
- // Inv: file does not belong to a package we know about.
- pgf, err := snapshot.ParseGo(ctx, fh, source.ParseHeader)
- if err != nil {
- return nil
- }
- if !pgf.File.Name.Pos().IsValid() {
- return nil
- }
- rng, err := pgf.NodeRange(pgf.File.Name)
- if err != nil {
- return nil
- }
- // If the file no longer has a name ending in .go, this diagnostic is wrong
- if filepath.Ext(fh.URI().Filename()) != ".go" {
- return nil
- }
- // TODO(rstambler): We should be able to parse the build tags in the
- // file and show a more specific error message. For now, put the diagnostic
- // on the package declaration.
- return &source.Diagnostic{
- URI: fh.URI(),
- Range: rng,
- Severity: protocol.SeverityWarning,
- Source: source.ListError,
- Message: fmt.Sprintf(`No packages found for open file %s: %v.
-If this file contains build tags, try adding "-tags=<build tag>" to your gopls "buildFlags" configuration (see (https://github.com/golang/tools/blob/master/gopls/doc/settings.md#buildflags-string).
-Otherwise, see the troubleshooting guidelines for help investigating (https://github.com/golang/tools/blob/master/gopls/doc/troubleshooting.md).
-`, fh.URI().Filename(), err),
- }
-}
-
// publishDiagnostics collects and publishes any unpublished diagnostic reports.
func (s *Server) publishDiagnostics(ctx context.Context, final bool, snapshot source.Snapshot) {
ctx, done := event.Start(ctx, "Server.publishDiagnostics", source.SnapshotLabels(snapshot)...)
diff --git a/gopls/internal/lsp/regtest/marker.go b/gopls/internal/lsp/regtest/marker.go
index bbab437..ddd05af 100644
--- a/gopls/internal/lsp/regtest/marker.go
+++ b/gopls/internal/lsp/regtest/marker.go
@@ -18,6 +18,7 @@
"path/filepath"
"reflect"
"regexp"
+ "runtime"
"sort"
"strings"
"testing"
@@ -105,6 +106,10 @@
// -cgo requires that CGO_ENABLED is set and the cgo tool is available
// -write_sumfile=a,b,c instructs the test runner to generate go.sum files
// in these directories before running the test.
+// -skip_goos=a,b,c instructs the test runner to skip the test for the
+// listed GOOS values.
+// TODO(rfindley): using build constraint expressions for -skip_goos would
+// be clearer.
// TODO(rfindley): support flag values containing whitespace.
// - "settings.json": this file is parsed as JSON, and used as the
// session configuration (see gopls/doc/settings.md)
@@ -338,6 +343,12 @@
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
+ for _, goos := range test.skipGOOS {
+ if runtime.GOOS == goos {
+ t.Skipf("skipping on %s due to -skip_goos", runtime.GOOS)
+ }
+ }
+
// TODO(rfindley): it may be more useful to have full support for build
// constraints.
if test.minGoVersion != "" {
@@ -361,16 +372,9 @@
config.Settings["diagnosticsDelay"] = "10ms"
}
- var writeGoSum []string
- if test.writeGoSum != "" {
- for _, d := range strings.Split(test.writeGoSum, ",") {
- writeGoSum = append(writeGoSum, strings.TrimSpace(d))
- }
- }
-
run := &markerTestRun{
test: test,
- env: newEnv(t, cache, test.files, test.proxyFiles, writeGoSum, config),
+ env: newEnv(t, cache, test.files, test.proxyFiles, test.writeGoSum, config),
locations: make(map[expect.Identifier]protocol.Location),
diags: make(map[protocol.Location][]protocol.Diagnostic),
}
@@ -575,7 +579,8 @@
// Parsed flags values.
minGoVersion string
cgo bool
- writeGoSum string // comma separated dirs to write go sum for
+ writeGoSum []string // comma separated dirs to write go sum for
+ skipGOOS []string // comma separated GOOS values to skip
}
// flagSet returns the flagset used for parsing the special "flags" file in the
@@ -584,10 +589,27 @@
flags := flag.NewFlagSet(t.name, flag.ContinueOnError)
flags.StringVar(&t.minGoVersion, "min_go", "", "if set, the minimum go1.X version required for this test")
flags.BoolVar(&t.cgo, "cgo", false, "if set, requires cgo (both the cgo tool and CGO_ENABLED=1)")
- flags.StringVar(&t.writeGoSum, "write_sumfile", "", "if set, write the sumfile for these directories")
+ flags.Var((*stringListValue)(&t.writeGoSum), "write_sumfile", "if set, write the sumfile for these directories")
+ flags.Var((*stringListValue)(&t.skipGOOS), "skip_goos", "if set, skip this test on these GOOS values")
return flags
}
+// stringListValue implements flag.Value.
+type stringListValue []string
+
+func (l *stringListValue) Set(s string) error {
+ if s != "" {
+ for _, d := range strings.Split(s, ",") {
+ *l = append(*l, strings.TrimSpace(d))
+ }
+ }
+ return nil
+}
+
+func (l stringListValue) String() string {
+ return strings.Join([]string(l), ",")
+}
+
func (t *markerTest) getGolden(id string) *Golden {
golden, ok := t.golden[id]
// If there was no golden content for this identifier, we must create one
diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go
index b2a2ebd..2a16ad6 100644
--- a/gopls/internal/lsp/source/view.go
+++ b/gopls/internal/lsp/source/view.go
@@ -151,7 +151,7 @@
BuiltinFile(ctx context.Context) (*ParsedGoFile, error)
// IsBuiltin reports whether uri is part of the builtin package.
- IsBuiltin(ctx context.Context, uri span.URI) bool
+ IsBuiltin(uri span.URI) bool
// CriticalError returns any critical errors in the workspace.
//
@@ -207,6 +207,10 @@
// It returns an error if the context was cancelled.
MetadataForFile(ctx context.Context, uri span.URI) ([]*Metadata, error)
+ // OrphanedFileDiagnostics reports diagnostics for files that have no package
+ // associations or which only have only command-line-arguments packages.
+ OrphanedFileDiagnostics(ctx context.Context) map[span.URI]*Diagnostic
+
// -- package type-checking --
// TypeCheck parses and type-checks the specified packages,
@@ -534,20 +538,25 @@
// An ad-hoc package (without go.mod or GOPATH) has its ID, PkgPath,
// and LoadDir equal to the absolute path of its directory.
type Metadata struct {
- ID PackageID
- PkgPath PackagePath
- Name PackageName
+ ID PackageID
+ PkgPath PackagePath
+ Name PackageName
+
+ // these three fields are as defined by go/packages.Package
GoFiles []span.URI
CompiledGoFiles []span.URI
- ForTest PackagePath // package path under test, or ""
- TypesSizes types.Sizes
- Errors []packages.Error // must be set for packages in import cycles
- DepsByImpPath map[ImportPath]PackageID // may contain dups; empty ID => missing
- DepsByPkgPath map[PackagePath]PackageID // values are unique and non-empty
- Module *packages.Module
- DepsErrors []*packagesinternal.PackageError
- Diagnostics []*Diagnostic // processed diagnostics from 'go list'
- LoadDir string // directory from which go/packages was run
+ IgnoredFiles []span.URI
+
+ ForTest PackagePath // package path under test, or ""
+ TypesSizes types.Sizes
+ Errors []packages.Error // must be set for packages in import cycles
+ DepsByImpPath map[ImportPath]PackageID // may contain dups; empty ID => missing
+ DepsByPkgPath map[PackagePath]PackageID // values are unique and non-empty
+ Module *packages.Module
+ DepsErrors []*packagesinternal.PackageError
+ Diagnostics []*Diagnostic // processed diagnostics from 'go list'
+ LoadDir string // directory from which go/packages was run
+ Standalone bool // package synthesized for a standalone file (e.g. ignore-tagged)
}
func (m *Metadata) String() string { return string(m.ID) }
diff --git a/gopls/internal/regtest/marker/testdata/diagnostics/excludedfile.txt b/gopls/internal/regtest/marker/testdata/diagnostics/excludedfile.txt
new file mode 100644
index 0000000..5944cbe
--- /dev/null
+++ b/gopls/internal/regtest/marker/testdata/diagnostics/excludedfile.txt
@@ -0,0 +1,38 @@
+This test demonstrates diagnostics for various forms of file exclusion.
+
+Skip on plan9, an arbitrary GOOS, so that we can exercise GOOS exclusions
+resulting from file suffixes.
+
+-- flags --
+-min_go=go1.18
+-skip_goos=plan9
+
+-- go.work --
+go 1.21
+
+use (
+ ./a
+)
+-- a/go.mod --
+module mod.com/a
+
+go 1.18
+
+-- a/a.go --
+package a
+
+-- a/a_plan9.go --
+package a //@diag(re"package (a)", re"excluded due to its GOOS/GOARCH")
+
+-- a/a_ignored.go --
+//go:build skip
+package a //@diag(re"package (a)", re"excluded due to its build tags")
+
+-- b/go.mod --
+module mod.com/b
+
+go 1.18
+
+-- b/b.go --
+package b //@diag(re"package (b)", re"add this module to your go.work")
+
diff --git a/gopls/internal/regtest/marker/testdata/quickfix/addgowork.txt b/gopls/internal/regtest/marker/testdata/quickfix/addgowork.txt
new file mode 100644
index 0000000..dbd1a95
--- /dev/null
+++ b/gopls/internal/regtest/marker/testdata/quickfix/addgowork.txt
@@ -0,0 +1,44 @@
+This test demonstrates the quick-fix for adding a go.work file.
+
+TODO(rfindley): actually add quick-fixes here.
+TODO(rfindley): improve the "cannot find package" import errors.
+
+-- flags --
+-min_go=go1.18
+
+-- a/go.mod --
+module mod.com/a
+
+go 1.18
+
+-- a/main.go --
+package main //@diag("main", re"add a go.work file")
+
+import "mod.com/a/lib" //@diag("\"mod.com", re"cannot find package")
+
+func main() {
+ _ = lib.C
+}
+
+-- a/lib/lib.go --
+package lib //@diag("lib", re"add a go.work file")
+
+const C = "b"
+-- b/go.mod --
+module mod.com/b
+
+go 1.18
+
+-- b/main.go --
+package main //@diag("main", re"add a go.work file")
+
+import "mod.com/b/lib" //@diag("\"mod.com", re"cannot find package")
+
+func main() {
+ _ = lib.C
+}
+
+-- b/lib/lib.go --
+package lib //@diag("lib", re"add a go.work file")
+
+const C = "b"
diff --git a/gopls/internal/regtest/marker/testdata/quickfix/usemodule.txt b/gopls/internal/regtest/marker/testdata/quickfix/usemodule.txt
new file mode 100644
index 0000000..62f4efc
--- /dev/null
+++ b/gopls/internal/regtest/marker/testdata/quickfix/usemodule.txt
@@ -0,0 +1,49 @@
+This test demonstrates the quick-fix for using a module directory.
+
+TODO(rfindley): actually add quick-fixes here.
+
+-- flags --
+-min_go=go1.18
+
+-- go.work --
+go 1.21
+
+use (
+ ./a
+)
+-- a/go.mod --
+module mod.com/a
+
+go 1.18
+
+-- a/main.go --
+package main
+
+import "mod.com/a/lib"
+
+func main() {
+ _ = lib.C
+}
+
+-- a/lib/lib.go --
+package lib
+
+const C = "b"
+-- b/go.mod --
+module mod.com/b
+
+go 1.18
+
+-- b/main.go --
+package main //@diag("main", re"add this module to your go.work")
+
+import "mod.com/b/lib" //@diag("\"mod.com", re"not included in a workspace module")
+
+func main() {
+ _ = lib.C
+}
+
+-- b/lib/lib.go --
+package lib //@diag("lib", re"add this module to your go.work")
+
+const C = "b"
diff --git a/gopls/internal/regtest/misc/references_test.go b/gopls/internal/regtest/misc/references_test.go
index cffbd60..1e14f1b 100644
--- a/gopls/internal/regtest/misc/references_test.go
+++ b/gopls/internal/regtest/misc/references_test.go
@@ -569,7 +569,7 @@
got := make([]string, 0, len(locs))
for _, loc := range locs {
path := env.Sandbox.Workdir.URIToPath(loc.URI) // (slashified)
- if i := strings.Index(path, "/src/"); i >= 0 && filepath.IsAbs(path) {
+ if i := strings.LastIndex(path, "/src/"); i >= 0 && filepath.IsAbs(path) {
// Absolute path with "src" segment: assume it's in GOROOT.
// Strip directory and don't add line/column since they are fragile.
path = "std:" + path[i+len("/src/"):]
diff --git a/gopls/internal/regtest/workspace/metadata_test.go b/gopls/internal/regtest/workspace/metadata_test.go
index ff72beb..cd91da8 100644
--- a/gopls/internal/regtest/workspace/metadata_test.go
+++ b/gopls/internal/regtest/workspace/metadata_test.go
@@ -97,7 +97,7 @@
// packages for bar.go
env.RegexpReplace("bar.go", "ignore", "excluded")
env.AfterChange(
- Diagnostics(env.AtRegexp("bar.go", "package (main)"), WithMessage("No packages")),
+ Diagnostics(env.AtRegexp("bar.go", "package (main)"), WithMessage("not included in your workspace")),
)
})
}
To view, visit change 494675. To unsubscribe, or for help writing mail filters, visit settings.
Reply all
Reply to author
Forward
0 new messages