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