From b6235391adb3b7f8bcfc4df81055e8f023de2688 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Fri, 24 May 2024 16:52:42 -0400 Subject: [PATCH] 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 LUCI-TryBot-Result: Go LUCI --- gopls/internal/cache/analysis.go | 6 ++- gopls/internal/cache/check.go | 2 +- gopls/internal/cache/load.go | 6 +-- gopls/internal/cache/metadata/metadata.go | 16 +++++--- gopls/internal/golang/known_packages.go | 2 +- .../diagnostics/gopackagesdriver_test.go | 37 +++++++++++++++++++ .../testdata/diagnostics/useinternal.txt | 3 ++ 7 files changed, 60 insertions(+), 12 deletions(-) diff --git a/gopls/internal/cache/analysis.go b/gopls/internal/cache/analysis.go index fb652be1452..4730830cb4f 100644 --- a/gopls/internal/cache/analysis.go +++ b/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) } diff --git a/gopls/internal/cache/check.go b/gopls/internal/cache/check.go index 33403060adb..bd2d6c2636e 100644 --- a/gopls/internal/cache/check.go +++ b/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) diff --git a/gopls/internal/cache/load.go b/gopls/internal/cache/load.go index e42290d5458..3bf79cb1615 100644 --- a/gopls/internal/cache/load.go +++ b/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). diff --git a/gopls/internal/cache/metadata/metadata.go b/gopls/internal/cache/metadata/metadata.go index 826edd15cdb..7860f336954 100644 --- a/gopls/internal/cache/metadata/metadata.go +++ b/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])) } diff --git a/gopls/internal/golang/known_packages.go b/gopls/internal/golang/known_packages.go index 60a89ca0285..3b320d4f782 100644 --- a/gopls/internal/golang/known_packages.go +++ b/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 diff --git a/gopls/internal/test/integration/diagnostics/gopackagesdriver_test.go b/gopls/internal/test/integration/diagnostics/gopackagesdriver_test.go index 7ed6d2a7737..65700b69795 100644 --- a/gopls/internal/test/integration/diagnostics/gopackagesdriver_test.go +++ b/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"`))) + }) +} diff --git a/gopls/internal/test/marker/testdata/diagnostics/useinternal.txt b/gopls/internal/test/marker/testdata/diagnostics/useinternal.txt index 11a3cc9a0c0..86010dc29c8 100644 --- a/gopls/internal/test/marker/testdata/diagnostics/useinternal.txt +++ b/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