From f31efc5a5c28ff793741668348ea4bc36d9e499e Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Sun, 6 Dec 2020 23:45:00 -0500 Subject: [PATCH] internal/lsp: add an orphaned file diagnostic for nested modules This change adds a diagnostic and error message that appears when a user edits a nested module in legacy mode. At first, I thought a diagnostic would be enough, but it's actually quite difficult to spot it when you have a bunch of "undeclared name" diagnostics caused by the nested module, so I figured a progress bar error message would also be useful. This error message just indicates to the user that they should open the nested module as its own workspace folder. Also, while debugging this, I noticed that command-line-arguments packages can have test variants, which we were never handling. So I addressed that in this change. Fixes golang/go#42109 Change-Id: Ifa6f6af401a3725835c09b76e35f889ec5cb8901 Reviewed-on: https://go-review.googlesource.com/c/tools/+/275554 Trust: Rebecca Stambler Run-TryBot: Rebecca Stambler TryBot-Result: Go Bot Reviewed-by: Robert Findley --- gopls/internal/regtest/diagnostics_test.go | 70 ++++++++++++++++- internal/lsp/cache/load.go | 89 +++++++++++++++------- internal/lsp/cache/snapshot.go | 37 +++++++-- internal/lsp/code_action.go | 4 +- internal/lsp/diagnostics.go | 10 +-- internal/lsp/source/view.go | 2 +- 6 files changed, 168 insertions(+), 44 deletions(-) diff --git a/gopls/internal/regtest/diagnostics_test.go b/gopls/internal/regtest/diagnostics_test.go index e52f564056..db52e4b1bd 100644 --- a/gopls/internal/regtest/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics_test.go @@ -1664,7 +1664,7 @@ package b env.Await( env.DiagnosticAtRegexp("a/a.go", "package a"), env.DiagnosticAtRegexp("b/go.mod", "module b.com"), - OutstandingWork(lsp.WorkspaceLoadFailure, "gopls requires one module per workspace folder."), + OutstandingWork(lsp.WorkspaceLoadFailure, "gopls requires a module at the root of your workspace."), ) }) }) @@ -1693,6 +1693,74 @@ package b }) } +func TestNestedModules(t *testing.T) { + const proxy = ` +-- nested.com@v1.0.0/go.mod -- +module nested.com + +go 1.12 +-- nested.com@v1.0.0/hello/hello.go -- +package hello + +func Hello() {} +` + + const nested = ` +-- go.mod -- +module mod.com + +go 1.12 + +require nested.com v1.0.0 +-- go.sum -- +nested.com v1.0.0 h1:I6spLE4CgFqMdBPc+wTV2asDO2QJ3tU0YAT+jkLeN1I= +nested.com v1.0.0/go.mod h1:ly53UzXQgVjSlV7wicdBB4p8BxfytuGT1Xcyv0ReJfI= +-- main.go -- +package main + +import "nested.com/hello" + +func main() { + hello.Hello() +} +-- nested/go.mod -- +module nested.com + +-- nested/hello/hello.go -- +package hello + +func Hello() { + helloHelper() +} +-- nested/hello/hello_helper.go -- +package hello + +func helloHelper() {} +` + withOptions( + WithProxyFiles(proxy), + WithModes(Singleton), + ).run(t, nested, func(t *testing.T, env *Env) { + // Expect a diagnostic in a nested module. + env.OpenFile("nested/hello/hello.go") + didOpen := CompletedWork(lsp.DiagnosticWorkTitle(lsp.FromDidOpen), 1) + env.Await( + OnceMet( + didOpen, + env.DiagnosticAtRegexp("nested/hello/hello.go", "helloHelper"), + ), + OnceMet( + didOpen, + env.DiagnosticAtRegexpWithMessage("nested/hello/hello.go", "package hello", "nested module"), + ), + OnceMet( + didOpen, + OutstandingWork(lsp.WorkspaceLoadFailure, "nested module"), + ), + ) + }) +} + func TestAdHocPackagesReloading(t *testing.T) { const nomod = ` -- main.go -- diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 6bc2c91473..cdcc3aea07 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -54,7 +54,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...interf for _, scope := range scopes { switch scope := scope.(type) { case packagePath: - if scope == "command-line-arguments" { + if isCommandLineArguments(string(scope)) { panic("attempted to load command-line-arguments") } // The only time we pass package paths is when we're doing a @@ -204,12 +204,6 @@ func (s *snapshot) parseLoadError(ctx context.Context, loadErr error) *source.Cr // workspaceLayoutErrors returns a diagnostic for every open file, as well as // an error message if there are no open files. func (s *snapshot) WorkspaceLayoutError(ctx context.Context) *source.CriticalError { - // Assume the workspace is misconfigured only if we've detected an invalid - // build configuration. Currently, a valid build configuration is either a - // module at the root of the view or a GOPATH workspace. - if s.ValidBuildConfiguration() { - return nil - } if len(s.workspace.getKnownModFiles()) == 0 { return nil } @@ -219,33 +213,70 @@ func (s *snapshot) WorkspaceLayoutError(ctx context.Context) *source.CriticalErr if s.workspace.moduleSource != legacyWorkspace { return nil } - // The user's workspace contains go.mod files and they have - // GO111MODULE=on, so we should guide them to create a - // workspace folder for each module. - - // Add a diagnostic to every open file, or return a general error if - // there aren't any. - var open []source.VersionedFileHandle - s.mu.Lock() - for _, fh := range s.files { - if s.isOpenLocked(fh.URI()) { - open = append(open, fh) - } + // If the user has one module per view, there is nothing to warn about. + if s.ValidBuildConfiguration() && len(s.workspace.getKnownModFiles()) == 1 { + return nil } - s.mu.Unlock() - msg := `gopls requires one module per workspace folder. + // Apply diagnostics about the workspace configuration to relevant open + // files. + openFiles := s.openFiles() + + // If the snapshot does not have a valid build configuration, it may be + // that the user has opened a directory that contains multiple modules. + // Check for that an warn about it. + if !s.ValidBuildConfiguration() { + msg := `gopls requires a module at the root of your workspace. You can work with multiple modules by opening each one as a workspace folder. Improvements to this workflow will be coming soon (https://github.com/golang/go/issues/32394), and you can learn more here: https://github.com/golang/go/issues/36899.` + return &source.CriticalError{ + MainError: errors.Errorf(msg), + ErrorList: s.applyCriticalErrorToFiles(ctx, msg, openFiles), + } + } - criticalError := &source.CriticalError{ - MainError: errors.New(msg), + // If the user has one active go.mod file, they may still be editing files + // in nested modules. Check the module of each open file and add warnings + // that the nested module must be opened as a workspace folder. + if len(s.workspace.getActiveModFiles()) == 1 { + // Get the active root go.mod file to compare against. + var rootModURI span.URI + for uri := range s.workspace.getActiveModFiles() { + rootModURI = uri + } + nestedModules := map[span.URI][]source.VersionedFileHandle{} + for _, fh := range openFiles { + modURI := moduleForURI(s.workspace.knownModFiles, fh.URI()) + if modURI != rootModURI { + nestedModules[modURI] = append(nestedModules[modURI], fh) + } + } + // Add a diagnostic to each file in a nested module to mark it as + // "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 srcErrs []*source.Error + for modURI, uris := range nestedModules { + msg := fmt.Sprintf(`This file is in %s, which is a nested module in the %s module. +gopls currently requires one module per workspace folder. +Please open %s as a separate workspace folder. +You can learn more here: https://github.com/golang/go/issues/36899. +`, modURI.Filename(), rootModURI.Filename(), modURI.Filename()) + srcErrs = append(srcErrs, s.applyCriticalErrorToFiles(ctx, msg, uris)...) + } + if len(srcErrs) != 0 { + return &source.CriticalError{ + MainError: errors.Errorf(`You are working in a nested module. Please open it as a separate workspace folder.`), + ErrorList: srcErrs, + } + } } - if len(open) == 0 { - return criticalError - } - for _, fh := range open { + return nil +} + +func (s *snapshot) applyCriticalErrorToFiles(ctx context.Context, msg string, files []source.VersionedFileHandle) []*source.Error { + var srcErrs []*source.Error + for _, fh := range files { // Place the diagnostics on the package or module declarations. var rng protocol.Range switch fh.Kind() { @@ -263,14 +294,14 @@ and you can learn more here: https://github.com/golang/go/issues/36899.` } } } - criticalError.ErrorList = append(criticalError.ErrorList, &source.Error{ + srcErrs = append(srcErrs, &source.Error{ URI: fh.URI(), Range: rng, Kind: source.ListError, Message: msg, }) } - return criticalError + return srcErrs } type workspaceDirKey string diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index c023defbc8..b06d9c8650 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -294,7 +294,7 @@ func (s *snapshot) goCommandInvocation(ctx context.Context, flags source.Invocat modURI = span.URIFromPath(filepath.Join(tmpDir.Filename(), "go.mod")) } } else { - modURI = s.GoModForFile(ctx, span.URIFromPath(inv.WorkingDir)) + modURI = s.GoModForFile(span.URIFromPath(inv.WorkingDir)) } var modContent []byte @@ -795,9 +795,13 @@ func (s *snapshot) CachedImportPaths(ctx context.Context) (map[string]source.Pac return results, nil } -func (s *snapshot) GoModForFile(ctx context.Context, uri span.URI) span.URI { +func (s *snapshot) GoModForFile(uri span.URI) span.URI { + return moduleForURI(s.workspace.activeModFiles, uri) +} + +func moduleForURI(modFiles map[span.URI]struct{}, uri span.URI) span.URI { var match span.URI - for modURI := range s.workspace.getActiveModFiles() { + for modURI := range modFiles { if !source.InDir(dirURI(modURI).Filename(), uri.Filename()) { continue } @@ -890,7 +894,7 @@ func (s *snapshot) addID(uri span.URI, id packageID) { } // If we are setting a real ID, when the package had only previously // had a command-line-arguments ID, we should just replace it. - if existingID == "command-line-arguments" { + if isCommandLineArguments(string(existingID)) { s.ids[uri][i] = id // Delete command-line-arguments if it was a workspace package. delete(s.workspacePackages, existingID) @@ -900,6 +904,14 @@ func (s *snapshot) addID(uri span.URI, id packageID) { s.ids[uri] = append(s.ids[uri], id) } +// isCommandLineArguments reports whether a given value denotes +// "command-line-arguments" package, which is a package with an unknown ID +// created by the go command. It can have a test variant, which is why callers +// should not check that a value equals "command-line-arguments" directly. +func isCommandLineArguments(s string) bool { + return strings.Contains(s, "command-line-arguments") +} + func (s *snapshot) isWorkspacePackage(id packageID) (packagePath, bool) { s.mu.Lock() defer s.mu.Unlock() @@ -962,6 +974,19 @@ func (s *snapshot) IsOpen(uri span.URI) bool { } +func (s *snapshot) openFiles() []source.VersionedFileHandle { + s.mu.Lock() + defer s.mu.Unlock() + + var open []source.VersionedFileHandle + for _, fh := range s.files { + if s.isOpenLocked(fh.URI()) { + open = append(open, fh) + } + } + return open +} + func (s *snapshot) isOpenLocked(uri span.URI) bool { _, open := s.files[uri].(*overlay) return open @@ -1012,7 +1037,7 @@ func (s *snapshot) reloadWorkspace(ctx context.Context) error { missingMetadata = true // Don't try to reload "command-line-arguments" directly. - if pkgPath == "command-line-arguments" { + if isCommandLineArguments(string(pkgPath)) { continue } pkgPathSet[pkgPath] = struct{}{} @@ -1361,7 +1386,7 @@ copyIDs: // go command when the user is outside of GOPATH and outside of a // module. Do not cache them as workspace packages for longer than // necessary. - if id == "command-line-arguments" { + if isCommandLineArguments(string(id)) { if invalidateMetadata, ok := transitiveIDs[id]; invalidateMetadata && ok { continue } diff --git a/internal/lsp/code_action.go b/internal/lsp/code_action.go index cfeaaa12a4..e5d229bf77 100644 --- a/internal/lsp/code_action.go +++ b/internal/lsp/code_action.go @@ -363,7 +363,7 @@ func diagnosticToAnalyzer(snapshot source.Snapshot, src, msg string) (analyzer * var importErrorRe = regexp.MustCompile(`could not import ([^\s]+)`) func goGetFixes(ctx context.Context, snapshot source.Snapshot, uri span.URI, diagnostics []protocol.Diagnostic) ([]protocol.CodeAction, error) { - if snapshot.GoModForFile(ctx, uri) == "" { + if snapshot.GoModForFile(uri) == "" { // Go get only supports module mode for now. return nil, nil } @@ -510,7 +510,7 @@ func moduleQuickFixes(ctx context.Context, snapshot source.Snapshot, fh source.V case source.Mod: modFH = fh case source.Go: - modURI := snapshot.GoModForFile(ctx, fh.URI()) + modURI := snapshot.GoModForFile(fh.URI()) if modURI == "" { return nil, nil } diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go index 801974dc2c..c6d6b81a8e 100644 --- a/internal/lsp/diagnostics.go +++ b/internal/lsp/diagnostics.go @@ -186,7 +186,7 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, forceAn // with the user's workspace layout. Workspace packages that only have the // ID "command-line-arguments" are usually a symptom of a bad workspace // configuration. - if onlyCommandLineArguments(wsPkgs) { + if containsCommandLineArguments(wsPkgs) { if criticalErr := snapshot.WorkspaceLayoutError(ctx); criticalErr != nil { err = criticalErr } @@ -563,11 +563,11 @@ func (s *Server) shouldIgnoreError(ctx context.Context, snapshot source.Snapshot return !hasGo } -func onlyCommandLineArguments(pkgs []source.Package) bool { +func containsCommandLineArguments(pkgs []source.Package) bool { for _, pkg := range pkgs { - if pkg.ID() != "command-line-arguments" { - return false + if strings.Contains(pkg.ID(), "command-line-arguments") { + return true } } - return true + return false } diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 1676ee09ab..97cf5c5642 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -118,7 +118,7 @@ type Snapshot interface { ModTidy(ctx context.Context, pm *ParsedModule) (*TidiedModule, error) // GoModForFile returns the URI of the go.mod file for the given URI. - GoModForFile(ctx context.Context, uri span.URI) span.URI + GoModForFile(uri span.URI) span.URI // BuiltinPackage returns information about the special builtin package. BuiltinPackage(ctx context.Context) (*BuiltinPackage, error)