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)