internal/lsp: make sure diagnostics only refer to existing files

We were previously sending diagnostics for nonexistent files, and then
adding them to the snapshot in the process. Remove this behavior, and
add a regression test. Case insensitive filesystems were too confusing
to write a test for, but fortunately, Filippo reported another instance
of this bug, so I used that for the regression test.

Fixes golang/go#38602

Change-Id: I4ef6b51944f3338e838875a5aafffd066e8392f4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/230315
Run-TryBot: Rebecca Stambler <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>
Reviewed-by: Robert Findley <[email protected]>

Author: stamblerre

internal/lsp/cache/load.go

@@ -92,8 +92,11 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error {
 	if ctx.Err() != nil {
 		return ctx.Err()
 	}
-
-	event.Log(ctx, "go/packages.Load", tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs)))
+	if err != nil {
+		event.Error(ctx, "go/packages.Load", err, tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs)))
+	} else {
+		event.Log(ctx, "go/packages.Load", tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs)))
+	}
 	if len(pkgs) == 0 {
 		return err
 	}

internal/lsp/cache/snapshot.go

@@ -500,6 +500,17 @@ func (s *snapshot) isWorkspacePackage(id packageID) (packagePath, bool) {
 	scope, ok := s.workspacePackages[id]
 	return scope, ok
 }
+func (s *snapshot) FindFile(uri span.URI) source.FileHandle {
+	f, err := s.view.getFile(uri)
+	if err != nil {
+		return nil
+	}
+
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	return s.files[f.URI()]
+}
 
 // GetFile returns a File for the given URI. It will always succeed because it
 // adds the file to the managed set if needed.

internal/lsp/fake/editor_test.go

@@ -48,7 +48,7 @@ func main() {
 `
 
 func TestClientEditing(t *testing.T) {
-	ws, err := NewSandbox("TestClientEditing", exampleProgram, "")
+	ws, err := NewSandbox("TestClientEditing", exampleProgram, "", false)
 	if err != nil {
 		t.Fatal(err)
 	}

internal/lsp/fake/sandbox.go

@@ -32,7 +32,7 @@ type Sandbox struct {
 // working directory populated by the txtar-encoded content in srctxt, and a
 // file-based module proxy populated with the txtar-encoded content in
 // proxytxt.
-func NewSandbox(name, srctxt, proxytxt string, env ...string) (_ *Sandbox, err error) {
+func NewSandbox(name, srctxt, proxytxt string, inGopath bool, env ...string) (_ *Sandbox, err error) {
 	sb := &Sandbox{
 		name: name,
 		env:  env,
@@ -48,16 +48,23 @@ func NewSandbox(name, srctxt, proxytxt string, env ...string) (_ *Sandbox, err e
 		return nil, fmt.Errorf("creating temporary workdir: %v", err)
 	}
 	sb.basedir = basedir
-	sb.gopath = filepath.Join(sb.basedir, "gopath")
-	workdir := filepath.Join(sb.basedir, "work")
 	proxydir := filepath.Join(sb.basedir, "proxy")
-	for _, subdir := range []string{sb.gopath, workdir, proxydir} {
+	sb.gopath = filepath.Join(sb.basedir, "gopath")
+	// Set the working directory as $GOPATH/src if inGopath is true.
+	workdir := filepath.Join(sb.gopath, "src")
+	dirs := []string{sb.gopath, proxydir}
+	if !inGopath {
+		workdir = filepath.Join(sb.basedir, "work")
+		dirs = append(dirs, workdir)
+	}
+	for _, subdir := range dirs {
 		if err := os.Mkdir(subdir, 0755); err != nil {
 			return nil, err
 		}
 	}
 	sb.Proxy, err = NewProxy(proxydir, proxytxt)
 	sb.Workdir, err = NewWorkdir(workdir, srctxt)
+
 	return sb, nil
 }
 

internal/lsp/lsprpc/lsprpc_test.go

@@ -190,7 +190,7 @@ func TestDebugInfoLifecycle(t *testing.T) {
 	resetExitFuncs := OverrideExitFuncsForTest()
 	defer resetExitFuncs()
 
-	sb, err := fake.NewSandbox("gopls-lsprpc-test", exampleProgram, "")
+	sb, err := fake.NewSandbox("gopls-lsprpc-test", exampleProgram, "", false)
 	if err != nil {
 		t.Fatal(err)
 	}

internal/lsp/regtest/diagnostics_test.go

@@ -5,6 +5,8 @@
 package regtest
 
 import (
+	"fmt"
+	"os"
 	"testing"
 
 	"golang.org/x/tools/internal/lsp"
@@ -291,7 +293,7 @@ func main() {}
 			// package renaming was fully processed. Therefore, in order for this
 			// test to actually exercise the bug, we must wait until that work has
 			// completed.
-			EmptyDiagnostics("a.go"),
+			NoDiagnostics("a.go"),
 			CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), 1),
 		)
 	})
@@ -405,6 +407,7 @@ var X = 0
 	}, WithEnv("GOFLAGS=-tags=foo"))
 }
 
+// Tests golang/go#38467.
 func TestNoSuggestedFixesForGeneratedFiles_Issue38467(t *testing.T) {
 	const generated = `
 -- go.mod --
@@ -441,36 +444,78 @@ func _() {
 	})
 }
 
-// Expect a module/gopath error if there is an error in the file at startup
+// Expect a module/GOPATH error if there is an error in the file at startup.
+// Tests golang/go#37279.
 func TestShowMessage_Issue37279(t *testing.T) {
 	const noModule = `
 -- a.go --
-	package foo
-	
-	func f() {
-		fmt.Printl()
-	}
-	`
+package foo
+
+func f() {
+	fmt.Printl()
+}
+`
 	runner.Run(t, noModule, func(t *testing.T, env *Env) {
 		env.OpenFile("a.go")
 		env.Await(env.DiagnosticAtRegexp("a.go", "fmt.Printl"), SomeShowMessage(""))
 	})
 }
 
-// Expecxt no module/gopath error if there is no error in the file
+// Expect no module/GOPATH error if there is no error in the file.
+// Tests golang/go#37279.
 func TestNoShowMessage_Issue37279(t *testing.T) {
 	const noModule = `
 -- a.go --
-	package foo
-	
-	func f() {
-	}
-	`
+package foo
+
+func f() {
+}
+`
 	runner.Run(t, noModule, func(t *testing.T, env *Env) {
 		env.OpenFile("a.go")
-		env.Await(EmptyDiagnostics("a.go"), EmptyShowMessage(""))
+		env.Await(NoDiagnostics("a.go"), EmptyShowMessage(""))
 		// introduce an error, expect no Show Message
 		env.RegexpReplace("a.go", "func", "fun")
 		env.Await(env.DiagnosticAtRegexp("a.go", "fun"), EmptyShowMessage(""))
 	})
 }
+
+// Tests golang/go#38602.
+func TestNonexistentFileDiagnostics_Issue38602(t *testing.T) {
+	const collision = `
+-- x/x.go --
+package x
+
+import "x/hello"
+
+func Hello() {
+	hello.HiThere()
+}
+-- x/main.go --
+package main
+
+func main() {
+	fmt.Println("")
+}
+`
+	runner.Run(t, collision, func(t *testing.T, env *Env) {
+		env.OpenFile("x/main.go")
+		env.Await(
+			env.DiagnosticAtRegexp("x/main.go", "fmt.Println"),
+		)
+		env.OrganizeImports("x/main.go")
+		// span.Parse misparses the error message when multiple packages are
+		// defined in the same directory, creating a garbage filename.
+		// Previously, we would send diagnostics for this nonexistent file.
+		// This test checks that we don't send diagnostics for this file.
+		dir, err := os.Getwd()
+		if err != nil {
+			t.Fatal(err)
+		}
+		badFile := fmt.Sprintf("%s/found packages main (main.go) and x (x.go) in %s/src/x", dir, env.Sandbox.GOPATH())
+		env.Await(
+			EmptyDiagnostics("x/main.go"),
+			NoDiagnostics(badFile),
+		)
+	}, InGOPATH())
+}

internal/lsp/regtest/env.go

@@ -461,11 +461,26 @@ func (e DiagnosticExpectation) Description() string {
 	return fmt.Sprintf("%s: %s", e.path, e.description)
 }
 
-// EmptyDiagnostics asserts that diagnostics are empty for the
+// EmptyDiagnostics asserts that empty diagnostics are sent for the
 // workspace-relative path name.
 func EmptyDiagnostics(name string) Expectation {
 	check := func(s State) (Verdict, interface{}) {
-		if diags, ok := s.diagnostics[name]; !ok || len(diags.Diagnostics) == 0 {
+		if diags := s.diagnostics[name]; diags != nil && len(diags.Diagnostics) == 0 {
+			return Met, nil
+		}
+		return Unmet, nil
+	}
+	return SimpleExpectation{
+		check:       check,
+		description: "empty diagnostics",
+	}
+}
+
+// NoDiagnostics asserts that no diagnostics are sent for the
+// workspace-relative path name.
+func NoDiagnostics(name string) Expectation {
+	check := func(s State) (Verdict, interface{}) {
+		if _, ok := s.diagnostics[name]; !ok {
 			return Met, nil
 		}
 		return Unmet, nil

internal/lsp/regtest/runner.go

@@ -71,6 +71,7 @@ type runConfig struct {
 	timeout     time.Duration
 	env         []string
 	skipCleanup bool
+	gopath      bool
 }
 
 func (r *Runner) defaultConfig() *runConfig {
@@ -120,6 +121,14 @@ func WithEnv(env ...string) RunOption {
 	})
 }
 
+// InGOPATH configures the workspace working directory to be GOPATH, rather
+// than a separate working directory for use with modules.
+func InGOPATH() RunOption {
+	return optionSetter(func(opts *runConfig) {
+		opts.gopath = true
+	})
+}
+
 // SkipCleanup is used only for debugging: is skips cleaning up the tests state
 // after completion.
 func SkipCleanup() RunOption {
@@ -159,7 +168,7 @@ func (r *Runner) Run(t *testing.T, filedata string, test func(t *testing.T, e *E
 			defer cancel()
 			ctx = debug.WithInstance(ctx, "", "")
 
-			sandbox, err := fake.NewSandbox("regtest", filedata, config.proxyTxt, config.env...)
+			sandbox, err := fake.NewSandbox("regtest", filedata, config.proxyTxt, config.gopath, config.env...)
 			if err != nil {
 				t.Fatal(err)
 			}

internal/lsp/source/diagnostics.go

@@ -84,9 +84,7 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, ph PackageHandle, missi
 	// Prepare the reports we will send for the files in this package.
 	reports := make(map[FileIdentity][]*Diagnostic)
 	for _, fh := range pkg.CompiledGoFiles() {
-		if err := clearReports(snapshot, reports, fh.File().Identity().URI); err != nil {
-			return nil, warn, err
-		}
+		clearReports(snapshot, reports, fh.File().Identity().URI)
 		if len(missing) > 0 {
 			if err := missingModulesDiagnostics(ctx, snapshot, reports, missing, fh.File().Identity().URI); err != nil {
 				return nil, warn, err
@@ -107,9 +105,7 @@ func Diagnostics(ctx context.Context, snapshot Snapshot, ph PackageHandle, missi
 				}
 			}
 		}
-		if err := clearReports(snapshot, reports, e.URI); err != nil {
-			return nil, warn, err
-		}
+		clearReports(snapshot, reports, e.URI)
 	}
 	// Run diagnostics for the package that this URI belongs to.
 	hadDiagnostics, hadTypeErrors, err := diagnostics(ctx, snapshot, reports, pkg, len(ph.MissingDependencies()) > 0)
@@ -307,30 +303,29 @@ func analyses(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][
 	return nil
 }
 
-func clearReports(snapshot Snapshot, reports map[FileIdentity][]*Diagnostic, uri span.URI) error {
+func clearReports(snapshot Snapshot, reports map[FileIdentity][]*Diagnostic, uri span.URI) {
 	if snapshot.View().Ignore(uri) {
-		return nil
+		return
 	}
-	fh, err := snapshot.GetFile(uri)
-	if err != nil {
-		return err
+	fh := snapshot.FindFile(uri)
+	if fh == nil {
+		return
 	}
 	reports[fh.Identity()] = []*Diagnostic{}
-	return nil
 }
 
 func addReports(snapshot Snapshot, reports map[FileIdentity][]*Diagnostic, uri span.URI, diagnostics ...*Diagnostic) error {
 	if snapshot.View().Ignore(uri) {
 		return nil
 	}
-	fh, err := snapshot.GetFile(uri)
-	if err != nil {
-		return err
+	fh := snapshot.FindFile(uri)
+	if fh == nil {
+		return nil
 	}
 	identity := fh.Identity()
 	existingDiagnostics, ok := reports[identity]
 	if !ok {
-		return errors.Errorf("diagnostics for unexpected file %s", uri)
+		return fmt.Errorf("diagnostics for unexpected file %s", uri)
 	}
 	if len(diagnostics) == 1 {
 		d1 := diagnostics[0]

internal/lsp/source/view.go

@@ -31,8 +31,12 @@ type Snapshot interface {
 	// Config returns the configuration for the view.
 	Config(ctx context.Context) *packages.Config
 
-	// GetFile returns the file object for a given URI, initializing it
-	// if it is not already part of the view.
+	// FindFile returns the FileHandle for the given URI, if it is already
+	// in the given snapshot.
+	FindFile(uri span.URI) FileHandle
+
+	// GetFile returns the FileHandle for a given URI, initializing it
+	// if it is not already part of the snapshot.
 	GetFile(uri span.URI) (FileHandle, error)
 
 	// IsOpen returns whether the editor currently has a file open.