From 43adb69d7e9407736bb2eda6bc96c80299187b0e Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Thu, 19 Nov 2020 15:51:09 -0500 Subject: [PATCH] internal/lsp: disable network access for some go commands For users with poor network connections, go command invocations that access the network may hang up for long periods of time. Even for users with good network connections, accessing proxy.golang.org or github can take a few seconds, which is a long time to lock up an editor. Disable network access via GOPROXY=off when running most go commands. There are two notable exceptions. First, the initial workspace load is allowed, though subsequent loads are not. My reasoning is that if the user is opening a project they've just downloaded, they probably expect to fetch its dependencies. (Also, it's convenient to keep the regtests going.) The second is the go mod tidy diagnostics, which I hope to address in a later change. All the other commands that access the network are at the user's request. When the workspace load fails due to a dependency that hasn't been downloaded, we present a quick fix on the go.mod line to do so. The only way that happens is if there's a manual modification to the go.mod file, since all of the other quick fixes will do the download. So I don't think there's a need to present the fix anywhere else. Updates golang/go#38462. Change-Id: I470bc1ba790db7c1afee54d0b28fa0c6fd203fb9 Reviewed-on: https://go-review.googlesource.com/c/tools/+/274120 Trust: Heschi Kreinick Run-TryBot: Heschi Kreinick gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler Reviewed-by: Robert Findley --- gopls/internal/regtest/modfile_test.go | 20 ++++++--------- gopls/internal/regtest/workspace_test.go | 32 ++++++++++++++++++------ internal/lsp/cache/load.go | 8 ++++-- internal/lsp/cache/mod.go | 22 +++++++++++++++- internal/lsp/cache/mod_tidy.go | 2 +- internal/lsp/cache/snapshot.go | 19 ++++++++------ internal/lsp/cache/view.go | 2 +- internal/lsp/command.go | 8 +++--- internal/lsp/mod/diagnostics.go | 9 ++++++- internal/lsp/source/view.go | 23 +++++++++++++---- 10 files changed, 103 insertions(+), 42 deletions(-) diff --git a/gopls/internal/regtest/modfile_test.go b/gopls/internal/regtest/modfile_test.go index 34e4080e05..338296e78c 100644 --- a/gopls/internal/regtest/modfile_test.go +++ b/gopls/internal/regtest/modfile_test.go @@ -444,18 +444,14 @@ func main() { env.RegexpReplace("go.mod", "v1.2.2", "v1.2.3") env.Editor.SaveBuffer(env.Ctx, "go.mod") // go.mod changes must be on disk - // Bring the go.sum file back into sync. Multi-module workspace - // mode currently doesn't set -mod=readonly, so won't need to. - if !strings.Contains(t.Name(), "experimental") { - d := protocol.PublishDiagnosticsParams{} - env.Await( - OnceMet( - env.DiagnosticAtRegexp("go.mod", "module"), - ReadDiagnostics("go.mod", &d), - ), - ) - env.ApplyQuickFixes("go.mod", d.Diagnostics) - } + d := protocol.PublishDiagnosticsParams{} + env.Await( + OnceMet( + env.DiagnosticAtRegexpWithMessage("go.mod", "example.com v1.2.3", "example.com@v1.2.3"), + ReadDiagnostics("go.mod", &d), + ), + ) + env.ApplyQuickFixes("go.mod", d.Diagnostics) env.Await( EmptyDiagnostics("go.mod"), diff --git a/gopls/internal/regtest/workspace_test.go b/gopls/internal/regtest/workspace_test.go index 1f584adc28..512114b9d9 100644 --- a/gopls/internal/regtest/workspace_test.go +++ b/gopls/internal/regtest/workspace_test.go @@ -14,6 +14,7 @@ import ( "golang.org/x/tools/internal/lsp" "golang.org/x/tools/internal/lsp/fake" + "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/testenv" ) @@ -272,6 +273,18 @@ func Hello() int { env.Await( CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), 2), ) + if testenv.Go1Point() < 14 { + // On 1.14 and above, the go mod tidy diagnostics accidentally + // download for us. This is the behavior we actually want. + d := protocol.PublishDiagnosticsParams{} + env.Await( + OnceMet( + env.DiagnosticAtRegexpWithMessage("moda/a/go.mod", "require b.com v1.2.3", "b.com@v1.2.3"), + ReadDiagnostics("moda/a/go.mod", &d), + ), + ) + env.ApplyQuickFixes("moda/a/go.mod", d.Diagnostics) + } got, _ := env.GoToDefinition("moda/a/a.go", env.RegexpSearch("moda/a/a.go", "Hello")) if want := "b.com@v1.2.3/b/b.go"; !strings.HasSuffix(got, want) { t.Errorf("expected %s, got %v", want, got) @@ -448,16 +461,20 @@ require ( replace a.com => %s/moda/a replace b.com => %s/modb `, workdir, workdir)) + env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), 1)) + // Check that go.mod diagnostics picked up the newly active mod file. + // The local version of modb has an extra dependency we need to download. + env.OpenFile("modb/go.mod") + var d protocol.PublishDiagnosticsParams env.Await( OnceMet( - CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChangeWatchedFiles), 1), - env.DiagnosticAtRegexp("modb/b/b.go", "x"), + env.DiagnosticAtRegexp("modb/go.mod", `require example.com v1.2.3`), + ReadDiagnostics("modb/go.mod", &d), ), ) - env.OpenFile("modb/go.mod") - // Check that go.mod diagnostics picked up the newly active mod file. - env.Await(env.DiagnosticAtRegexp("modb/go.mod", `require example.com v1.2.3`)) - // ...and that jumping to definition now goes to b.com in the workspace. + env.ApplyQuickFixes("modb/go.mod", d.Diagnostics) + env.Await(env.DiagnosticAtRegexp("modb/b/b.go", "x")) + // Jumping to definition should now go to b.com in the workspace. location, _ = env.GoToDefinition("moda/a/a.go", env.RegexpSearch("moda/a/a.go", "Hello")) if want := "modb/b/b.go"; !strings.HasSuffix(location, want) { t.Errorf("expected %s, got %v", want, location) @@ -474,9 +491,8 @@ require ( replace a.com => %s/moda/a `, workdir)) - env.Await(CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), 1)) env.Await(OnceMet( - CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), 1), + CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidChange), 2), EmptyDiagnostics("modb/go.mod"), )) diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 6a579fa04b..a127c69cf8 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -47,7 +47,7 @@ type metadata struct { // load calls packages.Load for the given scopes, updating package metadata, // import graph, and mapped files with the result. -func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { +func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interface{}) error { var query []string var containsDir bool // for logging for _, scope := range scopes { @@ -94,7 +94,11 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { ctx, done := event.Start(ctx, "cache.view.load", tag.Query.Of(query)) defer done() - _, inv, cleanup, err := s.goCommandInvocation(ctx, source.LoadWorkspace, &gocommand.Invocation{ + flags := source.LoadWorkspace + if allowNetwork { + flags |= source.AllowNetwork + } + _, inv, cleanup, err := s.goCommandInvocation(ctx, flags, &gocommand.Invocation{ WorkingDir: s.view.rootURI.Filename(), }) if err != nil { diff --git a/internal/lsp/cache/mod.go b/internal/lsp/cache/mod.go index 87a4baefd7..a5f0e1e134 100644 --- a/internal/lsp/cache/mod.go +++ b/internal/lsp/cache/mod.go @@ -290,7 +290,7 @@ func (s *snapshot) ModUpgrade(ctx context.Context, fh source.FileHandle) (map[st // (see golang/go#38711). inv.ModFlag = "readonly" } - stdout, err := snapshot.RunGoCommandDirect(ctx, source.Normal, inv) + stdout, err := snapshot.RunGoCommandDirect(ctx, source.Normal|source.AllowNetwork, inv) if err != nil { return &modUpgradeData{err: err} } @@ -387,6 +387,26 @@ func (s *snapshot) extractGoCommandError(ctx context.Context, snapshot source.Sn if err != nil { return nil } + if v.Path != "" && strings.Contains(goCmdError, "disabled by GOPROXY=off") { + args, err := source.MarshalArgs(fh.URI(), false, []string{fmt.Sprintf("%v@%v", v.Path, v.Version)}) + if err != nil { + return nil + } + return &source.Error{ + Message: fmt.Sprintf("%v@%v has not been downloaded", v.Path, v.Version), + Kind: source.ListError, + Range: rng, + URI: fh.URI(), + SuggestedFixes: []source.SuggestedFix{{ + Title: fmt.Sprintf("Download %v@%v", v.Path, v.Version), + Command: &protocol.Command{ + Title: source.CommandAddDependency.Title, + Command: source.CommandAddDependency.ID(), + Arguments: args, + }, + }}, + } + } return &source.Error{ Message: goCmdError, Range: rng, diff --git a/internal/lsp/cache/mod_tidy.go b/internal/lsp/cache/mod_tidy.go index 54f2180719..b613b838e7 100644 --- a/internal/lsp/cache/mod_tidy.go +++ b/internal/lsp/cache/mod_tidy.go @@ -105,7 +105,7 @@ func (s *snapshot) ModTidy(ctx context.Context, pm *source.ParsedModule) (*sourc Args: []string{"tidy"}, WorkingDir: filepath.Dir(fh.URI().Filename()), } - tmpURI, inv, cleanup, err := snapshot.goCommandInvocation(ctx, source.WriteTemporaryModFile, inv) + tmpURI, inv, cleanup, err := snapshot.goCommandInvocation(ctx, source.WriteTemporaryModFile|source.AllowNetwork, inv) if err != nil { return &modTidyData{err: err} } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 956daf1aef..79313176d5 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -228,7 +228,7 @@ func (s *snapshot) config(ctx context.Context, inv *gocommand.Invocation) *packa return cfg } -func (s *snapshot) RunGoCommandDirect(ctx context.Context, mode source.InvocationMode, inv *gocommand.Invocation) (*bytes.Buffer, error) { +func (s *snapshot) RunGoCommandDirect(ctx context.Context, mode source.InvocationFlags, inv *gocommand.Invocation) (*bytes.Buffer, error) { _, inv, cleanup, err := s.goCommandInvocation(ctx, mode, inv) if err != nil { return nil, err @@ -238,7 +238,7 @@ func (s *snapshot) RunGoCommandDirect(ctx context.Context, mode source.Invocatio return s.view.session.gocmdRunner.Run(ctx, *inv) } -func (s *snapshot) RunGoCommandPiped(ctx context.Context, mode source.InvocationMode, inv *gocommand.Invocation, stdout, stderr io.Writer) error { +func (s *snapshot) RunGoCommandPiped(ctx context.Context, mode source.InvocationFlags, inv *gocommand.Invocation, stdout, stderr io.Writer) error { _, inv, cleanup, err := s.goCommandInvocation(ctx, mode, inv) if err != nil { return err @@ -247,7 +247,7 @@ func (s *snapshot) RunGoCommandPiped(ctx context.Context, mode source.Invocation return s.view.session.gocmdRunner.RunPiped(ctx, *inv, stdout, stderr) } -func (s *snapshot) goCommandInvocation(ctx context.Context, mode source.InvocationMode, inv *gocommand.Invocation) (tmpURI span.URI, updatedInv *gocommand.Invocation, cleanup func(), err error) { +func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.InvocationFlags, inv *gocommand.Invocation) (tmpURI span.URI, updatedInv *gocommand.Invocation, cleanup func(), err error) { s.view.optionsMu.Lock() inv.Env = append(append(append(os.Environ(), s.view.options.EnvSlice()...), inv.Env...), "GO111MODULE="+s.view.go111module) inv.BuildFlags = append([]string{}, s.view.options.BuildFlags...) @@ -259,6 +259,11 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, mode source.Invocati return "", inv, cleanup, nil } + mode, allowNetwork := flags.Mode(), flags.AllowNetwork() + if !allowNetwork { + inv.Env = append(inv.Env, "GOPROXY=off") + } + var modURI span.URI // Select the module context to use. // If we're type checking, we need to use the workspace context, meaning @@ -453,7 +458,7 @@ func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode // calls to packages.Load. Determine what we should do instead. } if reload { - if err := s.load(ctx, fileURI(uri)); err != nil { + if err := s.load(ctx, false, fileURI(uri)); err != nil { return nil, err } } @@ -1008,7 +1013,7 @@ func (s *snapshot) reloadWorkspace(ctx context.Context) error { // If the view's build configuration is invalid, we cannot reload by // package path. Just reload the directory instead. if missingMetadata && !s.ValidBuildConfiguration() { - return s.load(ctx, viewLoadScope("LOAD_INVALID_VIEW")) + return s.load(ctx, false, viewLoadScope("LOAD_INVALID_VIEW")) } if len(pkgPathSet) == 0 { @@ -1019,7 +1024,7 @@ func (s *snapshot) reloadWorkspace(ctx context.Context) error { for pkgPath := range pkgPathSet { pkgPaths = append(pkgPaths, pkgPath) } - return s.load(ctx, pkgPaths...) + return s.load(ctx, false, pkgPaths...) } func (s *snapshot) reloadOrphanedFiles(ctx context.Context) error { @@ -1032,7 +1037,7 @@ func (s *snapshot) reloadOrphanedFiles(ctx context.Context) error { return nil } - err := s.load(ctx, scopes...) + err := s.load(ctx, false, scopes...) // If we failed to load some files, i.e. they have no metadata, // mark the failures so we don't bother retrying until the file's diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 132acd1d59..0a54ee3e2b 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -548,7 +548,7 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) { if len(scopes) == 0 { scopes = append(scopes, viewLoadScope("LOAD_VIEW")) } - err := s.load(ctx, append(scopes, packagePath("builtin"))...) + err := s.load(ctx, firstAttempt, append(scopes, packagePath("builtin"))...) if ctx.Err() != nil { return } diff --git a/internal/lsp/command.go b/internal/lsp/command.go index 037433570f..6044d1305a 100644 --- a/internal/lsp/command.go +++ b/internal/lsp/command.go @@ -203,7 +203,7 @@ func (s *Server) runCommand(ctx context.Context, work *workDone, command *source if command == source.CommandVendor { action = "vendor" } - return runSimpleGoCommand(ctx, snapshot, source.UpdateUserModFile, uri.SpanURI(), "mod", []string{action}) + return runSimpleGoCommand(ctx, snapshot, source.UpdateUserModFile|source.AllowNetwork, uri.SpanURI(), "mod", []string{action}) case source.CommandUpdateGoSum: var uri protocol.DocumentURI if err := source.UnmarshalArgs(args, &uri); err != nil { @@ -214,7 +214,7 @@ func (s *Server) runCommand(ctx context.Context, work *workDone, command *source if !ok { return err } - return runSimpleGoCommand(ctx, snapshot, source.UpdateUserModFile, uri.SpanURI(), "list", []string{"all"}) + return runSimpleGoCommand(ctx, snapshot, source.UpdateUserModFile|source.AllowNetwork, uri.SpanURI(), "list", []string{"all"}) case source.CommandAddDependency, source.CommandUpgradeDependency, source.CommandRemoveDependency: var uri protocol.DocumentURI var goCmdArgs []string @@ -397,10 +397,10 @@ func (s *Server) runGoGetModule(ctx context.Context, snapshot source.Snapshot, u return err } } - return runSimpleGoCommand(ctx, snapshot, source.UpdateUserModFile, uri, "get", append([]string{"-d"}, args...)) + return runSimpleGoCommand(ctx, snapshot, source.UpdateUserModFile|source.AllowNetwork, uri, "get", append([]string{"-d"}, args...)) } -func runSimpleGoCommand(ctx context.Context, snapshot source.Snapshot, mode source.InvocationMode, uri span.URI, verb string, args []string) error { +func runSimpleGoCommand(ctx context.Context, snapshot source.Snapshot, mode source.InvocationFlags, uri span.URI, verb string, args []string) error { _, err := snapshot.RunGoCommandDirect(ctx, mode, &gocommand.Invocation{ Verb: verb, Args: args, diff --git a/internal/lsp/mod/diagnostics.go b/internal/lsp/mod/diagnostics.go index 23f00599de..4ab70762cf 100644 --- a/internal/lsp/mod/diagnostics.go +++ b/internal/lsp/mod/diagnostics.go @@ -13,6 +13,7 @@ import ( "golang.org/x/tools/internal/lsp/debug/tag" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" + errors "golang.org/x/xerrors" ) func Diagnostics(ctx context.Context, snapshot source.Snapshot) (map[source.VersionedFileIdentity][]*source.Diagnostic, error) { @@ -36,7 +37,7 @@ func Diagnostics(ctx context.Context, snapshot source.Snapshot) (map[source.Vers Range: e.Range, Source: e.Category, } - if e.Category == "syntax" { + if e.Category == "syntax" || e.Kind == source.ListError { d.Severity = protocol.SeverityError } else { d.Severity = protocol.SeverityWarning @@ -65,6 +66,12 @@ func ErrorsForMod(ctx context.Context, snapshot source.Snapshot, fh source.FileH return nil, nil } if err != nil { + // Some error messages can also be displayed as diagnostics. + if criticalErr := (*source.CriticalError)(nil); errors.As(err, &criticalErr) { + return criticalErr.ErrorList, nil + } else if srcErrList := (source.ErrorList)(nil); errors.As(err, &srcErrList) { + return srcErrList, nil + } return nil, err } return tidied.Errors, nil diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index a196b2fb4e..56a8c77d4b 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -84,11 +84,11 @@ type Snapshot interface { // RunGoCommandPiped runs the given `go` command, writing its output // to stdout and stderr. Verb, Args, and WorkingDir must be specified. - RunGoCommandPiped(ctx context.Context, mode InvocationMode, inv *gocommand.Invocation, stdout, stderr io.Writer) error + RunGoCommandPiped(ctx context.Context, mode InvocationFlags, inv *gocommand.Invocation, stdout, stderr io.Writer) error // RunGoCommandDirect runs the given `go` command. Verb, Args, and // WorkingDir must be specified. - RunGoCommandDirect(ctx context.Context, mode InvocationMode, inv *gocommand.Invocation) (*bytes.Buffer, error) + RunGoCommandDirect(ctx context.Context, mode InvocationFlags, inv *gocommand.Invocation) (*bytes.Buffer, error) // RunProcessEnvFunc runs fn with the process env for this snapshot's view. // Note: the process env contains cached module and filesystem state. @@ -161,13 +161,14 @@ const ( WidestPackage ) -// InvocationMode represents the goal of a particular go command invocation. -type InvocationMode int +// InvocationFlags represents the settings of a particular go command invocation. +// It is a mode, plus a set of flag bits. +type InvocationFlags int const ( // Normal is appropriate for commands that might be run by a user and don't // deliberately modify go.mod files, e.g. `go test`. - Normal InvocationMode = iota + Normal InvocationFlags = iota // UpdateUserModFile is for commands that intend to update the user's real // go.mod file, e.g. `go mod tidy` in response to a user's request to tidy. UpdateUserModFile @@ -178,8 +179,20 @@ const ( // LoadWorkspace is for packages.Load, and other operations that should // consider the whole workspace at once. LoadWorkspace + + // AllowNetwork is a flag bit that indicates the invocation should be + // allowed to access the network. + AllowNetwork = 1 << 10 ) +func (m InvocationFlags) Mode() InvocationFlags { + return m & (AllowNetwork - 1) +} + +func (m InvocationFlags) AllowNetwork() bool { + return m&AllowNetwork != 0 +} + // View represents a single workspace. // This is the level at which we maintain configuration like working directory // and build tags.