From 6782af031e202cef57450347adde23bdff33bccc Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Mon, 19 Sep 2022 15:18:43 -0400 Subject: [PATCH] gopls/internal/lsp/source: clarify qualifiedObject Clarify documentation around qualifiedObject and its field invariants. (Initially I tried to delete it but it's not so easy.) Also, hoist KnownPackages et al outside of loop over qualifiedIdentifiers in implementations() to avoid unnecessary recomputation. Change-Id: Idf087634b918a2277eabf8bbab2fdf49a8fc946c Reviewed-on: https://go-review.googlesource.com/c/tools/+/431839 Reviewed-by: Robert Findley Auto-Submit: Alan Donovan TryBot-Result: Gopher Robot Run-TryBot: Alan Donovan --- gopls/internal/lsp/source/implementation.go | 82 ++++++++++----------- gopls/internal/lsp/source/references.go | 16 +++- gopls/internal/lsp/source/rename.go | 4 +- 3 files changed, 55 insertions(+), 47 deletions(-) diff --git a/gopls/internal/lsp/source/implementation.go b/gopls/internal/lsp/source/implementation.go index f612e35d25..b3896802aa 100644 --- a/gopls/internal/lsp/source/implementation.go +++ b/gopls/internal/lsp/source/implementation.go @@ -13,9 +13,9 @@ import ( "go/types" "sort" - "golang.org/x/tools/internal/event" "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/lsp/safetoken" + "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/span" ) @@ -59,23 +59,48 @@ var ErrNotAType = errors.New("not a type name or method") // implementations returns the concrete implementations of the specified // interface, or the interfaces implemented by the specified concrete type. +// It populates only the definition-related fields of qualifiedObject. +// (Arguably it should return a smaller data type.) func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position) ([]qualifiedObject, error) { + // Find all named types, even local types + // (which can have methods due to promotion). var ( - impls []qualifiedObject - seen = make(map[token.Position]bool) - fset = s.FileSet() + allNamed []*types.Named + pkgs = make(map[*types.Package]Package) ) + knownPkgs, err := s.KnownPackages(ctx) + if err != nil { + return nil, err + } + for _, pkg := range knownPkgs { + pkgs[pkg.GetTypes()] = pkg + for _, obj := range pkg.GetTypesInfo().Defs { + obj, ok := obj.(*types.TypeName) + // We ignore aliases 'type M = N' to avoid duplicate reporting + // of the Named type N. + if !ok || obj.IsAlias() { + continue + } + if named, ok := obj.Type().(*types.Named); ok { + allNamed = append(allNamed, named) + } + } + } qos, err := qualifiedObjsAtProtocolPos(ctx, s, f.URI(), pp) if err != nil { return nil, err } + var ( + impls []qualifiedObject + seen = make(map[token.Position]bool) + ) for _, qo := range qos { + // Ascertain the query identifier (type or method). var ( queryType types.Type queryMethod *types.Func ) - switch obj := qo.obj.(type) { case *types.Func: queryMethod = obj @@ -94,32 +119,6 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol. return nil, nil } - // Find all named types, even local types (which can have methods - // due to promotion). - var ( - allNamed []*types.Named - pkgs = make(map[*types.Package]Package) - ) - knownPkgs, err := s.KnownPackages(ctx) - if err != nil { - return nil, err - } - for _, pkg := range knownPkgs { - pkgs[pkg.GetTypes()] = pkg - info := pkg.GetTypesInfo() - for _, obj := range info.Defs { - obj, ok := obj.(*types.TypeName) - // We ignore aliases 'type M = N' to avoid duplicate reporting - // of the Named type N. - if !ok || obj.IsAlias() { - continue - } - if named, ok := obj.Type().(*types.Named); ok { - allNamed = append(allNamed, named) - } - } - } - // Find all the named types that match our query. for _, named := range allNamed { var ( @@ -146,7 +145,7 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol. candObj = sel.Obj() } - pos := fset.Position(candObj.Pos()) + pos := s.FileSet().Position(candObj.Pos()) if candObj == queryMethod || seen[pos] { continue } @@ -155,7 +154,7 @@ func implementations(ctx context.Context, s Snapshot, f FileHandle, pp protocol. impls = append(impls, qualifiedObject{ obj: candObj, - pkg: pkgs[candObj.Pkg()], + pkg: pkgs[candObj.Pkg()], // may be nil (e.g. error) }) } } @@ -192,17 +191,16 @@ func ensurePointer(T types.Type) types.Type { return T } +// A qualifiedObject is the result of resolving a reference from an +// identifier to an object. type qualifiedObject struct { - obj types.Object + // definition + obj types.Object // the referenced object + pkg Package // the Package that defines the object (nil => universe) - // pkg is the Package that contains obj's definition. - pkg Package - - // node is the *ast.Ident or *ast.ImportSpec we followed to find obj, if any. - node ast.Node - - // sourcePkg is the Package that contains node, if any. - sourcePkg Package + // reference (optional) + node ast.Node // the reference (*ast.Ident or *ast.ImportSpec) to the object + sourcePkg Package // the Package containing node } var ( diff --git a/gopls/internal/lsp/source/references.go b/gopls/internal/lsp/source/references.go index eacadab854..d1cf1eaa8f 100644 --- a/gopls/internal/lsp/source/references.go +++ b/gopls/internal/lsp/source/references.go @@ -15,9 +15,9 @@ import ( "sort" "strconv" - "golang.org/x/tools/internal/event" - "golang.org/x/tools/internal/bug" "golang.org/x/tools/gopls/internal/lsp/protocol" + "golang.org/x/tools/internal/bug" + "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/span" ) @@ -126,6 +126,10 @@ func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Posit } // references is a helper function to avoid recomputing qualifiedObjsAtProtocolPos. +// The first element of qos is considered to be the declaration; +// if isDeclaration, the first result is an extra item for it. +// Only the definition-related fields of qualifiedObject are used. +// (Arguably it should accept a smaller data type.) func references(ctx context.Context, snapshot Snapshot, qos []qualifiedObject, includeDeclaration, includeInterfaceRefs, includeEmbeddedRefs bool) ([]*ReferenceInfo, error) { var ( references []*ReferenceInfo @@ -134,8 +138,10 @@ func references(ctx context.Context, snapshot Snapshot, qos []qualifiedObject, i pos := qos[0].obj.Pos() if pos == token.NoPos { - return nil, fmt.Errorf("no position for %s", qos[0].obj) + return nil, fmt.Errorf("no position for %s", qos[0].obj) // e.g. error.Error } + // Inv: qos[0].pkg != nil, since Pos is valid. + // Inv: qos[*].pkg != nil, since all qos are logically the same declaration. filename := snapshot.FileSet().Position(pos).Filename pgf, err := qos[0].pkg.File(span.URIFromPath(filename)) if err != nil { @@ -220,6 +226,8 @@ func references(ctx context.Context, snapshot Snapshot, qos []qualifiedObject, i // happened to have a String method. _, isType := declIdent.Declaration.obj.(*types.TypeName) if includeInterfaceRefs && !isType { + // TODO(adonovan): opt: don't go back into the position domain: + // we have complete type information already. declRange, err := declIdent.Range() if err != nil { return nil, err @@ -256,6 +264,8 @@ func interfaceReferences(ctx context.Context, s Snapshot, f FileHandle, pp proto return nil, err } + // Make a separate call to references() for each element + // since it treats the first qualifiedObject as a definition. var refs []*ReferenceInfo for _, impl := range implementations { implRefs, err := references(ctx, s, []qualifiedObject{impl}, false, false, false) diff --git a/gopls/internal/lsp/source/rename.go b/gopls/internal/lsp/source/rename.go index e31dfd5e09..bb4d9a843e 100644 --- a/gopls/internal/lsp/source/rename.go +++ b/gopls/internal/lsp/source/rename.go @@ -17,9 +17,9 @@ import ( "strings" "golang.org/x/tools/go/types/typeutil" - "golang.org/x/tools/internal/event" - "golang.org/x/tools/internal/diff" "golang.org/x/tools/gopls/internal/lsp/protocol" + "golang.org/x/tools/internal/diff" + "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/span" "golang.org/x/tools/refactor/satisfy" )