From e0783a89e9a456f9b64aa66717db398a20e7921d Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 31 Jul 2023 13:05:41 -0400 Subject: [PATCH] internal/gcimporter: remove bug report on objectpath failure As reported in golang/go#61674, there are certain cases involving instantiated fields that simply can't be encoded using objectpaths. Therefore, the workaround is fundamentally insufficient, and should probably be removed or made irrelevant (golang/go#61674). In the meantime, update commentary and remove the bug report. Update the gopls regression test for this behavior to exercise the objectpath failure. While at it, ensure that the marker regtests panic on bugs, like all other regtests. This ran into an existing imprecise bug report, which is amended. Updates golang/go#61674 Change-Id: I0eaea00d46cec88ac4c20cebdde805a7db3a33ad Reviewed-on: https://go-review.googlesource.com/c/tools/+/514575 gopls-CI: kokoro Run-TryBot: Robert Findley TryBot-Result: Gopher Robot Auto-Submit: Robert Findley Reviewed-by: Hyang-Ah Hana Kim --- gopls/internal/lsp/cache/check.go | 10 +++-- gopls/internal/regtest/marker/marker_test.go | 2 + .../marker/testdata/references/issue60676.txt | 8 ++++ internal/gcimporter/iexport.go | 37 ++++++++++--------- internal/gcimporter/iexport_test.go | 2 +- 5 files changed, 36 insertions(+), 23 deletions(-) diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go index 6852c741f08..50ce8955014 100644 --- a/gopls/internal/lsp/cache/check.go +++ b/gopls/internal/lsp/cache/check.go @@ -1457,8 +1457,10 @@ func typeCheckImpl(ctx context.Context, b *typeCheckBatch, inputs typeCheckInput diags, err := typeErrorDiagnostics(inputs.moduleMode, inputs.linkTarget, pkg, e) if err != nil { // If we fail here and there are no parse errors, it means we are hiding - // a valid type-checking error from the user. This must be a bug. - if len(pkg.parseErrors) == 0 { + // a valid type-checking error from the user. This must be a bug, with + // one exception: relocated primary errors may fail processing, because + // they reference locations outside of the package. + if len(pkg.parseErrors) == 0 && !e.relocated { bug.Reportf("failed to compute position for type error %v: %v", e, err) } continue @@ -1788,6 +1790,7 @@ func missingPkgError(from PackageID, pkgPath string, moduleMode bool) error { } type extendedError struct { + relocated bool // if set, this is a relocation of a primary error to a secondary location primary types.Error secondaries []types.Error } @@ -1840,7 +1843,7 @@ func expandErrors(errs []types.Error, supportsRelatedInformation bool) []extende // Copy over the secondary errors, noting the location of the // current error we're cloning. - clonedError := extendedError{primary: relocatedSecondary, secondaries: []types.Error{original.primary}} + clonedError := extendedError{relocated: true, primary: relocatedSecondary, secondaries: []types.Error{original.primary}} for j, secondary := range original.secondaries { if i == j { secondary.Msg += " (this error)" @@ -1849,7 +1852,6 @@ func expandErrors(errs []types.Error, supportsRelatedInformation bool) []extende } result = append(result, clonedError) } - } return result } diff --git a/gopls/internal/regtest/marker/marker_test.go b/gopls/internal/regtest/marker/marker_test.go index 41c8e4697cb..557c2228d79 100644 --- a/gopls/internal/regtest/marker/marker_test.go +++ b/gopls/internal/regtest/marker/marker_test.go @@ -8,11 +8,13 @@ import ( "os" "testing" + "golang.org/x/tools/gopls/internal/bug" . "golang.org/x/tools/gopls/internal/lsp/regtest" "golang.org/x/tools/internal/testenv" ) func TestMain(m *testing.M) { + bug.PanicOnBugs = true testenv.ExitIfSmallMachine() os.Exit(m.Run()) } diff --git a/gopls/internal/regtest/marker/testdata/references/issue60676.txt b/gopls/internal/regtest/marker/testdata/references/issue60676.txt index 9116d77100a..cacf6fd4cff 100644 --- a/gopls/internal/regtest/marker/testdata/references/issue60676.txt +++ b/gopls/internal/regtest/marker/testdata/references/issue60676.txt @@ -5,6 +5,9 @@ shared by types from multiple packages. See golang/go#60676. Note that the marker test runner awaits the initial workspace load, so export data should be populated at the time references are requested. +-- flags -- +-min_go=go1.18 + -- go.mod -- module mod.test @@ -32,8 +35,11 @@ type EI interface { N() //@loc(NDef, "N") } +type T[P any] struct{ f P } + type Error error + -- b/b.go -- package b @@ -43,6 +49,8 @@ type B a.A type BI a.AI +type T a.T[int] // must not panic + -- c/c.go -- package c diff --git a/internal/gcimporter/iexport.go b/internal/gcimporter/iexport.go index da2fa7a1bb9..6103dd7102b 100644 --- a/internal/gcimporter/iexport.go +++ b/internal/gcimporter/iexport.go @@ -46,7 +46,7 @@ func IExportShallow(fset *token.FileSet, pkg *types.Package, reportf ReportFunc) // TODO(adonovan): use byte slices throughout, avoiding copying. const bundle, shallow = false, true var out bytes.Buffer - err := iexportCommon(&out, fset, bundle, shallow, iexportVersion, []*types.Package{pkg}, reportf) + err := iexportCommon(&out, fset, bundle, shallow, iexportVersion, []*types.Package{pkg}) return out.Bytes(), err } @@ -86,16 +86,16 @@ const bundleVersion = 0 // so that calls to IImportData can override with a provided package path. func IExportData(out io.Writer, fset *token.FileSet, pkg *types.Package) error { const bundle, shallow = false, false - return iexportCommon(out, fset, bundle, shallow, iexportVersion, []*types.Package{pkg}, nil) + return iexportCommon(out, fset, bundle, shallow, iexportVersion, []*types.Package{pkg}) } // IExportBundle writes an indexed export bundle for pkgs to out. func IExportBundle(out io.Writer, fset *token.FileSet, pkgs []*types.Package) error { const bundle, shallow = true, false - return iexportCommon(out, fset, bundle, shallow, iexportVersion, pkgs, nil) + return iexportCommon(out, fset, bundle, shallow, iexportVersion, pkgs) } -func iexportCommon(out io.Writer, fset *token.FileSet, bundle, shallow bool, version int, pkgs []*types.Package, reportf ReportFunc) (err error) { +func iexportCommon(out io.Writer, fset *token.FileSet, bundle, shallow bool, version int, pkgs []*types.Package) (err error) { if !debug { defer func() { if e := recover(); e != nil { @@ -113,7 +113,6 @@ func iexportCommon(out io.Writer, fset *token.FileSet, bundle, shallow bool, ver fset: fset, version: version, shallow: shallow, - reportf: reportf, allPkgs: map[*types.Package]bool{}, stringIndex: map[string]uint64{}, declIndex: map[types.Object]uint64{}, @@ -330,7 +329,6 @@ type iexporter struct { shallow bool // don't put types from other packages in the index objEncoder *objectpath.Encoder // encodes objects from other packages in shallow mode; lazily allocated - reportf ReportFunc // if non-nil, used to report bugs localpkg *types.Package // (nil in bundle mode) // allPkgs tracks all packages that have been referenced by @@ -917,22 +915,25 @@ func (w *exportWriter) objectPath(obj types.Object) { objectPath, err := w.p.objectpathEncoder().For(obj) if err != nil { // Fall back to the empty string, which will cause the importer to create a - // new object. + // new object, which matches earlier behavior. Creating a new object is + // sufficient for many purposes (such as type checking), but causes certain + // references algorithms to fail (golang/go#60819). However, we didn't + // notice this problem during months of gopls@v0.12.0 testing. // - // This is incorrect in shallow mode (golang/go#60819), but matches - // the previous behavior. This code is defensive, as it is hard to - // prove that the objectpath algorithm will succeed in all cases, and - // creating a new object sort of works. - // (we didn't notice the bug during months of gopls@v0.12.0 testing) + // TODO(golang/go#61674): this workaround is insufficient, as in the case + // where the field forwarded from an instantiated type that may not appear + // in the export data of the original package: // - // However, report a bug so that we can eventually have confidence - // that export/import is producing a correct package. + // // package a + // type A[P any] struct{ F P } // - // TODO: remove reportf once we have such confidence. + // // package b + // type B a.A[int] + // + // We need to update references algorithms not to depend on this + // de-duplication, at which point we may want to simply remove the + // workaround here. w.string("") - if w.p.reportf != nil { - w.p.reportf("unable to encode object %q in package %q: %v", obj.Name(), obj.Pkg().Path(), err) - } return } w.string(string(objectPath)) diff --git a/internal/gcimporter/iexport_test.go b/internal/gcimporter/iexport_test.go index fd4c200f1db..7f77a796077 100644 --- a/internal/gcimporter/iexport_test.go +++ b/internal/gcimporter/iexport_test.go @@ -61,7 +61,7 @@ func readExportFile(filename string) ([]byte, error) { func iexport(fset *token.FileSet, version int, pkg *types.Package) ([]byte, error) { var buf bytes.Buffer const bundle, shallow = false, false - if err := gcimporter.IExportCommon(&buf, fset, bundle, shallow, version, []*types.Package{pkg}, nil); err != nil { + if err := gcimporter.IExportCommon(&buf, fset, bundle, shallow, version, []*types.Package{pkg}); err != nil { return nil, err } return buf.Bytes(), nil