From f520afa52e4fc37ae4d4ff9b31ff23d028579ba9 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Fri, 5 Jun 2020 17:30:52 -0400 Subject: [PATCH] internal/lsp: remove Ignore feature Ignore ignored the builtin package and files that start with _. The latter should already be ignored by "go list". The former seems like too much effort to me. People shouldn't edit random parts of the stdlib, and ignoring changes to (e.g.) the Error interface seems like the least of the trouble they can get themselves into. Remove it for now. If we get complains I'll re-add it, probably by rejecting the write entirely somewhere. We incidentally relied on this in the identifier functions; change those to treat the builtin package slightly more specially. Change-Id: I005b02a66b1a987c50a3074d53a2d28ff07d3324 Reviewed-on: https://go-review.googlesource.com/c/tools/+/237597 Run-TryBot: Heschi Kreinick TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cache/session.go | 4 -- internal/lsp/cache/view.go | 62 ++++++++--------------- internal/lsp/source/completion_builtin.go | 4 +- internal/lsp/source/diagnostics.go | 8 +-- internal/lsp/source/identifier.go | 9 ++-- internal/lsp/source/types_format.go | 4 +- internal/lsp/source/util.go | 34 +------------ internal/lsp/source/view.go | 12 +++-- 8 files changed, 41 insertions(+), 96 deletions(-) diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index 543f6ff10c..34448afba4 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -146,7 +146,6 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, unloadableFiles: make(map[span.URI]struct{}), modHandles: make(map[span.URI]*modHandle), }, - ignoredURIs: make(map[span.URI]struct{}), gocmdRunner: &gocommand.Runner{}, } v.snapshot.view = v @@ -319,9 +318,6 @@ func (s *Session) DidModifyFiles(ctx context.Context, changes []source.FileModif // Look through all of the session's views, invalidating the file for // all of the views to which it is known. for _, view := range s.views { - if view.Ignore(c.URI) { - return nil, errors.Errorf("ignored file %v", c.URI) - } // Don't propagate changes that are outside of the view's scope // or knowledge. if !view.relevantChange(c) { diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go index 76bb7aaeda..d4a59dbc97 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -81,10 +81,6 @@ type View struct { snapshotMu sync.Mutex snapshot *snapshot - // ignoredURIs is the set of URIs of files that we ignore. - ignoredURIsMu sync.Mutex - ignoredURIs map[span.URI]struct{} - // initialized is closed when the view has been fully initialized. // On initialization, the view's workspace packages are loaded. // All of the fields below are set as part of initialization. @@ -127,9 +123,18 @@ type builtinPackageData struct { memoize.NoCopy pkg *ast.Package + pgh *parseGoHandle err error } +func (d *builtinPackageData) Package() *ast.Package { + return d.pkg +} + +func (d *builtinPackageData) ParseGoHandle() source.ParseGoHandle { + return d.pgh +} + // fileBase holds the common functionality for all files. // It is intended to be embedded in the file implementations type fileBase struct { @@ -212,7 +217,7 @@ func (v *View) Rebuild(ctx context.Context) (source.Snapshot, error) { return snapshot, err } -func (v *View) LookupBuiltin(ctx context.Context, name string) (*ast.Object, error) { +func (v *View) BuiltinPackage(ctx context.Context) (source.BuiltinPackage, error) { v.awaitInitialized(ctx) if v.builtin == nil { @@ -235,11 +240,7 @@ func (v *View) LookupBuiltin(ctx context.Context, name string) (*ast.Object, err if d.pkg == nil || d.pkg.Scope == nil { return nil, errors.Errorf("no builtin package") } - astObj := d.pkg.Scope.Lookup(name) - if astObj == nil { - return nil, errors.Errorf("no builtin object for %s", name) - } - return astObj, nil + return d, nil } func (v *View) buildBuiltinPackage(ctx context.Context, goFiles []string) error { @@ -247,7 +248,6 @@ func (v *View) buildBuiltinPackage(ctx context.Context, goFiles []string) error return errors.Errorf("only expected 1 file, got %v", len(goFiles)) } uri := span.URIFromPath(goFiles[0]) - v.addIgnoredFile(uri) // to avoid showing diagnostics for builtin.go // Get the FileHandle through the cache to avoid adding it to the snapshot // and to get the file content from disk. @@ -255,19 +255,23 @@ func (v *View) buildBuiltinPackage(ctx context.Context, goFiles []string) error if err != nil { return err } - pgh := v.session.cache.ParseGoHandle(ctx, fh, source.ParseFull) + pgh := v.session.cache.parseGoHandle(ctx, fh, source.ParseFull) fset := v.session.cache.fset h := v.session.cache.store.Bind(fh.Identity(), func(ctx context.Context) interface{} { - data := &builtinPackageData{} file, _, _, _, err := pgh.Parse(ctx) if err != nil { - data.err = err - return data + return &builtinPackageData{err: err} } - data.pkg, data.err = ast.NewPackage(fset, map[string]*ast.File{ + pkg, err := ast.NewPackage(fset, map[string]*ast.File{ pgh.File().URI().Filename(): file, }, nil, nil) - return data + if err != nil { + return &builtinPackageData{err: err} + } + return &builtinPackageData{ + pgh: pgh, + pkg: pkg, + } }) v.builtin = &builtinPackageHandle{ handle: h, @@ -534,30 +538,6 @@ func (v *View) shutdown(ctx context.Context) { } } -// Ignore checks if the given URI is a URI we ignore. -// As of right now, we only ignore files in the "builtin" package. -func (v *View) Ignore(uri span.URI) bool { - v.ignoredURIsMu.Lock() - defer v.ignoredURIsMu.Unlock() - - _, ok := v.ignoredURIs[uri] - - // Files with _ prefixes are always ignored. - if !ok && strings.HasPrefix(filepath.Base(uri.Filename()), "_") { - v.ignoredURIs[uri] = struct{}{} - return true - } - - return ok -} - -func (v *View) addIgnoredFile(uri span.URI) { - v.ignoredURIsMu.Lock() - defer v.ignoredURIsMu.Unlock() - - v.ignoredURIs[uri] = struct{}{} -} - func (v *View) BackgroundContext() context.Context { v.mu.Lock() defer v.mu.Unlock() diff --git a/internal/lsp/source/completion_builtin.go b/internal/lsp/source/completion_builtin.go index f314146c5b..e67efb0d34 100644 --- a/internal/lsp/source/completion_builtin.go +++ b/internal/lsp/source/completion_builtin.go @@ -14,13 +14,13 @@ import ( // argument. It attempts to use the AST hints from builtin.go where // possible. func (c *completer) builtinArgKind(ctx context.Context, obj types.Object, call *ast.CallExpr) objKind { - astObj, err := c.snapshot.View().LookupBuiltin(ctx, obj.Name()) + builtin, err := c.snapshot.View().BuiltinPackage(ctx) if err != nil { return 0 } exprIdx := exprAtPos(c.pos, call.Args) - decl, ok := astObj.Decl.(*ast.FuncDecl) + decl, ok := builtin.Package().Scope.Lookup(obj.Name()).Decl.(*ast.FuncDecl) if !ok || exprIdx >= len(decl.Type.Params.List) { return 0 } diff --git a/internal/lsp/source/diagnostics.go b/internal/lsp/source/diagnostics.go index 2d59d5aa7e..e558e737bb 100644 --- a/internal/lsp/source/diagnostics.go +++ b/internal/lsp/source/diagnostics.go @@ -213,7 +213,7 @@ func diagnostics(ctx context.Context, snapshot Snapshot, reports map[FileIdentit } func missingModulesDiagnostics(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][]*Diagnostic, missingModules map[string]*modfile.Require, uri span.URI) error { - if snapshot.View().Ignore(uri) || len(missingModules) == 0 { + if len(missingModules) == 0 { return nil } fh, err := snapshot.GetFile(ctx, uri) @@ -305,9 +305,6 @@ func analyses(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][ } func clearReports(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][]*Diagnostic, uri span.URI) { - if snapshot.View().Ignore(uri) { - return - } fh := snapshot.FindFile(uri) if fh == nil { return @@ -316,9 +313,6 @@ func clearReports(ctx context.Context, snapshot Snapshot, reports map[FileIdenti } func addReports(ctx context.Context, snapshot Snapshot, reports map[FileIdentity][]*Diagnostic, uri span.URI, diagnostics ...*Diagnostic) error { - if snapshot.View().Ignore(uri) { - return nil - } fh := snapshot.FindFile(uri) if fh == nil { return nil diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index 85ca19fb1e..0c6d28804c 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -165,20 +165,23 @@ func findIdentifier(ctx context.Context, s Snapshot, pkg Package, file *ast.File // Handle builtins separately. if result.Declaration.obj.Parent() == types.Universe { - astObj, err := view.LookupBuiltin(ctx, result.Name) + builtin, err := view.BuiltinPackage(ctx) if err != nil { return nil, err } - decl, ok := astObj.Decl.(ast.Node) + decl, ok := builtin.Package().Scope.Lookup(result.Name).Decl.(ast.Node) if !ok { return nil, errors.Errorf("no declaration for %s", result.Name) } result.Declaration.node = decl - rng, err := nameToMappedRange(view, pkg, decl.Pos(), result.Name) + // The builtin package isn't in the dependency graph, so the usual utilities + // won't work here. + _, _, m, _, err := builtin.ParseGoHandle().Cached() if err != nil { return nil, err } + rng := newMappedRange(view.Session().Cache().FileSet(), m, decl.Pos(), decl.Pos()+token.Pos(len(result.Name))) result.Declaration.MappedRange = append(result.Declaration.MappedRange, rng) return result, nil diff --git a/internal/lsp/source/types_format.go b/internal/lsp/source/types_format.go index b9e89558a4..53323f9dde 100644 --- a/internal/lsp/source/types_format.go +++ b/internal/lsp/source/types_format.go @@ -75,11 +75,11 @@ func (s *signature) format() string { } func newBuiltinSignature(ctx context.Context, view View, name string) (*signature, error) { - astObj, err := view.LookupBuiltin(ctx, name) + builtin, err := view.BuiltinPackage(ctx) if err != nil { return nil, err } - decl, ok := astObj.Decl.(*ast.FuncDecl) + decl, ok := builtin.Package().Scope.Lookup(name).Decl.(*ast.FuncDecl) if !ok { return nil, fmt.Errorf("no function declaration for builtin: %s", name) } diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index 1d2f2ba44a..ca0df199a9 100644 --- a/internal/lsp/source/util.go +++ b/internal/lsp/source/util.go @@ -536,17 +536,7 @@ func findPosInPackage(v View, searchpkg Package, pos token.Pos) (*ast.File, Pack } uri := span.URIFromPath(tok.Name()) - var ( - ph ParseGoHandle - pkg Package - err error - ) - // Special case for ignored files. - if v.Ignore(uri) { - ph, err = findIgnoredFile(v, uri) - } else { - ph, pkg, err = FindFileInPackage(searchpkg, uri) - } + ph, pkg, err := FindFileInPackage(searchpkg, uri) if err != nil { return nil, nil, err } @@ -561,16 +551,7 @@ func findPosInPackage(v View, searchpkg Package, pos token.Pos) (*ast.File, Pack } func findMapperInPackage(v View, searchpkg Package, uri span.URI) (*protocol.ColumnMapper, error) { - var ( - ph ParseGoHandle - err error - ) - // Special case for ignored files. - if v.Ignore(uri) { - ph, err = findIgnoredFile(v, uri) - } else { - ph, _, err = FindFileInPackage(searchpkg, uri) - } + ph, _, err := FindFileInPackage(searchpkg, uri) if err != nil { return nil, err } @@ -581,17 +562,6 @@ func findMapperInPackage(v View, searchpkg Package, uri span.URI) (*protocol.Col return m, nil } -func findIgnoredFile(v View, uri span.URI) (ParseGoHandle, error) { - // Using the View's context here is not good, but threading a context - // through to this function is prohibitively difficult for such a rare - // code path. - fh, err := v.Snapshot().GetFile(v.BackgroundContext(), uri) - if err != nil { - return nil, err - } - return v.Session().Cache().ParseGoHandle(v.BackgroundContext(), fh, ParseFull), nil -} - // FindFileInPackage finds uri in pkg or its dependencies. func FindFileInPackage(pkg Package, uri span.URI) (ParseGoHandle, Package, error) { queue := []Package{pkg} diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 529e985480..d60e37c377 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -113,8 +113,8 @@ type View interface { // ModFiles returns the URIs of the go.mod files attached to the view associated with this snapshot. ModFiles() (span.URI, span.URI) - // LookupBuiltin returns the go/ast.Object for the given name in the builtin package. - LookupBuiltin(ctx context.Context, name string) (*ast.Object, error) + // BuiltinPackage returns the go/ast.Object for the given name in the builtin package. + BuiltinPackage(ctx context.Context) (BuiltinPackage, error) // BackgroundContext returns a context used for all background processing // on behalf of this view. @@ -123,9 +123,6 @@ type View interface { // Shutdown closes this view, and detaches it from it's session. Shutdown(ctx context.Context) - // Ignore returns true if this file should be ignored by this view. - Ignore(span.URI) bool - // WriteEnv writes the view-specific environment to the io.Writer. WriteEnv(ctx context.Context, w io.Writer) error @@ -158,6 +155,11 @@ type View interface { IsGoPrivatePath(path string) bool } +type BuiltinPackage interface { + Package() *ast.Package + ParseGoHandle() ParseGoHandle +} + // Session represents a single connection from a client. // This is the level at which things like open files are maintained on behalf // of the client.