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 <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Auto-Submit: Robert Findley <[email protected]>
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>

Author: findleyr

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
 }

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())
 }

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
 

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 [email protected] 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 [email protected] 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))

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