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 <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

Author: stamblerre

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 --

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.`
-
-	criticalError := &source.CriticalError{
-		MainError: errors.New(msg),
+		return &source.CriticalError{
+			MainError: errors.Errorf(msg),
+			ErrorList: s.applyCriticalErrorToFiles(ctx, msg, openFiles),
+		}
 	}
-	if len(open) == 0 {
-		return criticalError
+
+	// 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,
+			}
+		}
 	}
-	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

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
 			}

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
 		}

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
 }

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)