gopls/internal/cache: suppress "internal" import check on Bazel

The go command treats imports of packages whose path contains
"/internal/" specially, and gopls must simulate it in several
places. However, other build systems such as Bazel have their
own mechanisms for representing visibility.

This CL suppresses the check for packages obtained from a
build system other than go list. (We derive this information
from the view type, which in turn simulates the go/packages
driver protocol switch using $GOPACKAGESDRIVER, etc.)

Added test using Rob's new pass-through gopackagesdriver.

Fixes golang/go#66856

Change-Id: I6e0671caeabe2146d397eb56d5cd4f7a40384370
Reviewed-on: https://go-review.googlesource.com/c/tools/+/587931
Reviewed-by: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: adonovan

gopls/internal/cache/analysis.go

@@ -257,6 +257,7 @@ func (s *Snapshot) Analyze(ctx context.Context, pkgs map[PackageID]*metadata.Pac
 			an = &analysisNode{
 				fset:        fset,
 				fsource:     struct{ file.Source }{s}, // expose only ReadFile
+				viewType:    s.View().Type(),
 				mp:          mp,
 				analyzers:   facty, // all nodes run at least the facty analyzers
 				allDeps:     make(map[PackagePath]*analysisNode),
@@ -522,6 +523,7 @@ func (an *analysisNode) decrefPreds() {
 type analysisNode struct {
 	fset            *token.FileSet              // file set shared by entire batch (DAG)
 	fsource         file.Source                 // Snapshot.ReadFile, for use by Pass.ReadFile
+	viewType        ViewType                    // type of view
 	mp              *metadata.Package           // metadata for this package
 	files           []file.Handle               // contents of CompiledGoFiles
 	analyzers       []*analysis.Analyzer        // set of analyzers to run
@@ -742,6 +744,8 @@ func (an *analysisNode) cacheKey() [sha256.Size]byte {
 	// package metadata
 	mp := an.mp
 	fmt.Fprintf(hasher, "package: %s %s %s\n", mp.ID, mp.Name, mp.PkgPath)
+	fmt.Fprintf(hasher, "viewtype: %s\n", an.viewType) // (affects diagnostics)
+
 	// We can ignore m.DepsBy{Pkg,Import}Path: although the logic
 	// uses those fields, we account for them by hashing vdeps.
 
@@ -1023,7 +1027,7 @@ func (an *analysisNode) typeCheck(parsed []*parsego.File) *analysisPackage {
 			}
 
 			// (Duplicates logic from check.go.)
-			if !metadata.IsValidImport(an.mp.PkgPath, dep.mp.PkgPath) {
+			if !metadata.IsValidImport(an.mp.PkgPath, dep.mp.PkgPath, an.viewType != GoPackagesDriverView) {
 				return nil, fmt.Errorf("invalid use of internal package %s", importPath)
 			}
 

gopls/internal/cache/check.go

@@ -1632,7 +1632,7 @@ func (b *typeCheckBatch) typesConfig(ctx context.Context, inputs typeCheckInputs
 				// e.g. missing metadata for dependencies in buildPackageHandle
 				return nil, missingPkgError(inputs.id, path, inputs.viewType)
 			}
-			if !metadata.IsValidImport(inputs.pkgPath, depPH.mp.PkgPath) {
+			if !metadata.IsValidImport(inputs.pkgPath, depPH.mp.PkgPath, inputs.viewType != GoPackagesDriverView) {
 				return nil, fmt.Errorf("invalid use of internal package %q", path)
 			}
 			return b.getImportPackage(ctx, id)

gopls/internal/cache/load.go

@@ -262,7 +262,7 @@ func (s *Snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc
 		if allFilesExcluded(pkg.GoFiles, filterFunc) {
 			continue
 		}
-		buildMetadata(newMetadata, pkg, cfg.Dir, standalone)
+		buildMetadata(newMetadata, pkg, cfg.Dir, standalone, s.view.typ != GoPackagesDriverView)
 	}
 
 	s.mu.Lock()
@@ -354,7 +354,7 @@ func (m *moduleErrorMap) Error() string {
 // Returns the metadata.Package that was built (or which was already present in
 // updates), or nil if the package could not be built. Notably, the resulting
 // metadata.Package may have an ID that differs from pkg.ID.
-func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Package, loadDir string, standalone bool) *metadata.Package {
+func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Package, loadDir string, standalone, goListView bool) *metadata.Package {
 	// Allow for multiple ad-hoc packages in the workspace (see #47584).
 	pkgPath := PackagePath(pkg.PkgPath)
 	id := PackageID(pkg.ID)
@@ -520,7 +520,7 @@ func buildMetadata(updates map[PackageID]*metadata.Package, pkg *packages.Packag
 			continue
 		}
 
-		dep := buildMetadata(updates, imported, loadDir, false) // only top level packages can be standalone
+		dep := buildMetadata(updates, imported, loadDir, false, goListView) // only top level packages can be standalone
 
 		// Don't record edges to packages with no name, as they cause trouble for
 		// the importer (golang/go#60952).

gopls/internal/cache/metadata/metadata.go

@@ -236,19 +236,23 @@ func RemoveIntermediateTestVariants(pmetas *[]*Package) {
 	*pmetas = res
 }
 
-// IsValidImport returns whether importPkgPath is importable
-// by pkgPath.
-func IsValidImport(pkgPath, importPkgPath PackagePath) bool {
-	i := strings.LastIndex(string(importPkgPath), "/internal/")
+// IsValidImport returns whether from may import to.
+func IsValidImport(from, to PackagePath, goList bool) bool {
+	// If the metadata came from a build system other than go list
+	// (e.g. bazel) it is beyond our means to compute visibility.
+	if !goList {
+		return true
+	}
+	i := strings.LastIndex(string(to), "/internal/")
 	if i == -1 {
 		return true
 	}
 	// TODO(rfindley): this looks wrong: IsCommandLineArguments is meant to
 	// operate on package IDs, not package paths.
-	if IsCommandLineArguments(PackageID(pkgPath)) {
+	if IsCommandLineArguments(PackageID(from)) {
 		return true
 	}
 	// TODO(rfindley): this is wrong. mod.testx/p should not be able to
 	// import mod.test/internal: https://go.dev/play/p/-Ca6P-E4V4q
-	return strings.HasPrefix(string(pkgPath), string(importPkgPath[:i]))
+	return strings.HasPrefix(string(from), string(to[:i]))
 }

gopls/internal/golang/known_packages.go

@@ -76,7 +76,7 @@ func KnownPackagePaths(ctx context.Context, snapshot *cache.Snapshot, fh file.Ha
 			continue
 		}
 		// make sure internal packages are importable by the file
-		if !metadata.IsValidImport(current.PkgPath, knownPkg.PkgPath) {
+		if !metadata.IsValidImport(current.PkgPath, knownPkg.PkgPath, snapshot.View().Type() != cache.GoPackagesDriverView) {
 			continue
 		}
 		// naive check on cyclical imports

gopls/internal/test/integration/diagnostics/gopackagesdriver_test.go

@@ -46,3 +46,40 @@ func f() {
 		)
 	})
 }
+
+func TestValidImportCheck_GoPackagesDriver(t *testing.T) {
+	const files = `
+-- go.work --
+use .
+
+-- go.mod --
+module example.com
+go 1.0
+
+-- a/a.go --
+package a
+import _ "example.com/b/internal/c"
+
+-- b/internal/c/c.go --
+package c
+`
+
+	// Note that 'go list' produces an error ("use of internal package %q not allowed")
+	// and gopls produces another ("invalid use of internal package %q") with source=compiler.
+	// Here we assert that the second one is not reported with a go/packages driver.
+	// (We don't assert that the first is missing, because the test driver wraps go list!)
+
+	// go list
+	Run(t, files, func(t *testing.T, env *Env) {
+		env.OpenFile("a/a.go")
+		env.AfterChange(Diagnostics(WithMessage(`invalid use of internal package "example.com/b/internal/c"`)))
+	})
+
+	// test driver
+	WithOptions(
+		FakeGoPackagesDriver(t),
+	).Run(t, files, func(t *testing.T, env *Env) {
+		env.OpenFile("a/a.go")
+		env.AfterChange(NoDiagnostics(WithMessage(`invalid use of internal package "example.com/b/internal/c"`)))
+	})
+}

gopls/internal/test/marker/testdata/diagnostics/useinternal.txt

@@ -2,6 +2,9 @@ This test checks a diagnostic for invalid use of internal packages.
 
 This list error changed in Go 1.21.
 
+See TestValidImportCheck_GoPackagesDriver for a test that no diagnostic
+is produced when using a GOPACKAGESDRIVER (such as for Bazel).
+
 -- flags --
 -min_go=go1.21