internal/lsp: set correct directness when adding new requires

When adding a require, we should add an `// indirect` comment if that's
what go mod tidy would do.

It's possible I should split Add out from Update and Remove, but this
was quick and easy and I'm not too worried about it for now.

Also minimize the test that covered this case, which was way more
complicated than it needed to be AFAICT.

Fixes golang/go#38914.

Change-Id: I89c44f8573873227c4c9e637d1d31d8c1a6530aa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/267578
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>

Author: heschi

gopls/internal/regtest/modfile_test.go

@@ -324,9 +324,7 @@ func main() {
 func TestBadlyVersionedModule(t *testing.T) {
 	testenv.NeedsGo1Point(t, 14)
 
-	const badModule = `
--- example.com/blah/@v/list --
-v1.0.0
+	const proxy = `
 -- example.com/blah/@v/v1.0.0.mod --
 module example.com
 
@@ -335,55 +333,33 @@ go 1.12
 package blah
 
 const Name = "Blah"
--- example.com/blah@v1.0.0/blah_test.go --
-package blah_test
-
-import (
-	"testing"
-)
-
-func TestBlah(t *testing.T) {}
-
--- example.com/blah/v2/@v/list --
-v2.0.0
 -- example.com/blah/v2/@v/v2.0.0.mod --
 module example.com
 
 go 1.12
 -- example.com/blah/v2@v2.0.0/blah.go --
 package blah
 
-const Name = "Blah"
--- example.com/blah/v2@v2.0.0/blah_test.go --
-package blah_test
-
-import (
-	"testing"
-
-	"example.com/blah"
-)
+import "example.com/blah"
 
-func TestBlah(t *testing.T) {}
+var _ = blah.Name
+const Name = "Blah"
 `
-	const pkg = `
+	const files = `
 -- go.mod --
 module mod.com
 
 go 1.12
 
-require (
-	example.com/blah/v2 v2.0.0
-)
+require example.com/blah/v2 v2.0.0
 -- main.go --
 package main
 
 import "example.com/blah/v2"
 
-func main() {
-	println(blah.Name)
-}
+var _ = blah.Name
 `
-	runner.Run(t, pkg, func(t *testing.T, env *Env) {
+	withOptions(WithProxyFiles(proxy)).run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 		env.OpenFile("go.mod")
 		var d protocol.PublishDiagnosticsParams
@@ -399,7 +375,7 @@ func main() {
 go 1.12
 
 require (
-	example.com/blah v1.0.0
+	example.com/blah v1.0.0 // indirect
 	example.com/blah/v2 v2.0.0
 )
 `
@@ -409,7 +385,7 @@ require (
 		if got := env.ReadWorkspaceFile("go.mod"); got != want {
 			t.Fatalf("suggested fixes failed:\n%s", tests.Diff(want, got))
 		}
-	}, WithProxyFiles(badModule))
+	})
 }
 
 // Reproduces golang/go#38232.

internal/lsp/cache/mod_tidy.go

@@ -353,7 +353,7 @@ func unusedError(m *protocol.ColumnMapper, req *modfile.Require, computeEdits di
 	if err != nil {
 		return source.Error{}, err
 	}
-	args, err := source.MarshalArgs(m.URI, []string{req.Mod.Path + "@none"})
+	args, err := source.MarshalArgs(m.URI, false, []string{req.Mod.Path + "@none"})
 	if err != nil {
 		return source.Error{}, err
 	}
@@ -425,7 +425,7 @@ func missingModuleError(snapshot source.Snapshot, pm *source.ParsedModule, req *
 			return source.Error{}, err
 		}
 	}
-	args, err := source.MarshalArgs(pm.Mapper.URI, []string{req.Mod.Path + "@" + req.Mod.Version})
+	args, err := source.MarshalArgs(pm.Mapper.URI, !req.Indirect, []string{req.Mod.Path + "@" + req.Mod.Version})
 	if err != nil {
 		return source.Error{}, err
 	}

internal/lsp/command.go

@@ -200,13 +200,14 @@ func (s *Server) runCommand(ctx context.Context, work *workDone, command *source
 	case source.CommandAddDependency, source.CommandUpgradeDependency, source.CommandRemoveDependency:
 		var uri protocol.DocumentURI
 		var goCmdArgs []string
-		if err := source.UnmarshalArgs(args, &uri, &goCmdArgs); err != nil {
+		var addRequire bool
+		if err := source.UnmarshalArgs(args, &uri, &addRequire, &goCmdArgs); err != nil {
 			return err
 		}
-		if command == source.CommandAddDependency {
+		if addRequire {
 			// Using go get to create a new dependency results in an
-			// `// indirect` comment we don't want. The only way to avoid it is
-			// to add the require as direct first. Then we can use go get to
+			// `// indirect` comment we may not want. The only way to avoid it
+			// is to add the require as direct first. Then we can use go get to
 			// update go.sum and tidy up.
 			if err := s.directGoModCommand(ctx, uri, "mod", append([]string{"edit", "-require"}, goCmdArgs...)...); err != nil {
 				return err

internal/lsp/mod/code_lens.go

@@ -52,7 +52,7 @@ func upgradeLens(ctx context.Context, snapshot source.Snapshot, fh source.FileHa
 		if err != nil {
 			return nil, err
 		}
-		upgradeDepArgs, err := source.MarshalArgs(fh.URI(), []string{dep})
+		upgradeDepArgs, err := source.MarshalArgs(fh.URI(), false, []string{dep})
 		if err != nil {
 			return nil, err
 		}
@@ -69,7 +69,7 @@ func upgradeLens(ctx context.Context, snapshot source.Snapshot, fh source.FileHa
 	// If there is at least 1 upgrade, add "Upgrade all dependencies" to
 	// the module statement.
 	if len(allUpgrades) > 0 {
-		upgradeDepArgs, err := source.MarshalArgs(fh.URI(), append([]string{"-u"}, allUpgrades...))
+		upgradeDepArgs, err := source.MarshalArgs(fh.URI(), false, append([]string{"-u"}, allUpgrades...))
 		if err != nil {
 			return nil, err
 		}