gopls/internal/regtest/marker: support proxy files in marker tests

Add new functionality to wire-in a file-based GOPROXY value for marker
tests containing files starting with "proxy/". Use this to port an
example regression test, adding better coverage for go.work support
while doing so.

To solve one of the main pain points from working with proxy files in
the regtests, add a -write_sumfile=dir1,dir2 flag to the test framework,
to allow auto-generation of go.sum files.

Along the way, loosen the diagnostic matching logic, ignoring end
positions.

Change-Id: I8421cea807fd87dcbe6b1619720a46b3f1f7bc3f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/494396
TryBot-Result: Gopher Robot <[email protected]>
gopls-CI: kokoro <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
Run-TryBot: Robert Findley <[email protected]>

Author: findleyr

gopls/internal/lsp/fake/sandbox.go

@@ -254,10 +254,11 @@ func (sb *Sandbox) goCommandInvocation() gocommand.Invocation {
 // RunGoCommand executes a go command in the sandbox. If checkForFileChanges is
 // true, the sandbox scans the working directory and emits file change events
 // for any file changes it finds.
-func (sb *Sandbox) RunGoCommand(ctx context.Context, dir, verb string, args []string, checkForFileChanges bool) error {
+func (sb *Sandbox) RunGoCommand(ctx context.Context, dir, verb string, args, env []string, checkForFileChanges bool) error {
 	inv := sb.goCommandInvocation()
 	inv.Verb = verb
 	inv.Args = args
+	inv.Env = append(inv.Env, env...)
 	if dir != "" {
 		inv.WorkingDir = sb.Workdir.AbsPath(dir)
 	}
@@ -289,7 +290,7 @@ func (sb *Sandbox) GoVersion(ctx context.Context) (int, error) {
 func (sb *Sandbox) Close() error {
 	var goCleanErr error
 	if sb.gopath != "" {
-		goCleanErr = sb.RunGoCommand(context.Background(), "", "clean", []string{"-modcache"}, false)
+		goCleanErr = sb.RunGoCommand(context.Background(), "", "clean", []string{"-modcache"}, nil, false)
 	}
 	err := robustio.RemoveAll(sb.rootdir)
 	if err != nil || goCleanErr != nil {

gopls/internal/lsp/regtest/marker.go

@@ -97,12 +97,14 @@ var update = flag.Bool("update", false, "if set, update test data during marker
 //
 // # Special files
 //
-// There are three types of file within the test archive that are given special
+// There are several types of file within the test archive that are given special
 // treatment by the test runner:
 //   - "flags": this file is treated as a whitespace-separated list of flags
 //     that configure the MarkerTest instance. Supported flags:
 //     -min_go=go1.18 sets the minimum Go version for the test;
 //     -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.
 //     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)
@@ -115,6 +117,9 @@ var update = flag.Bool("update", false, "if set, update test data during marker
 //     Foo were of type *Golden, the test runner would convert the identifier a
 //     in the call @foo(a, "b", 3) into a *Golden by collecting golden file
 //     data starting with "@a/".
+//   - proxy files: any file starting with proxy/ is treated as a Go proxy
+//     file. If present, these files are written to a separate temporary
+//     directory and GOPROXY is set to file://<proxy directory>.
 //
 // # Marker types
 //
@@ -136,10 +141,8 @@ var update = flag.Bool("update", false, "if set, update test data during marker
 //     a 1:1 correspondence between observed diagnostics and diag annotations.
 //     The diagnostics source and kind fields are ignored, to reduce fuss.
 //
-//     The marker must accurately represent the diagnostic's range.
-//     Use grouping parens in the location regular expression to indicate
-//     a portion in context.
-//     TODO(adonovan): make this less strict, like the old framework.
+//     The specified location must match the start position of the diagnostic,
+//     but end positions are ignored.
 //
 //     TODO(adonovan): in the older marker framework, the annotation asserted
 //     two additional fields (source="compiler", kind="error"). Restore them?
@@ -357,10 +360,17 @@ func RunMarkerTests(t *testing.T, dir string) {
 				}
 				config.Settings["diagnosticsDelay"] = "10ms"
 			}
-			run := &markerTestRun{
-				test: test,
-				env:  newEnv(t, cache, test.files, config),
 
+			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),
 				locations: make(map[expect.Identifier]protocol.Location),
 				diags:     make(map[protocol.Location][]protocol.Diagnostic),
 			}
@@ -397,8 +407,11 @@ func RunMarkerTests(t *testing.T, dir string) {
 				uri := run.env.Sandbox.Workdir.URI(path)
 				for _, diag := range params.Diagnostics {
 					loc := protocol.Location{
-						URI:   uri,
-						Range: diag.Range,
+						URI: uri,
+						Range: protocol.Range{
+							Start: diag.Range.Start,
+							End:   diag.Range.Start, // ignore end positions
+						},
 					}
 					run.diags[loc] = append(run.diags[loc], diag)
 				}
@@ -546,21 +559,23 @@ var markerFuncs = map[string]markerFunc{
 // See the documentation for RunMarkerTests for more information on the archive
 // format.
 type markerTest struct {
-	name     string                 // relative path to the txtar file in the testdata dir
-	fset     *token.FileSet         // fileset used for parsing notes
-	content  []byte                 // raw test content
-	archive  *txtar.Archive         // original test archive
-	settings map[string]interface{} // gopls settings
-	env      map[string]string      // editor environment
-	files    map[string][]byte      // data files from the archive (excluding special files)
-	notes    []*expect.Note         // extracted notes from data files
-	golden   map[string]*Golden     // extracted golden content, by identifier name
+	name       string                 // relative path to the txtar file in the testdata dir
+	fset       *token.FileSet         // fileset used for parsing notes
+	content    []byte                 // raw test content
+	archive    *txtar.Archive         // original test archive
+	settings   map[string]interface{} // gopls settings
+	env        map[string]string      // editor environment
+	proxyFiles map[string][]byte      // proxy content
+	files      map[string][]byte      // data files from the archive (excluding special files)
+	notes      []*expect.Note         // extracted notes from data files
+	golden     map[string]*Golden     // extracted golden content, by identifier name
 
 	// flags holds flags extracted from the special "flags" archive file.
 	flags []string
 	// Parsed flags values.
 	minGoVersion string
 	cgo          bool
+	writeGoSum   string // comma separated dirs to write go sum for
 }
 
 // flagSet returns the flagset used for parsing the special "flags" file in the
@@ -569,6 +584,7 @@ func (t *markerTest) flagSet() *flag.FlagSet {
 	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")
 	return flags
 }
 
@@ -711,6 +727,13 @@ func loadMarkerTest(name string, content []byte) (*markerTest, error) {
 			}
 			test.golden[id].data[name] = file.Data
 
+		case strings.HasPrefix(file.Name, "proxy/"):
+			name := file.Name[len("proxy/"):]
+			if test.proxyFiles == nil {
+				test.proxyFiles = make(map[string][]byte)
+			}
+			test.proxyFiles[name] = file.Data
+
 		default: // ordinary file content
 			notes, err := expect.Parse(test.fset, file.Name, file.Data)
 			if err != nil {
@@ -773,6 +796,8 @@ func formatTest(test *markerTest) ([]byte, error) {
 		default:
 			if _, ok := test.files[file.Name]; ok { // ordinary file
 				arch.Files = append(arch.Files, file)
+			} else if strings.HasPrefix(file.Name, "proxy/") { // proxy file
+				arch.Files = append(arch.Files, file)
 			} else if data, ok := updatedGolden[file.Name]; ok { // golden file
 				arch.Files = append(arch.Files, txtar.File{Name: file.Name, Data: data})
 				delete(updatedGolden, file.Name)
@@ -798,16 +823,22 @@ func formatTest(test *markerTest) ([]byte, error) {
 //
 // TODO(rfindley): simplify and refactor the construction of testing
 // environments across regtests, marker tests, and benchmarks.
-func newEnv(t *testing.T, cache *cache.Cache, files map[string][]byte, config fake.EditorConfig) *Env {
+func newEnv(t *testing.T, cache *cache.Cache, files, proxyFiles map[string][]byte, writeGoSum []string, config fake.EditorConfig) *Env {
 	sandbox, err := fake.NewSandbox(&fake.SandboxConfig{
-		RootDir: t.TempDir(),
-		GOPROXY: "https://proxy.golang.org",
-		Files:   files,
+		RootDir:    t.TempDir(),
+		Files:      files,
+		ProxyFiles: proxyFiles,
 	})
 	if err != nil {
 		t.Fatal(err)
 	}
 
+	for _, dir := range writeGoSum {
+		if err := sandbox.RunGoCommand(context.Background(), dir, "list", []string{"-mod=mod", "..."}, []string{"GOWORK=off"}, true); err != nil {
+			t.Fatal(err)
+		}
+	}
+
 	// Put a debug instance in the context to prevent logging to stderr.
 	// See associated TODO in runner.go: we should revisit this pattern.
 	ctx := context.Background()
@@ -851,7 +882,7 @@ type markerTestRun struct {
 	// Collected information.
 	// Each @diag/@suggestedfix marker eliminates an entry from diags.
 	locations map[expect.Identifier]protocol.Location
-	diags     map[protocol.Location][]protocol.Diagnostic
+	diags     map[protocol.Location][]protocol.Diagnostic // diagnostics by position; location end == start
 }
 
 // sprintf returns a formatted string after applying pre-processing to
@@ -1324,7 +1355,14 @@ func diagMarker(mark marker, loc protocol.Location, re *regexp.Regexp) {
 	}
 }
 
+// removeDiagnostic looks for a diagnostic matching loc at the given position.
+//
+// If found, it returns (diag, true), and eliminates the matched diagnostic
+// from the unmatched set.
+//
+// If not found, it returns (protocol.Diagnostic{}, false).
 func removeDiagnostic(mark marker, loc protocol.Location, re *regexp.Regexp) (protocol.Diagnostic, bool) {
+	loc.Range.End = loc.Range.Start // diagnostics ignore end position.
 	diags := mark.run.diags[loc]
 	for i, diag := range diags {
 		if re.MatchString(diag.Message) {
@@ -1444,6 +1482,7 @@ func codeActionErrMarker(mark marker, actionKind string, start, end protocol.Loc
 // the expectation of a diagnostic, but then it applies the first code
 // action of the specified kind suggested by the matched diagnostic.
 func suggestedfixMarker(mark marker, loc protocol.Location, re *regexp.Regexp, actionKind string, golden *Golden) {
+	loc.Range.End = loc.Range.Start // diagnostics ignore end position.
 	// Find and remove the matching diagnostic.
 	diag, ok := removeDiagnostic(mark, loc, re)
 	if !ok {

gopls/internal/lsp/regtest/wrappers.go

@@ -256,7 +256,7 @@ func (e *Env) RunGenerate(dir string) {
 // directory.
 func (e *Env) RunGoCommand(verb string, args ...string) {
 	e.T.Helper()
-	if err := e.Sandbox.RunGoCommand(e.Ctx, "", verb, args, true); err != nil {
+	if err := e.Sandbox.RunGoCommand(e.Ctx, "", verb, args, nil, true); err != nil {
 		e.T.Fatal(err)
 	}
 }
@@ -265,7 +265,7 @@ func (e *Env) RunGoCommand(verb string, args ...string) {
 // relative directory of the sandbox.
 func (e *Env) RunGoCommandInDir(dir, verb string, args ...string) {
 	e.T.Helper()
-	if err := e.Sandbox.RunGoCommand(e.Ctx, dir, verb, args, true); err != nil {
+	if err := e.Sandbox.RunGoCommand(e.Ctx, dir, verb, args, nil, true); err != nil {
 		e.T.Fatal(err)
 	}
 }
@@ -286,7 +286,7 @@ func (e *Env) GoVersion() int {
 func (e *Env) DumpGoSum(dir string) {
 	e.T.Helper()
 
-	if err := e.Sandbox.RunGoCommand(e.Ctx, dir, "list", []string{"-mod=mod", "..."}, true); err != nil {
+	if err := e.Sandbox.RunGoCommand(e.Ctx, dir, "list", []string{"-mod=mod", "..."}, nil, true); err != nil {
 		e.T.Fatal(err)
 	}
 	sumFile := path.Join(dir, "/go.sum")

gopls/internal/regtest/diagnostics/diagnostics_test.go

@@ -299,7 +299,7 @@ func Hello() {
 				InitialWorkspaceLoad,
 				Diagnostics(env.AtRegexp("main.go", `"mod.com/bob"`)),
 			)
-			if err := env.Sandbox.RunGoCommand(env.Ctx, "", "mod", []string{"init", "mod.com"}, true); err != nil {
+			if err := env.Sandbox.RunGoCommand(env.Ctx, "", "mod", []string{"init", "mod.com"}, nil, true); err != nil {
 				t.Fatal(err)
 			}
 			env.AfterChange(

gopls/internal/regtest/marker/testdata/undeclaredname/undeclared.txt renamed to gopls/internal/regtest/marker/testdata/quickfix/undeclared.txt

@@ -0,0 +1,24 @@
+This test checks the suggested fix to remove unused require statements from
+go.mod files.
+
+-- flags --
+-write_sumfile=a
+
+-- proxy/[email protected]/x.go --
+package pkg
+const X = 1
+
+-- a/go.mod --
+module mod.com
+
+go 1.14
+
+require example.com v1.0.0 //@suggestedfix("require", re"not used", "quickfix", a)
+
+-- @a/a/go.mod --
+module mod.com
+
+go 1.14
+-- a/main.go --
+package main
+func main() {}

gopls/internal/regtest/marker/testdata/quickfix/unusedrequire.txt

@@ -0,0 +1,49 @@
+This test checks the suggested fix to remove unused require statements from
+go.mod files, when a go.work file is used.
+
+Note that unlike unusedrequire.txt, we need not write go.sum files when
+a go.work file is used.
+
+-- flags --
+-min_go=go1.18
+
+-- proxy/[email protected]/x.go --
+package pkg
+const X = 1
+
+-- go.work --
+go 1.21
+
+use (
+	./a
+	./b
+)
+-- a/go.mod --
+module mod.com/a
+
+go 1.14
+
+require example.com v1.0.0 //@suggestedfix("require", re"not used", "quickfix", a)
+
+-- @a/a/go.mod --
+module mod.com/a
+
+go 1.14
+-- a/main.go --
+package main
+func main() {}
+
+-- b/go.mod --
+module mod.com/b
+
+go 1.14
+
+require example.com v1.0.0 //@suggestedfix("require", re"not used", "quickfix", b)
+
+-- @b/b/go.mod --
+module mod.com/b
+
+go 1.14
+-- b/main.go --
+package main
+func main() {}

gopls/internal/regtest/marker/testdata/quickfix/unusedrequire_gowork.txt

@@ -476,7 +476,7 @@ const _ = b.K
 		}
 
 		// Run 'go mod vendor' outside the editor.
-		if err := env.Sandbox.RunGoCommand(env.Ctx, ".", "mod", []string{"vendor"}, true); err != nil {
+		if err := env.Sandbox.RunGoCommand(env.Ctx, ".", "mod", []string{"vendor"}, nil, true); err != nil {
 			t.Fatalf("go mod vendor: %v", err)
 		}
 

gopls/internal/regtest/misc/definition_test.go

@@ -419,7 +419,7 @@ var _ b.B
 		checkVendor(env.Implementations(refLoc), false)
 
 		// Run 'go mod vendor' outside the editor.
-		if err := env.Sandbox.RunGoCommand(env.Ctx, ".", "mod", []string{"vendor"}, true); err != nil {
+		if err := env.Sandbox.RunGoCommand(env.Ctx, ".", "mod", []string{"vendor"}, nil, true); err != nil {
 			t.Fatalf("go mod vendor: %v", err)
 		}
 

gopls/internal/regtest/misc/references_test.go

@@ -336,48 +336,6 @@ require example.com v1.2.3
 	})
 }
 
-func TestUnusedDiag(t *testing.T) {
-
-	const proxy = `
--- [email protected]/x.go --
-package pkg
-const X = 1
-`
-	const files = `
--- a/go.mod --
-module mod.com
-go 1.14
-require example.com v1.0.0
--- a/go.sum --
-example.com v1.0.0 h1:38O7j5rEBajXk+Q5wzLbRN7KqMkSgEiN9NqcM1O2bBM=
-example.com v1.0.0/go.mod h1:vUsPMGpx9ZXXzECCOsOmYCW7npJTwuA16yl89n3Mgls=
--- a/main.go --
-package main
-func main() {}
-`
-
-	const want = `module mod.com
-
-go 1.14
-`
-
-	RunMultiple{
-		{"default", WithOptions(ProxyFiles(proxy), WorkspaceFolders("a"))},
-		{"nested", WithOptions(ProxyFiles(proxy))},
-	}.Run(t, files, func(t *testing.T, env *Env) {
-		env.OpenFile("a/go.mod")
-		var d protocol.PublishDiagnosticsParams
-		env.AfterChange(
-			Diagnostics(env.AtRegexp("a/go.mod", `require example.com`)),
-			ReadDiagnostics("a/go.mod", &d),
-		)
-		env.ApplyQuickFixes("a/go.mod", d.Diagnostics)
-		if got := env.BufferText("a/go.mod"); got != want {
-			t.Fatalf("unexpected go.mod content:\n%s", compare.Text(want, got))
-		}
-	})
-}
-
 // Test to reproduce golang/go#39041. It adds a new require to a go.mod file
 // that already has an unused require.
 func TestNewDepWithUnusedDep(t *testing.T) {