internal/lsp/cache: support loading multiple orphaned files

go/packages returns at most one command-line-arguments package per
query. Previously, this led to N^2 loading of orphaned files. CL 480197
fixed this by marking all queried files as unloadable after a load
completes. However, as a result gopls would fail to load multiple
orphaned files.

Fix this properly by calling Load once for each orphaned file.

Fixes golang/go#59318

Change-Id: Ibfb3742fcb70ea3976d8b1b5b384fe6b97350cf4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/494401
Reviewed-by: Alan Donovan <[email protected]>
gopls-CI: kokoro <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>

Author: findleyr

gopls/internal/lsp/cache/load.go

@@ -451,14 +451,12 @@ func buildMetadata(updates map[PackageID]*source.Metadata, pkg *packages.Package
 	pkgPath := PackagePath(pkg.PkgPath)
 	id := PackageID(pkg.ID)
 
-	// TODO(rfindley): this creates at most one command-line-arguments package
-	// per load, but if we pass multiple file= queries to go/packages, there may
-	// be multiple command-line-arguments packages.
-	//
-	// As reported in golang/go#59318, this can result in accidentally quadratic
-	// loading behavior.
 	if source.IsCommandLineArguments(id) {
-		suffix := ":" + strings.Join(query, ",")
+		if len(pkg.CompiledGoFiles) != 1 {
+			bug.Reportf("unexpected files in command-line-arguments package: %v", pkg.CompiledGoFiles)
+			return
+		}
+		suffix := pkg.CompiledGoFiles[0]
 		id = PackageID(pkg.ID + suffix)
 		pkgPath = PackagePath(pkg.PkgPath + suffix)
 	}

gopls/internal/lsp/cache/snapshot.go

@@ -1595,40 +1595,52 @@ func (s *snapshot) reloadOrphanedOpenFiles(ctx context.Context) error {
 	// available in the snapshot and reload their metadata individually using a
 	// file= query if the metadata is unavailable.
 	files := s.orphanedOpenFiles()
+	if len(files) == 0 {
+		return nil
+	}
 
-	// Files without a valid package declaration can't be loaded. Don't try.
-	var scopes []loadScope
+	var uris []span.URI
 	for _, file := range files {
-		pgf, err := s.ParseGo(ctx, file, source.ParseHeader)
-		if err != nil {
-			continue
-		}
-		if !pgf.File.Package.IsValid() {
-			continue
-		}
-
-		scopes = append(scopes, fileLoadScope(file.URI()))
+		uris = append(uris, file.URI())
 	}
 
-	if len(scopes) == 0 {
-		return nil
-	}
+	event.Log(ctx, "reloadOrphanedFiles reloading", tag.Files.Of(uris))
 
-	// The regtests match this exact log message, keep them in sync.
-	event.Log(ctx, "reloadOrphanedFiles reloading", tag.Query.Of(scopes))
-	err := s.load(ctx, false, scopes...)
+	var g errgroup.Group
+
+	cpulimit := runtime.GOMAXPROCS(0)
+	g.SetLimit(cpulimit)
+
+	// Load files one-at-a-time. go/packages can return at most one
+	// command-line-arguments package per query.
+	for _, file := range files {
+		file := file
+		g.Go(func() error {
+			pgf, err := s.ParseGo(ctx, file, source.ParseHeader)
+			if err != nil || !pgf.File.Package.IsValid() {
+				return nil // need a valid header
+			}
+			return s.load(ctx, false, fileLoadScope(file.URI()))
+		})
+	}
 
 	// 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
 	// content changes.
 	//
 	// TODO(rfindley): is it possible that the load stopped early for an
 	// unrelated errors? If so, add a fallback?
-	//
-	// Check for context cancellation so that we don't incorrectly mark files
-	// as unloadable, but don't return before setting all workspace packages.
-	if ctx.Err() != nil {
-		return ctx.Err()
+
+	if err := g.Wait(); err != nil {
+		// Check for context cancellation so that we don't incorrectly mark files
+		// as unloadable, but don't return before setting all workspace packages.
+		if ctx.Err() != nil {
+			return ctx.Err()
+		}
+
+		if !errors.Is(err, errNoPackages) {
+			event.Error(ctx, "reloadOrphanedFiles: failed to load", err, tag.Files.Of(uris))
+		}
 	}
 
 	// If the context was not canceled, we assume that the result of loading
@@ -1637,19 +1649,15 @@ func (s *snapshot) reloadOrphanedOpenFiles(ctx context.Context) error {
 	// prevents us from falling into recursive reloading where we only make a bit
 	// of progress each time.
 	s.mu.Lock()
-	for _, scope := range scopes {
+	defer s.mu.Unlock()
+	for _, file := range files {
 		// TODO(rfindley): instead of locking here, we should have load return the
 		// metadata graph that resulted from loading.
-		uri := span.URI(scope.(fileLoadScope))
+		uri := file.URI()
 		if s.noValidMetadataForURILocked(uri) {
 			s.unloadableFiles[uri] = struct{}{}
 		}
 	}
-	s.mu.Unlock()
-
-	if err != nil && !errors.Is(err, errNoPackages) {
-		event.Error(ctx, "reloadOrphanedFiles: failed to load", err, tag.Query.Of(scopes))
-	}
 
 	return nil
 }

gopls/internal/regtest/marker/testdata/fixedbugs/issue59318.txt

@@ -0,0 +1,22 @@
+This test verifies that we can load multiple orphaned files as
+command-line-arguments packages.
+
+Previously, we would load only one because go/packages returns at most one
+command-line-arguments package per query.
+
+-- a/main.go --
+package main
+
+func main() {
+	var a int //@diag(re"var (a)", re"not used")
+}
+-- b/main.go --
+package main
+
+func main() {
+	var b int //@diag(re"var (b)", re"not used")
+}
+-- c/go.mod --
+module c.com // The existence of this module avoids a workspace error.
+
+go 1.18