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 <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index 6852c74..50ce895 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -1457,8 +1457,10 @@
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 @@
}
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 @@
// 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 @@
}
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 41c8e46..557c222 100644
--- a/gopls/internal/regtest/marker/marker_test.go
+++ b/gopls/internal/regtest/marker/marker_test.go
@@ -8,11 +8,13 @@
"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 9116d77..cacf6fd 100644
--- a/gopls/internal/regtest/marker/testdata/references/issue60676.txt
+++ b/gopls/internal/regtest/marker/testdata/references/issue60676.txt
@@ -5,6 +5,9 @@
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 @@
N() //@loc(NDef, "N")
}
+type T[P any] struct{ f P }
+
type Error error
+
-- b/b.go --
package b
@@ -43,6 +49,8 @@
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 da2fa7a..6103dd7 100644
--- a/internal/gcimporter/iexport.go
+++ b/internal/gcimporter/iexport.go
@@ -46,7 +46,7 @@
// 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 @@
// 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 @@
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 @@
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 @@
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 fd4c200..7f77a79 100644
--- a/internal/gcimporter/iexport_test.go
+++ b/internal/gcimporter/iexport_test.go
@@ -61,7 +61,7 @@
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
