Skip to content

Commit

Permalink
gopls/internal/lsp/source/rename: use incremental algorithm
Browse files Browse the repository at this point in the history
This change is a reimplementation of the renaming operation so
that it no longer mixes types.Objects from different packages.
The basic approach is as follows.

First, the referenced object is found and classified. If it is
nonexported (e.g. lowercase, or inherently local such as an
import or label, or within a function body), the operation is
local to that package and is carried out essentially as before.

However, if it is exported, then the scope of the global search
is deduced (direct for package-level var/func/const/type,
transitive for fields and methods). The object is converted to an
objectpath, and all the reverse dependencies are analyzed, using
the objectpath to identify the target in each package.

The renameObject function (the entry point for the fiddly renamer
algorithm) is now called once per package, and the results of all
calls are combined.

Because we process variants, duplicate edits are likely. We sort
and de-dup at the very end under the assumption that edits are
well behaved. The "seen edit" tracking in package renaming is no
longer needed.

Also:
- Package.importMap maps PackagePath to Package for all dependencies,
  so that we can resolve targets identified by (PackagePath,
  objectpath) to their objects in a different types.Importer "realm".
  It is materialized as a DAG of k/v pairs and exposed as
  Package.DependencyTypes.
- The rename_check algorithm (renamer) is now applied once to each
  package instead of once to all packages.
- The set of references to update is derived from types.Info, not the
  references operation.

Still to do in follow-ups:
- Method renaming needs to expand the set of renamed types (based on
  'satisfy') and recompute the dependencies of their declarations,
  until a fixed point is reached. (Not supporting this case is a
  functional regression since v0.11.0, but we plan to submit this
  change to unblock foundational progress and then fix it before the
  next release. See #58461)
- Lots of generics cases to consider (see #58462).
- Lots more tests required. For #57987.

Change-Id: I5fd8538ab35d61744d765d8bd101cd4efa41bd33
Reviewed-on: https://go-review.googlesource.com/c/tools/+/464902
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
  • Loading branch information
adonovan committed Feb 14, 2023
1 parent c8e8b3b commit 64f9d62
Show file tree
Hide file tree
Showing 11 changed files with 762 additions and 754 deletions.
11 changes: 7 additions & 4 deletions gopls/internal/lsp/cache/check.go
Expand Up @@ -447,10 +447,11 @@ func doTypeCheck(ctx context.Context, snapshot *snapshot, inputs typeCheckInputs
defer done()

pkg := &syntaxPackage{
id: inputs.id,
mode: inputs.mode,
fset: snapshot.FileSet(), // must match parse call below (snapshot.ParseGo for now)
types: types.NewPackage(string(inputs.pkgPath), string(inputs.name)),
id: inputs.id,
mode: inputs.mode,
fset: snapshot.FileSet(), // must match parse call below (snapshot.ParseGo for now)
types: types.NewPackage(string(inputs.pkgPath), string(inputs.name)),
importMap: new(importMap),
typesInfo: &types.Info{
Types: make(map[ast.Expr]types.TypeAndValue),
Defs: make(map[*ast.Ident]types.Object),
Expand All @@ -461,6 +462,7 @@ func doTypeCheck(ctx context.Context, snapshot *snapshot, inputs typeCheckInputs
},
}
typeparams.InitInstanceInfo(pkg.typesInfo)
defer func() { pkg.importMap.types = pkg.types }() // simplifies early return in "unsafe"

// Parse the non-compiled GoFiles. (These aren't presented to
// the type checker but are part of the returned pkg.)
Expand Down Expand Up @@ -533,6 +535,7 @@ func doTypeCheck(ctx context.Context, snapshot *snapshot, inputs typeCheckInputs
if err != nil {
return nil, err
}
pkg.importMap.union(depPkg.importMap)
return depPkg.types, nil
}),
}
Expand Down
47 changes: 45 additions & 2 deletions gopls/internal/lsp/cache/pkg.go
Expand Up @@ -104,6 +104,7 @@ type syntaxPackage struct {
typeErrors []types.Error
types *types.Package
typesInfo *types.Info
importMap *importMap // required for DependencyTypes (until we have shallow export data)
hasFixedFiles bool // if true, AST was sufficiently mangled that we should hide type errors
xrefs []byte // serializable index of outbound cross-references
analyses memoize.Store // maps analyzer.Name to Promise[actionResult]
Expand All @@ -112,8 +113,42 @@ type syntaxPackage struct {

func (p *Package) String() string { return string(p.m.ID) }

func (p *Package) Metadata() *source.Metadata {
return p.m
func (p *Package) Metadata() *source.Metadata { return p.m }

// An importMap is an mapping from source.PackagePath to types.Package
// of the dependencies of a syntaxPackage. Once constructed (by calls
// to union) it is never subsequently modified.
type importMap struct {
// Concretely, it is a node that contains one element of the
// mapping and whose deps are edges in DAG that comprises the
// rest of the mapping. This design optimizes union over get.
//
// TODO(adonovan): simplify when we use shallow export data.
// At that point it becomes a simple lookup in the importers
// map, which should be retained in syntaxPackage.
// (Otherwise this implementation could expose types.Packages
// that represent an old state that has since changed, but
// not in a way that is consequential to a downstream package.)

types *types.Package // map entry for types.Path => types
deps []*importMap // all other entries
}

func (m *importMap) union(dep *importMap) { m.deps = append(m.deps, dep) }

func (m *importMap) get(path source.PackagePath, seen map[*importMap]bool) *types.Package {
if !seen[m] {
seen[m] = true
if source.PackagePath(m.types.Path()) == path {
return m.types
}
for _, dep := range m.deps {
if pkg := dep.get(path, seen); pkg != nil {
return pkg
}
}
}
return nil
}

// A loadScope defines a package loading scope for use with go/packages.
Expand Down Expand Up @@ -182,6 +217,14 @@ func (p *Package) GetTypesInfo() *types.Info {
return p.pkg.typesInfo
}

// DependencyTypes returns the type checker's symbol for the specified
// package. It returns nil if path is not among the transitive
// dependencies of p, or if no symbols from that package were
// referenced during the type-checking of p.
func (p *Package) DependencyTypes(path source.PackagePath) *types.Package {
return p.pkg.importMap.get(path, make(map[*importMap]bool))
}

func (p *Package) HasParseErrors() bool {
return len(p.pkg.parseErrors) != 0
}
Expand Down
2 changes: 1 addition & 1 deletion gopls/internal/lsp/cmd/test/integration_test.go
Expand Up @@ -592,7 +592,7 @@ func oldname() {}
{
res := gopls(t, tree, "rename", "a.go:1:3", "newname")
res.checkExit(false)
res.checkStderr("no object found")
res.checkStderr("no identifier found")
}
// in 'package' identifier
{
Expand Down
10 changes: 5 additions & 5 deletions gopls/internal/lsp/lsp_test.go
Expand Up @@ -965,11 +965,9 @@ func (r *runner) Rename(t *testing.T, spn span.Span, newText string) {
}

wedit, err := r.server.Rename(r.ctx, &protocol.RenameParams{
TextDocument: protocol.TextDocumentIdentifier{
URI: protocol.URIFromSpanURI(uri),
},
Position: loc.Range.Start,
NewName: newText,
TextDocument: protocol.TextDocumentIdentifier{URI: loc.URI},
Position: loc.Range.Start,
NewName: newText,
})
if err != nil {
renamed := string(r.data.Golden(t, tag, filename, func() ([]byte, error) {
Expand All @@ -990,6 +988,8 @@ func (r *runner) Rename(t *testing.T, spn span.Span, newText string) {
}
sort.Strings(orderedURIs)

// Print the name and content of each modified file,
// concatenated, and compare against the golden.
var got string
for i := 0; i < len(res); i++ {
if i != 0 {
Expand Down
5 changes: 2 additions & 3 deletions gopls/internal/lsp/rename.go
Expand Up @@ -10,7 +10,6 @@ import (

"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/source"
"golang.org/x/tools/gopls/internal/span"
)

func (s *Server) rename(ctx context.Context, params *protocol.RenameParams) (*protocol.WorkspaceEdit, error) {
Expand All @@ -36,8 +35,8 @@ func (s *Server) rename(ctx context.Context, params *protocol.RenameParams) (*pr
docChanges = append(docChanges, documentChanges(fh, e)...)
}
if isPkgRenaming {
uri := params.TextDocument.URI.SpanURI()
oldBase := filepath.Dir(span.URI.Filename(uri))
// Update the last component of the file's enclosing directory.
oldBase := filepath.Dir(fh.URI().Filename())
newURI := filepath.Join(filepath.Dir(oldBase), params.NewName)
docChanges = append(docChanges, protocol.DocumentChanges{
RenameFile: &protocol.RenameFile{
Expand Down
2 changes: 2 additions & 0 deletions gopls/internal/lsp/source/references2.go
Expand Up @@ -523,6 +523,8 @@ func effectiveReceiver(obj types.Object) types.Type {
// Each object is mapped to the syntax node that was treated as an
// identifier, which is not always an ast.Ident. The second component
// of the result is the innermost node enclosing pos.
//
// TODO(adonovan): factor in common with referencedObject.
func objectsAt(info *types.Info, file *ast.File, pos token.Pos) (map[types.Object]ast.Node, ast.Node, error) {
path := pathEnclosingObjNode(file, pos)
if path == nil {
Expand Down

0 comments on commit 64f9d62

Please sign in to comment.