From 2b779830f9d33eccacea7d9e741475351b225b1a Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Wed, 23 Oct 2019 16:10:24 -0400 Subject: [PATCH] internal/lsp: don't associate package with snapshot This change effectively reverts CL 202039. This CL was a mistake, as it creates a cycle. Snapshots hold CheckPackageHandles, which in turn hold pkgs. Change-Id: I944304cb365f0ef98b5e54ea38edea6cece40453 Reviewed-on: https://go-review.googlesource.com/c/tools/+/202740 Run-TryBot: Rebecca Stambler Reviewed-by: Heschi Kreinick --- internal/lsp/cache/check.go | 2 +- internal/lsp/cache/errors.go | 16 ++++---- internal/lsp/cache/pkg.go | 10 ++--- internal/lsp/source/completion_format.go | 2 +- internal/lsp/source/hover.go | 2 +- internal/lsp/source/identifier.go | 47 ++++++++++++------------ internal/lsp/source/rename.go | 3 +- internal/lsp/source/util.go | 4 +- internal/lsp/source/view.go | 3 +- 9 files changed, 45 insertions(+), 44 deletions(-) diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 211734c30e..79603abc01 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -270,7 +270,7 @@ func (imp *importer) typeCheck(ctx context.Context, cph *checkPackageHandle) (*p } pkg := &pkg{ - snapshot: imp.snapshot, + view: imp.snapshot.view, id: cph.m.id, mode: cph.mode, pkgPath: cph.m.pkgPath, diff --git a/internal/lsp/cache/errors.go b/internal/lsp/cache/errors.go index c03986f7da..e3440cfc44 100644 --- a/internal/lsp/cache/errors.go +++ b/internal/lsp/cache/errors.go @@ -27,7 +27,7 @@ func sourceError(ctx context.Context, pkg *pkg, e interface{}) (*source.Error, e fixes []source.SuggestedFix related []source.RelatedInformation ) - fset := pkg.snapshot.view.session.cache.fset + fset := pkg.view.session.cache.fset switch e := e.(type) { case packages.Error: if e.Pos == "" { @@ -64,18 +64,18 @@ func sourceError(ctx context.Context, pkg *pkg, e interface{}) (*source.Error, e spn, err = typeErrorRange(ctx, fset, pkg, e.Pos) case *analysis.Diagnostic: - spn, err = span.NewRange(pkg.snapshot.view.session.cache.fset, e.Pos, e.End).Span() + spn, err = span.NewRange(fset, e.Pos, e.End).Span() if err != nil { return nil, err } msg = e.Message kind = source.Analysis category = e.Category - fixes, err = suggestedFixes(ctx, pkg, e) + fixes, err = suggestedFixes(ctx, fset, pkg, e) if err != nil { return nil, err } - related, err = relatedInformation(ctx, pkg, e) + related, err = relatedInformation(ctx, fset, pkg, e) if err != nil { return nil, err } @@ -95,12 +95,12 @@ func sourceError(ctx context.Context, pkg *pkg, e interface{}) (*source.Error, e }, nil } -func suggestedFixes(ctx context.Context, pkg *pkg, diag *analysis.Diagnostic) ([]source.SuggestedFix, error) { +func suggestedFixes(ctx context.Context, fset *token.FileSet, pkg *pkg, diag *analysis.Diagnostic) ([]source.SuggestedFix, error) { var fixes []source.SuggestedFix for _, fix := range diag.SuggestedFixes { edits := make(map[span.URI][]protocol.TextEdit) for _, e := range fix.TextEdits { - spn, err := span.NewRange(pkg.Snapshot().View().Session().Cache().FileSet(), e.Pos, e.End).Span() + spn, err := span.NewRange(fset, e.Pos, e.End).Span() if err != nil { return nil, err } @@ -121,10 +121,10 @@ func suggestedFixes(ctx context.Context, pkg *pkg, diag *analysis.Diagnostic) ([ return fixes, nil } -func relatedInformation(ctx context.Context, pkg *pkg, diag *analysis.Diagnostic) ([]source.RelatedInformation, error) { +func relatedInformation(ctx context.Context, fset *token.FileSet, pkg *pkg, diag *analysis.Diagnostic) ([]source.RelatedInformation, error) { var out []source.RelatedInformation for _, related := range diag.Related { - spn, err := span.NewRange(pkg.Snapshot().View().Session().Cache().FileSet(), related.Pos, related.End).Span() + spn, err := span.NewRange(fset, related.Pos, related.End).Span() if err != nil { return nil, err } diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index e2866dd47f..ca094a9e2c 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -19,7 +19,7 @@ import ( // pkg contains the type information needed by the source package. type pkg struct { - snapshot *snapshot + view *view // ID and package path have their own types to avoid being used interchangeably. id packageID @@ -43,8 +43,8 @@ type pkg struct { type packageID string type packagePath string -func (p *pkg) Snapshot() source.Snapshot { - return p.snapshot +func (p *pkg) View() source.View { + return p.view } func (p *pkg) ID() string { @@ -139,8 +139,8 @@ func (p *pkg) FindDiagnostic(pdiag protocol.Diagnostic) (*source.Diagnostic, err func (p *pkg) FindFile(ctx context.Context, uri span.URI) (source.ParseGoHandle, source.Package, error) { // Special case for ignored files. - if p.snapshot.view.Ignore(uri) { - return p.snapshot.view.findIgnoredFile(ctx, uri) + if p.view.Ignore(uri) { + return p.view.findIgnoredFile(ctx, uri) } queue := []*pkg{p} diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go index 514cf04b0d..f45cca531a 100644 --- a/internal/lsp/source/completion_format.go +++ b/internal/lsp/source/completion_format.go @@ -136,7 +136,7 @@ func (c *completer) item(cand candidate) (CompletionItem, error) { if !(file.Pos() <= obj.Pos() && obj.Pos() <= file.End()) { return CompletionItem{}, errors.Errorf("no file for %s", obj.Name()) } - ident, err := findIdentifier(c.ctx, pkg, file, obj.Pos()) + ident, err := findIdentifier(c.ctx, c.snapshot, pkg, file, obj.Pos()) if err != nil { return CompletionItem{}, err } diff --git a/internal/lsp/source/hover.go b/internal/lsp/source/hover.go index c7a3f112d8..0123e1c3a7 100644 --- a/internal/lsp/source/hover.go +++ b/internal/lsp/source/hover.go @@ -47,7 +47,7 @@ func (i *IdentifierInfo) Hover(ctx context.Context) (*HoverInformation, error) { switch x := h.source.(type) { case ast.Node: var b strings.Builder - if err := format.Node(&b, i.Snapshot().View().Session().Cache().FileSet(), x); err != nil { + if err := format.Node(&b, i.Snapshot.View().Session().Cache().FileSet(), x); err != nil { return nil, err } h.Signature = b.String() diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index 531f833e5a..0d4b1d0ea5 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -20,8 +20,9 @@ import ( // IdentifierInfo holds information about an identifier in Go source. type IdentifierInfo struct { - Name string - File ParseGoHandle + Name string + File ParseGoHandle + Snapshot Snapshot mappedRange Type struct { @@ -37,10 +38,6 @@ type IdentifierInfo struct { qf types.Qualifier } -func (i *IdentifierInfo) Snapshot() Snapshot { - return i.pkg.Snapshot() -} - type Declaration struct { mappedRange node ast.Node @@ -51,7 +48,7 @@ type Declaration struct { // Identifier returns identifier information for a position // in a file, accounting for a potentially incomplete selector. func Identifier(ctx context.Context, view View, f File, pos protocol.Position) (*IdentifierInfo, error) { - _, cphs, err := view.CheckPackageHandles(ctx, f) + snapshot, cphs, err := view.CheckPackageHandles(ctx, f) if err != nil { return nil, err } @@ -79,17 +76,17 @@ func Identifier(ctx context.Context, view View, f File, pos protocol.Position) ( if err != nil { return nil, err } - return findIdentifier(ctx, pkg, file, rng.Start) + return findIdentifier(ctx, snapshot, pkg, file, rng.Start) } -func findIdentifier(ctx context.Context, pkg Package, file *ast.File, pos token.Pos) (*IdentifierInfo, error) { - if result, err := identifier(ctx, pkg, file, pos); err != nil || result != nil { +func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, file *ast.File, pos token.Pos) (*IdentifierInfo, error) { + if result, err := identifier(ctx, snapshot, pkg, file, pos); err != nil || result != nil { return result, err } // If the position is not an identifier but immediately follows // an identifier or selector period (as is common when // requesting a completion), use the path to the preceding node. - ident, err := identifier(ctx, pkg, file, pos-1) + ident, err := identifier(ctx, snapshot, pkg, file, pos-1) if ident == nil && err == nil { err = errors.New("no identifier found") } @@ -97,21 +94,21 @@ func findIdentifier(ctx context.Context, pkg Package, file *ast.File, pos token. } // identifier checks a single position for a potential identifier. -func identifier(ctx context.Context, pkg Package, file *ast.File, pos token.Pos) (*IdentifierInfo, error) { +func identifier(ctx context.Context, snapshot Snapshot, pkg Package, file *ast.File, pos token.Pos) (*IdentifierInfo, error) { ctx, done := trace.StartSpan(ctx, "source.identifier") defer done() var err error // Handle import specs separately, as there is no formal position for a package declaration. - if result, err := importSpec(ctx, pkg, file, pos); result != nil || err != nil { + if result, err := importSpec(ctx, snapshot, pkg, file, pos); result != nil || err != nil { return result, err } path, _ := astutil.PathEnclosingInterval(file, pos, pos) if path == nil { return nil, errors.Errorf("can't find node enclosing position") } - view := pkg.Snapshot().View() + view := pkg.View() uri := span.FileURI(view.Session().Cache().FileSet().Position(pos).Filename) var ph ParseGoHandle for _, h := range pkg.Files() { @@ -120,10 +117,11 @@ func identifier(ctx context.Context, pkg Package, file *ast.File, pos token.Pos) } } result := &IdentifierInfo{ - File: ph, - qf: qualifier(file, pkg.GetTypes(), pkg.GetTypesInfo()), - pkg: pkg, - ident: searchForIdent(path[0]), + File: ph, + Snapshot: snapshot, + qf: qualifier(file, pkg.GetTypes(), pkg.GetTypesInfo()), + pkg: pkg, + ident: searchForIdent(path[0]), } // No identifier at the given position. if result.ident == nil { @@ -244,7 +242,7 @@ func hasErrorType(obj types.Object) bool { } func objToNode(ctx context.Context, pkg Package, obj types.Object) (ast.Decl, error) { - view := pkg.Snapshot().View() + view := pkg.View() uri := span.FileURI(view.Session().Cache().FileSet().Position(obj.Pos()).Filename) ph, _, err := pkg.FindFile(ctx, uri) if err != nil { @@ -280,7 +278,7 @@ func objToNode(ctx context.Context, pkg Package, obj types.Object) (ast.Decl, er } // importSpec handles positions inside of an *ast.ImportSpec. -func importSpec(ctx context.Context, pkg Package, file *ast.File, pos token.Pos) (*IdentifierInfo, error) { +func importSpec(ctx context.Context, snapshot Snapshot, pkg Package, file *ast.File, pos token.Pos) (*IdentifierInfo, error) { var imp *ast.ImportSpec for _, spec := range file.Imports { if spec.Path.Pos() <= pos && pos < spec.Path.End() { @@ -294,7 +292,7 @@ func importSpec(ctx context.Context, pkg Package, file *ast.File, pos token.Pos) if err != nil { return nil, errors.Errorf("import path not quoted: %s (%v)", imp.Path.Value, err) } - uri := span.FileURI(pkg.Snapshot().View().Session().Cache().FileSet().Position(pos).Filename) + uri := span.FileURI(pkg.View().Session().Cache().FileSet().Position(pos).Filename) var ph ParseGoHandle for _, h := range pkg.Files() { if h.File().Identity().URI == uri { @@ -302,9 +300,10 @@ func importSpec(ctx context.Context, pkg Package, file *ast.File, pos token.Pos) } } result := &IdentifierInfo{ - File: ph, - Name: importPath, - pkg: pkg, + File: ph, + Snapshot: snapshot, + Name: importPath, + pkg: pkg, } if result.mappedRange, err = posToMappedRange(ctx, pkg, imp.Path.Pos(), imp.Path.End()); err != nil { return nil, err diff --git a/internal/lsp/source/rename.go b/internal/lsp/source/rename.go index 1ccdb7a8f7..9a46016653 100644 --- a/internal/lsp/source/rename.go +++ b/internal/lsp/source/rename.go @@ -151,7 +151,7 @@ func (i *IdentifierInfo) Rename(ctx context.Context, view View, newName string) if err != nil { return nil, err } - fh := i.Snapshot().Handle(ctx, f) + fh := i.Snapshot.Handle(ctx, f) data, _, err := fh.Read(ctx) if err != nil { return nil, err @@ -225,6 +225,7 @@ func getPkgNameIdentifier(ctx context.Context, ident *IdentifierInfo, pkgName *t return nil, err } return &IdentifierInfo{ + Snapshot: ident.Snapshot, Name: pkgName.Name(), mappedRange: decl.mappedRange, File: ident.File, diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index f74e1a15f6..431a80f1ea 100644 --- a/internal/lsp/source/util.go +++ b/internal/lsp/source/util.go @@ -159,7 +159,7 @@ func posToMappedRange(ctx context.Context, pkg Package, pos, end token.Pos) (map if err != nil { return mappedRange{}, err } - return posToRange(ctx, pkg.Snapshot().View(), m, pos, end) + return posToRange(ctx, pkg.View(), m, pos, end) } func posToRange(ctx context.Context, view View, m *protocol.ColumnMapper, pos, end token.Pos) (mappedRange, error) { @@ -176,7 +176,7 @@ func posToRange(ctx context.Context, view View, m *protocol.ColumnMapper, pos, e } func posToMapper(ctx context.Context, pkg Package, pos token.Pos) (*protocol.ColumnMapper, error) { - posn := pkg.Snapshot().View().Session().Cache().FileSet().Position(pos) + posn := pkg.View().Session().Cache().FileSet().Position(pos) ph, _, err := pkg.FindFile(ctx, span.FileURI(posn.Filename)) if err != nil { return nil, err diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 0f95dc2756..057641275f 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -274,7 +274,6 @@ type File interface { // Package represents a Go package that has been type-checked. It maintains // only the relevant fields of a *go/packages.Package. type Package interface { - Snapshot() Snapshot ID() string PkgPath() string Files() []ParseGoHandle @@ -295,6 +294,8 @@ type Package interface { // FindFile returns the AST and type information for a file that may // belong to or be part of a dependency of the given package. FindFile(ctx context.Context, uri span.URI) (ParseGoHandle, Package, error) + + View() View } type Error struct {