gopls/internal/lsp/source: ignore objectpath error in rename

An error from objectpath.Object previously caused an
assertion failure (bug.Reportf); this change deletes
the assertion because the error is benign.

It is caused by a discrepancy between the set of cases
supported by objectpath.For/Object in syntax packages
and export data packages: objects in the former may
have a path that is not valid in the latter.

Added a regression test.

The surprising behavior is documented at objectpath,
but it seems unsatisfying, and perhaps should be treated
as a bug. Suggestions welcome.

Fixes golang/go#60789

Change-Id: I3657f6a465c397adc05ea70331a59c8c9963daa7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/507495
Run-TryBot: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

go/types/objectpath/objectpath.go

@@ -137,6 +137,17 @@ type Encoder struct {
 // These objects are sufficient to define the API of their package.
 // The objects described by a package's export data are drawn from this set.
 //
+// The set of objects accessible from a package's Scope depends on
+// whether the package was produced by type-checking syntax, or
+// reading export data; the latter may have a smaller Scope since
+// export data trims objects that are not reachable from an exported
+// declaration. For example, the For function will return a path for
+// an exported method of an unexported type that is not reachable
+// from any public declaration; this path will cause the Object
+// function to fail if called on a package loaded from export data.
+// TODO(adonovan): is this a bug or feature? Should this package
+// compute accessibility in the same way?
+//
 // For does not return a path for predeclared names, imported package
 // names, local names, and unexported package-level names (except
 // types).

go/types/objectpath/objectpath_test.go

@@ -47,6 +47,8 @@ type M map[struct{x int}]struct{y int}
 func unexportedFunc()
 type unexportedType struct{}
 
+func (unexportedType) F() {} // not reachable from package's public API (export data)
+
 type S struct{t struct{x int}}
 type R []struct{y int}
 type Q [2]struct{z int}
@@ -84,6 +86,7 @@ type T struct{x, y int}
 		{"b", "T.M0.RA0", "var  *interface{f()}", ""},       // parameter
 		{"b", "T.M0.RA0.EM0", "func (interface).f()", ""},   // interface method
 		{"b", "unexportedType", "type b.unexportedType struct{}", ""},
+		{"b", "unexportedType.M0", "func (b.unexportedType).F()", ""},
 		{"b", "S.UF0.F0", "field x int", ""},
 		{"b", "R.UEF0", "field y int", ""},
 		{"b", "Q.UEF0", "field z int", ""},
@@ -234,6 +237,14 @@ type Foo interface {
 
 var X chan struct{ Z int }
 var Z map[string]struct{ A int }
+
+var V unexported
+type unexported struct{}
+func (unexported) F() {} // reachable via V
+
+// The name 'unreachable' has special meaning to the test.
+type unreachable struct{}
+func (unreachable) F() {} // not reachable in export data
 `
 
 	// Parse source file and type-check it as a package, "src".
@@ -277,11 +288,20 @@ var Z map[string]struct{ A int }
 			t.Errorf("For(%v): %v", srcobj, err)
 			continue
 		}
+
+		// Do we expect to find this object in the export data?
+		reachable := !strings.Contains(string(path), "unreachable")
+
 		binobj, err := objectpath.Object(binpkg, path)
 		if err != nil {
-			t.Errorf("Object(%s, %q): %v", binpkg.Path(), path, err)
+			if reachable {
+				t.Errorf("Object(%s, %q): %v", binpkg.Path(), path, err)
+			}
 			continue
 		}
+		if !reachable {
+			t.Errorf("Object(%s, %q) = %v (unexpectedly reachable)", binpkg.Path(), path, binobj)
+		}
 
 		// Check the object strings match.
 		// (We can't check that types are identical because the

gopls/internal/lsp/source/rename.go

@@ -569,12 +569,15 @@ func renameExported(ctx context.Context, snapshot Snapshot, pkgs []Package, decl
 			}
 			obj, err := objectpath.Object(p, t.obj)
 			if err != nil {
-				// Though this can happen with regular export data
-				// due to trimming of inconsequential objects,
-				// it can't happen if we load dependencies from full
-				// syntax (as today) or shallow export data (soon),
-				// as both are complete.
-				bug.Reportf("objectpath.Object(%v, %v) failed: %v", p, t.obj, err)
+				// Possibly a method or an unexported type
+				// that is not reachable through export data?
+				// See https://github.com/golang/go/issues/60789.
+				//
+				// TODO(adonovan): it seems unsatisfactory that Object
+				// should return an error for a "valid" path. Perhaps
+				// we should define such paths as invalid and make
+				// objectpath.For compute reachability?
+				// Would that be a compatible change?
 				continue
 			}
 			objects = append(objects, obj)

gopls/internal/regtest/marker/testdata/rename/issue60789.txt

@@ -0,0 +1,36 @@
+
+This test renames an exported method of an unexported type,
+which is an edge case for objectpath, since it computes a path
+from a syntax package that is no good when applied to an
+export data package.
+
+See issue #60789.
+
+-- go.mod --
+module example.com
+go 1.12
+
+-- a/a.go --
+package a
+
+type unexported int 
+func (unexported) F() {} //@rename("F", G, fToG)
+
+var _ = unexported(0).F
+
+-- b/b.go --
+package b
+
+// The existence of this package is sufficient to exercise
+// the bug even though it cannot reference a.unexported.
+
+import _ "example.com/a"
+
+-- @fToG/a/a.go --
+package a
+
+type unexported int 
+func (unexported) G() {} //@rename("F", G, fToG)
+
+var _ = unexported(0).G
+