From cd1d0887dc8cfcfb844340a5fce628c61da00a20 Mon Sep 17 00:00:00 2001 From: Heschi Kreinick Date: Mon, 12 Apr 2021 18:44:46 -0400 Subject: [PATCH] internal/lsp/cache: trim more stuff in ParseExported mode Despite the name, ParseExported only hollowed out declarations -- it didn't actually drop any from the AST. This leaves a fair amount of unexported crud behind. Unfortunately, there are a *lot* of ways to expose an unexported declaration from an exported one, and it can be done across files. Because of that, discarding unexported declarations requires a lot of work. This CL implements a decent attempt at pruning as much as possible from the AST in ParseExported mode. First, we analyze the AST of all the files in the package for exported uses of unexported identifiers, iterating to a fixed point. Then, we type check those ASTs. If there are missing identifiers (probably due to a bug in the dependency analysis) we use those errors to re-parse. After that we give up and fall back to the older, less effective trimming. The pkg type changes slightly to accomodate the new control flow. We have to analyze all the files at once because an unexported type might be exposed in another file. Unfortunately, that means we can't parse a single file at a time any more -- the result of parsing a file depends on the result of parsing its siblings. To avoid cache corruption, we have to do the parsing directly in type checking, uncached. This, in turn, required changes to the PosTo* functions. Previously, they operated just on files, but a file name is no longer sufficient to get a ParseExported AST. Change them to work on Packages instead. I squeezed in a bit of refactoring while I was touching them. Change-Id: I61249144ffa43ad645ed38d79e873e3998b0f38d Reviewed-on: https://go-review.googlesource.com/c/tools/+/312471 Trust: Heschi Kreinick Run-TryBot: Heschi Kreinick gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Robert Findley Reviewed-by: Rebecca Stambler --- internal/lsp/cache/analysis.go | 2 +- internal/lsp/cache/check.go | 343 +++++++++++++---------- internal/lsp/cache/parse.go | 340 +++++++++++++++++++--- internal/lsp/cache/parse_test.go | 180 ++++++++++++ internal/lsp/cache/pkg.go | 31 +- internal/lsp/cache/snapshot.go | 1 - internal/lsp/source/completion/format.go | 17 +- internal/lsp/source/hover.go | 14 +- internal/lsp/source/identifier.go | 18 +- internal/lsp/source/signature_help.go | 6 +- internal/lsp/source/types_format.go | 9 +- internal/lsp/source/util.go | 13 +- internal/lsp/source/view.go | 11 +- 13 files changed, 709 insertions(+), 276 deletions(-) diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go index e3d744352bc..c9c50f97595 100644 --- a/internal/lsp/cache/analysis.go +++ b/internal/lsp/cache/analysis.go @@ -403,7 +403,7 @@ func (s *snapshot) DiagnosePackage(ctx context.Context, spkg source.Package) (ma analyzers = append(analyzers, a) } var errorAnalyzerDiag []*source.Diagnostic - if pkg.hasTypeErrors { + if pkg.HasTypeErrors() { var err error errorAnalyzerDiag, err = s.Analyze(ctx, pkg.ID(), analyzers) if err != nil { diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 50f75c083d0..58356ff1d37 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -9,7 +9,6 @@ import ( "context" "fmt" "go/ast" - "go/scanner" "go/types" "path" "path/filepath" @@ -303,26 +302,34 @@ func (s *snapshot) parseGoHandles(ctx context.Context, files []span.URI, mode so } func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source.ParseMode, deps map[packagePath]*packageHandle) (*pkg, error) { - ctx, done := event.Start(ctx, "cache.importer.typeCheck", tag.Package.Of(string(m.id))) - defer done() + var pkg *pkg + var err error - fset := snapshot.view.session.cache.fset - pkg := &pkg{ - m: m, - mode: mode, - goFiles: make([]*source.ParsedGoFile, len(m.goFiles)), - compiledGoFiles: make([]*source.ParsedGoFile, len(m.compiledGoFiles)), - imports: make(map[packagePath]*pkg), - typesSizes: m.typesSizes, - typesInfo: &types.Info{ - Types: make(map[ast.Expr]types.TypeAndValue), - Defs: make(map[*ast.Ident]types.Object), - Uses: make(map[*ast.Ident]types.Object), - Implicits: make(map[ast.Node]types.Object), - Selections: make(map[*ast.SelectorExpr]*types.Selection), - Scopes: make(map[ast.Node]*types.Scope), - }, + var filter *unexportedFilter + if mode == source.ParseExported { + filter = &unexportedFilter{uses: map[string]bool{}} + } + pkg, err = doTypeCheck(ctx, snapshot, m, mode, deps, filter) + if err != nil { + return nil, err } + + if mode == source.ParseExported { + // The AST filtering is a little buggy and may remove things it + // shouldn't. If we only got undeclared name errors, try one more + // time keeping those names. + missing, unexpected := filter.ProcessErrors(pkg.typeErrors) + if len(unexpected) == 0 && len(missing) != 0 { + event.Log(ctx, fmt.Sprintf("discovered missing identifiers: %v", missing), tag.Package.Of(string(m.id))) + pkg, err = doTypeCheck(ctx, snapshot, m, mode, deps, filter) + missing, unexpected = filter.ProcessErrors(pkg.typeErrors) + } + if len(unexpected) != 0 || len(missing) != 0 { + event.Log(ctx, fmt.Sprintf("falling back to safe trimming due to type errors: %v or still-missing identifiers: %v", unexpected, missing), tag.Package.Of(string(m.id))) + pkg, err = doTypeCheck(ctx, snapshot, m, mode, deps, nil) + } + } + // If this is a replaced module in the workspace, the version is // meaningless, and we don't want clients to access it. if m.module != nil { @@ -335,86 +342,133 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source Version: version, } } - var ( - files = make([]*ast.File, len(m.compiledGoFiles)) - parseErrors = make([]scanner.ErrorList, len(m.compiledGoFiles)) - actualErrors = make([]error, len(m.compiledGoFiles)) - wg sync.WaitGroup - - mu sync.Mutex - haveFixedFiles bool - ) - for i, cgf := range m.compiledGoFiles { - wg.Add(1) - go func(i int, cgf span.URI) { - defer wg.Done() - fh, err := snapshot.GetFile(ctx, cgf) - if err != nil { - actualErrors[i] = err - return - } - pgh := snapshot.parseGoHandle(ctx, fh, mode) - pgf, fixed, err := snapshot.parseGo(ctx, pgh) - if err != nil { - actualErrors[i] = err - return - } - pkg.compiledGoFiles[i] = pgf - files[i], parseErrors[i], actualErrors[i] = pgf.File, pgf.ParseErr, err - - // If we have fixed parse errors in any of the files, we should hide type - // errors, as they may be completely nonsensical. - mu.Lock() - haveFixedFiles = haveFixedFiles || fixed - mu.Unlock() - }(i, cgf) - } - for i, gf := range m.goFiles { - wg.Add(1) - // We need to parse the non-compiled go files, but we don't care about their errors. - go func(i int, gf span.URI) { - defer wg.Done() - fh, err := snapshot.GetFile(ctx, gf) - if err != nil { - return - } - pgf, _ := snapshot.ParseGo(ctx, fh, mode) - pkg.goFiles[i] = pgf - }(i, gf) + + // We don't care about a package's errors unless we have parsed it in full. + if mode != source.ParseFull { + return pkg, nil } - wg.Wait() - for _, err := range actualErrors { + + for _, e := range m.errors { + diags, err := goPackagesErrorDiagnostics(snapshot, pkg, e) if err != nil { - return nil, err + event.Error(ctx, "unable to compute positions for list errors", err, tag.Package.Of(pkg.ID())) + continue + } + pkg.diagnostics = append(pkg.diagnostics, diags...) + } + + // Our heuristic for whether to show type checking errors is: + // + If any file was 'fixed', don't show type checking errors as we + // can't guarantee that they reference accurate locations in the source. + // + If there is a parse error _in the current file_, suppress type + // errors in that file. + // + Otherwise, show type errors even in the presence of parse errors in + // other package files. go/types attempts to suppress follow-on errors + // due to bad syntax, so on balance type checking errors still provide + // a decent signal/noise ratio as long as the file in question parses. + + // Track URIs with parse errors so that we can suppress type errors for these + // files. + unparseable := map[span.URI]bool{} + for _, e := range pkg.parseErrors { + diags, err := parseErrorDiagnostics(snapshot, pkg, e) + if err != nil { + event.Error(ctx, "unable to compute positions for parse errors", err, tag.Package.Of(pkg.ID())) + continue + } + for _, diag := range diags { + unparseable[diag.URI] = true + pkg.diagnostics = append(pkg.diagnostics, diag) } } - var i int - for _, e := range parseErrors { - if e != nil { - parseErrors[i] = e - i++ + if pkg.hasFixedFiles { + return pkg, nil + } + + unexpanded := pkg.typeErrors + pkg.typeErrors = nil + for _, e := range expandErrors(unexpanded, snapshot.View().Options().RelatedInformationSupported) { + diags, err := typeErrorDiagnostics(snapshot, pkg, e) + if err != nil { + event.Error(ctx, "unable to compute positions for type errors", err, tag.Package.Of(pkg.ID())) + continue } + pkg.typeErrors = append(pkg.typeErrors, e.primary) + for _, diag := range diags { + // If the file didn't parse cleanly, it is highly likely that type + // checking errors will be confusing or redundant. But otherwise, type + // checking usually provides a good enough signal to include. + if !unparseable[diag.URI] { + pkg.diagnostics = append(pkg.diagnostics, diag) + } + } + } + + depsErrors, err := snapshot.depsErrors(ctx, pkg) + if err != nil { + return nil, err + } + pkg.diagnostics = append(pkg.diagnostics, depsErrors...) + + return pkg, nil +} + +func doTypeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source.ParseMode, deps map[packagePath]*packageHandle, astFilter *unexportedFilter) (*pkg, error) { + ctx, done := event.Start(ctx, "cache.typeCheck", tag.Package.Of(string(m.id))) + defer done() + + pkg := &pkg{ + m: m, + mode: mode, + imports: make(map[packagePath]*pkg), + types: types.NewPackage(string(m.pkgPath), string(m.name)), + typesInfo: &types.Info{ + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + Implicits: make(map[ast.Node]types.Object), + Selections: make(map[*ast.SelectorExpr]*types.Selection), + Scopes: make(map[ast.Node]*types.Scope), + }, + typesSizes: m.typesSizes, } - parseErrors = parseErrors[:i] - i = 0 - for _, f := range files { - if f != nil { - files[i] = f - i++ + for _, gf := range pkg.m.goFiles { + // In the presence of line directives, we may need to report errors in + // non-compiled Go files, so we need to register them on the package. + // However, we only need to really parse them in ParseFull mode, when + // the user might actually be looking at the file. + fh, err := snapshot.GetFile(ctx, gf) + if err != nil { + return nil, err + } + goMode := source.ParseFull + if mode != source.ParseFull { + goMode = source.ParseHeader + } + pgf, err := snapshot.ParseGo(ctx, fh, goMode) + if err != nil { + return nil, err } + pkg.goFiles = append(pkg.goFiles, pgf) + } + + if err := parseCompiledGoFiles(ctx, snapshot, mode, pkg, astFilter); err != nil { + return nil, err } - files = files[:i] // Use the default type information for the unsafe package. - if pkg.m.pkgPath == "unsafe" { - pkg.types = types.Unsafe + if m.pkgPath == "unsafe" { // Don't type check Unsafe: it's unnecessary, and doing so exposes a data // race to Unsafe.completed. + pkg.types = types.Unsafe return pkg, nil - } else if len(files) == 0 { // not the unsafe package, no parsed files - // Try to attach error messages to the file as much as possible. + } + + if len(m.compiledGoFiles) == 0 { + // No files most likely means go/packages failed. Try to attach error + // messages to the file as much as possible. var found bool for _, e := range m.errors { srcDiags, err := goPackagesErrorDiagnostics(snapshot, pkg, e) @@ -428,14 +482,11 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source return pkg, nil } return nil, errors.Errorf("no parsed files for package %s, expected: %v, errors: %v", pkg.m.pkgPath, pkg.compiledGoFiles, m.errors) - } else { - pkg.types = types.NewPackage(string(m.pkgPath), string(m.name)) } - var typeErrors []types.Error cfg := &types.Config{ Error: func(e error) { - typeErrors = append(typeErrors, e.(types.Error)) + pkg.typeErrors = append(pkg.typeErrors, e.(types.Error)) }, Importer: importerFunc(func(pkgPath string) (*types.Package, error) { // If the context was cancelled, we should abort. @@ -457,12 +508,22 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source return depPkg.types, nil }), } + + if mode != source.ParseFull { + cfg.DisableUnusedImportCheck = true + cfg.IgnoreFuncBodies = true + } + // We want to type check cgo code if go/types supports it. // We passed typecheckCgo to go/packages when we Loaded. typesinternal.SetUsesCgo(cfg) - check := types.NewChecker(cfg, fset, pkg.types, pkg.typesInfo) + check := types.NewChecker(cfg, snapshot.FileSet(), pkg.types, pkg.typesInfo) + var files []*ast.File + for _, cgf := range pkg.compiledGoFiles { + files = append(files, cgf.File) + } // Type checking errors are handled via the config, so ignore them here. _ = check.Files(files) // If the context was cancelled, we may have returned a ton of transient @@ -470,81 +531,53 @@ func typeCheck(ctx context.Context, snapshot *snapshot, m *metadata, mode source if ctx.Err() != nil { return nil, ctx.Err() } + return pkg, nil +} - // We don't care about a package's errors unless we have parsed it in full. - if mode != source.ParseFull { - return pkg, nil - } - - if len(m.errors) != 0 { - pkg.hasListOrParseErrors = true - for _, e := range m.errors { - diags, err := goPackagesErrorDiagnostics(snapshot, pkg, e) - if err != nil { - event.Error(ctx, "unable to compute positions for list errors", err, tag.Package.Of(pkg.ID())) - continue - } - pkg.diagnostics = append(pkg.diagnostics, diags...) +func parseCompiledGoFiles(ctx context.Context, snapshot *snapshot, mode source.ParseMode, pkg *pkg, astFilter *unexportedFilter) error { + for _, cgf := range pkg.m.compiledGoFiles { + fh, err := snapshot.GetFile(ctx, cgf) + if err != nil { + return err } - } - - // Our heuristic for whether to show type checking errors is: - // + If any file was 'fixed', don't show type checking errors as we - // can't guarantee that they reference accurate locations in the source. - // + If there is a parse error _in the current file_, suppress type - // errors in that file. - // + Otherwise, show type errors even in the presence of parse errors in - // other package files. go/types attempts to suppress follow-on errors - // due to bad syntax, so on balance type checking errors still provide - // a decent signal/noise ratio as long as the file in question parses. - // Track URIs with parse errors so that we can suppress type errors for these - // files. - unparseable := map[span.URI]bool{} - if len(parseErrors) != 0 { - pkg.hasListOrParseErrors = true - for _, e := range parseErrors { - diags, err := parseErrorDiagnostics(snapshot, pkg, e) - if err != nil { - event.Error(ctx, "unable to compute positions for parse errors", err, tag.Package.Of(pkg.ID())) - continue - } - for _, diag := range diags { - unparseable[diag.URI] = true - pkg.diagnostics = append(pkg.diagnostics, diag) - } + var pgf *source.ParsedGoFile + var fixed bool + // Only parse Full through the cache -- we need to own Exported ASTs + // to prune them. + if mode == source.ParseFull { + pgh := snapshot.parseGoHandle(ctx, fh, mode) + pgf, fixed, err = snapshot.parseGo(ctx, pgh) + } else { + d := parseGo(ctx, snapshot.FileSet(), fh, mode) + pgf, fixed, err = d.parsed, d.fixed, d.err } - } - - if haveFixedFiles { - return pkg, nil - } - - for _, e := range expandErrors(typeErrors, snapshot.View().Options().RelatedInformationSupported) { - pkg.hasTypeErrors = true - diags, err := typeErrorDiagnostics(snapshot, pkg, e) if err != nil { - event.Error(ctx, "unable to compute positions for type errors", err, tag.Package.Of(pkg.ID())) - continue + return err } - pkg.typeErrors = append(pkg.typeErrors, e.primary) - for _, diag := range diags { - // If the file didn't parse cleanly, it is highly likely that type - // checking errors will be confusing or redundant. But otherwise, type - // checking usually provides a good enough signal to include. - if !unparseable[diag.URI] { - pkg.diagnostics = append(pkg.diagnostics, diag) - } + pkg.compiledGoFiles = append(pkg.compiledGoFiles, pgf) + if pgf.ParseErr != nil { + pkg.parseErrors = append(pkg.parseErrors, pgf.ParseErr) } + // If we have fixed parse errors in any of the files, we should hide type + // errors, as they may be completely nonsensical. + pkg.hasFixedFiles = pkg.hasFixedFiles || fixed } - - depsErrors, err := snapshot.depsErrors(ctx, pkg) - if err != nil { - return nil, err + if mode != source.ParseExported { + return nil } - pkg.diagnostics = append(pkg.diagnostics, depsErrors...) - - return pkg, nil + if astFilter != nil { + var files []*ast.File + for _, cgf := range pkg.compiledGoFiles { + files = append(files, cgf.File) + } + astFilter.Filter(files) + } else { + for _, cgf := range pkg.compiledGoFiles { + trimAST(cgf.File) + } + } + return nil } func (s *snapshot) depsErrors(ctx context.Context, pkg *pkg) ([]*source.Diagnostic, error) { diff --git a/internal/lsp/cache/parse.go b/internal/lsp/cache/parse.go index e1332354380..3827cac977c 100644 --- a/internal/lsp/cache/parse.go +++ b/internal/lsp/cache/parse.go @@ -12,8 +12,10 @@ import ( "go/parser" "go/scanner" "go/token" + "go/types" "reflect" "strconv" + "strings" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/lsp/debug/tag" @@ -32,15 +34,10 @@ type parseKey struct { mode source.ParseMode } -// astCacheKey is similar to parseKey, but is a distinct type because -// it is used to key a different value within the same map. -type astCacheKey parseKey - type parseGoHandle struct { - handle *memoize.Handle - file source.FileHandle - mode source.ParseMode - astCacheHandle *memoize.Handle + handle *memoize.Handle + file source.FileHandle + mode source.ParseMode } type parseGoData struct { @@ -65,16 +62,10 @@ func (s *snapshot) parseGoHandle(ctx context.Context, fh source.FileHandle, mode return parseGo(ctx, snapshot.view.session.cache.fset, fh, mode) }, nil) - astHandle := s.generation.Bind(astCacheKey(key), func(ctx context.Context, arg memoize.Arg) interface{} { - snapshot := arg.(*snapshot) - return buildASTCache(ctx, snapshot, parseHandle) - }, nil) - pgh := &parseGoHandle{ - handle: parseHandle, - file: fh, - mode: mode, - astCacheHandle: astHandle, + handle: parseHandle, + file: fh, + mode: mode, } return s.addGoFile(key, pgh) } @@ -98,6 +89,9 @@ func (s *snapshot) ParseGo(ctx context.Context, fh source.FileHandle, mode sourc } func (s *snapshot) parseGo(ctx context.Context, pgh *parseGoHandle) (*source.ParsedGoFile, bool, error) { + if pgh.mode == source.ParseExported { + panic("only type checking should use Exported") + } d, err := pgh.handle.Get(ctx, s.generation, s) if err != nil { return nil, false, err @@ -106,36 +100,55 @@ func (s *snapshot) parseGo(ctx context.Context, pgh *parseGoHandle) (*source.Par return data.parsed, data.fixed, data.err } -func (s *snapshot) PosToDecl(ctx context.Context, pgf *source.ParsedGoFile) (map[token.Pos]ast.Decl, error) { - fh, err := s.GetFile(ctx, pgf.URI) +type astCacheKey struct { + pkg packageHandleKey + uri span.URI +} + +func (s *snapshot) astCacheData(ctx context.Context, spkg source.Package, pos token.Pos) (*astCacheData, error) { + pkg := spkg.(*pkg) + pkgHandle := s.getPackage(pkg.m.id, pkg.mode) + if pkgHandle == nil { + return nil, fmt.Errorf("could not reconstruct package handle for %v", pkg.m.id) + } + tok := s.FileSet().File(pos) + if tok == nil { + return nil, fmt.Errorf("no file for pos %v", pos) + } + pgf, err := pkg.File(span.URIFromPath(tok.Name())) if err != nil { return nil, err } + astHandle := s.generation.Bind(astCacheKey{pkgHandle.key, pgf.URI}, func(ctx context.Context, arg memoize.Arg) interface{} { + snapshot := arg.(*snapshot) + return buildASTCache(ctx, snapshot, pgf) + }, nil) - pgh := s.parseGoHandle(ctx, fh, pgf.Mode) - d, err := pgh.astCacheHandle.Get(ctx, s.generation, s) + d, err := astHandle.Get(ctx, s.generation, s) if err != nil { return nil, err } - data := d.(*astCacheData) - return data.posToDecl, data.err + if data.err != nil { + return nil, data.err + } + return data, nil } -func (s *snapshot) PosToField(ctx context.Context, pgf *source.ParsedGoFile) (map[token.Pos]*ast.Field, error) { - fh, err := s.GetFile(ctx, pgf.URI) +func (s *snapshot) PosToDecl(ctx context.Context, spkg source.Package, pos token.Pos) (ast.Decl, error) { + data, err := s.astCacheData(ctx, spkg, pos) if err != nil { return nil, err } + return data.posToDecl[pos], nil +} - pgh := s.parseGoHandle(ctx, fh, pgf.Mode) - d, err := pgh.astCacheHandle.Get(ctx, s.generation, s) - if err != nil || d == nil { +func (s *snapshot) PosToField(ctx context.Context, spkg source.Package, pos token.Pos) (*ast.Field, error) { + data, err := s.astCacheData(ctx, spkg, pos) + if err != nil { return nil, err } - - data := d.(*astCacheData) - return data.posToField, data.err + return data.posToField[pos], nil } type astCacheData struct { @@ -147,7 +160,7 @@ type astCacheData struct { // buildASTCache builds caches to aid in quickly going from the typed // world to the syntactic world. -func buildASTCache(ctx context.Context, snapshot *snapshot, parseHandle *memoize.Handle) *astCacheData { +func buildASTCache(ctx context.Context, snapshot *snapshot, pgf *source.ParsedGoFile) *astCacheData { var ( // path contains all ancestors, including n. path []ast.Node @@ -155,21 +168,12 @@ func buildASTCache(ctx context.Context, snapshot *snapshot, parseHandle *memoize decls []ast.Decl ) - v, err := parseHandle.Get(ctx, snapshot.generation, snapshot) - if err != nil { - return &astCacheData{err: err} - } - file := v.(*parseGoData).parsed.File - if err != nil { - return &astCacheData{err: fmt.Errorf("nil file")} - } - data := &astCacheData{ posToDecl: make(map[token.Pos]ast.Decl), posToField: make(map[token.Pos]*ast.Field), } - ast.Inspect(file, func(n ast.Node) bool { + ast.Inspect(pgf.File, func(n ast.Node) bool { if n == nil { lastP := path[len(path)-1] path = path[:len(path)-1] @@ -310,10 +314,6 @@ func parseGo(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mod } } - if mode == source.ParseExported { - trimAST(file) - } - return &parseGoData{ parsed: &source.ParsedGoFile{ URI: fh.URI(), @@ -332,6 +332,252 @@ func parseGo(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mod } } +// An unexportedFilter removes as much unexported AST from a set of Files as possible. +type unexportedFilter struct { + uses map[string]bool +} + +// Filter records uses of unexported identifiers and filters out all other +// unexported declarations. +func (f *unexportedFilter) Filter(files []*ast.File) { + // Iterate to fixed point -- unexported types can include other unexported types. + oldLen := len(f.uses) + for { + for _, file := range files { + f.recordUses(file) + } + if len(f.uses) == oldLen { + break + } + oldLen = len(f.uses) + } + + for _, file := range files { + var newDecls []ast.Decl + for _, decl := range file.Decls { + if f.filterDecl(decl) { + newDecls = append(newDecls, decl) + } + } + file.Decls = newDecls + file.Scope = nil + file.Unresolved = nil + file.Comments = nil + trimAST(file) + } +} + +func (f *unexportedFilter) keep(ident *ast.Ident) bool { + return ast.IsExported(ident.Name) || f.uses[ident.Name] +} + +func (f *unexportedFilter) filterDecl(decl ast.Decl) bool { + switch decl := decl.(type) { + case *ast.FuncDecl: + if ident := recvIdent(decl); ident != nil && !f.keep(ident) { + return false + } + return f.keep(decl.Name) + case *ast.GenDecl: + if decl.Tok == token.CONST { + // Constants can involve iota, and iota is hard to deal with. + return true + } + var newSpecs []ast.Spec + for _, spec := range decl.Specs { + if f.filterSpec(spec) { + newSpecs = append(newSpecs, spec) + } + } + decl.Specs = newSpecs + return len(newSpecs) != 0 + case *ast.BadDecl: + return false + } + panic(fmt.Sprintf("unknown ast.Decl %T", decl)) +} + +func (f *unexportedFilter) filterSpec(spec ast.Spec) bool { + switch spec := spec.(type) { + case *ast.ImportSpec: + return true + case *ast.ValueSpec: + var newNames []*ast.Ident + for _, name := range spec.Names { + if f.keep(name) { + newNames = append(newNames, name) + } + } + spec.Names = newNames + return len(spec.Names) != 0 + case *ast.TypeSpec: + if !f.keep(spec.Name) { + return false + } + switch typ := spec.Type.(type) { + case *ast.StructType: + f.filterFieldList(typ.Fields) + case *ast.InterfaceType: + f.filterFieldList(typ.Methods) + } + return true + } + panic(fmt.Sprintf("unknown ast.Spec %T", spec)) +} + +func (f *unexportedFilter) filterFieldList(fields *ast.FieldList) { + var newFields []*ast.Field + for _, field := range fields.List { + if len(field.Names) == 0 { + // Keep embedded fields: they can export methods and fields. + newFields = append(newFields, field) + } + for _, name := range field.Names { + if f.keep(name) { + newFields = append(newFields, field) + break + } + } + } + fields.List = newFields +} + +func (f *unexportedFilter) recordUses(file *ast.File) { + for _, decl := range file.Decls { + switch decl := decl.(type) { + case *ast.FuncDecl: + // Ignore methods on dropped types. + if ident := recvIdent(decl); ident != nil && !f.keep(ident) { + break + } + // Ignore functions with dropped names. + if !f.keep(decl.Name) { + break + } + f.recordFuncType(decl.Type) + case *ast.GenDecl: + for _, spec := range decl.Specs { + switch spec := spec.(type) { + case *ast.ValueSpec: + for i, name := range spec.Names { + // Don't mess with constants -- iota is hard. + if f.keep(name) || decl.Tok == token.CONST { + f.recordIdents(spec.Type) + if len(spec.Values) > i { + f.recordIdents(spec.Values[i]) + } + } + } + case *ast.TypeSpec: + switch typ := spec.Type.(type) { + case *ast.StructType: + f.recordFieldUses(false, typ.Fields) + case *ast.InterfaceType: + f.recordFieldUses(false, typ.Methods) + } + } + } + } + } +} + +// recvIdent returns the identifier of a method receiver, e.g. *int. +func recvIdent(decl *ast.FuncDecl) *ast.Ident { + if decl.Recv == nil || len(decl.Recv.List) == 0 { + return nil + } + x := decl.Recv.List[0].Type + if star, ok := x.(*ast.StarExpr); ok { + x = star.X + } + if ident, ok := x.(*ast.Ident); ok { + return ident + } + return nil +} + +// recordIdents records unexported identifiers in an Expr in uses. +// These may be types, e.g. in map[key]value, function names, e.g. in foo(), +// or simple variable references. References that will be discarded, such +// as those in function literal bodies, are ignored. +func (f *unexportedFilter) recordIdents(x ast.Expr) { + ast.Inspect(x, func(n ast.Node) bool { + if n == nil { + return false + } + if complit, ok := n.(*ast.CompositeLit); ok { + // We clear out composite literal contents; just record their type. + f.recordIdents(complit.Type) + return false + } + if flit, ok := n.(*ast.FuncLit); ok { + f.recordFuncType(flit.Type) + return false + } + if ident, ok := n.(*ast.Ident); ok && !ast.IsExported(ident.Name) { + f.uses[ident.Name] = true + } + return true + }) +} + +// recordFuncType records the types mentioned by a function type. +func (f *unexportedFilter) recordFuncType(x *ast.FuncType) { + f.recordFieldUses(true, x.Params) + f.recordFieldUses(true, x.Results) +} + +// recordFieldUses records unexported identifiers used in fields, which may be +// struct members, interface members, or function parameter/results. +func (f *unexportedFilter) recordFieldUses(isParams bool, fields *ast.FieldList) { + if fields == nil { + return + } + for _, field := range fields.List { + if isParams { + // Parameter types of retained functions need to be retained. + f.recordIdents(field.Type) + continue + } + if ft, ok := field.Type.(*ast.FuncType); ok { + // Function declarations in interfaces need all their types retained. + f.recordFuncType(ft) + continue + } + if len(field.Names) == 0 { + // Embedded fields might contribute exported names. + f.recordIdents(field.Type) + } + for _, name := range field.Names { + // We only need normal fields if they're exported. + if ast.IsExported(name.Name) { + f.recordIdents(field.Type) + break + } + } + } +} + +// ProcessErrors records additional uses from errors, returning the new uses +// and any unexpected errors. +func (f *unexportedFilter) ProcessErrors(errors []types.Error) (map[string]bool, []types.Error) { + var unexpected []types.Error + missing := map[string]bool{} + for _, err := range errors { + if strings.Contains(err.Msg, "missing return") { + continue + } + const undeclared = "undeclared name: " + if strings.HasPrefix(err.Msg, undeclared) { + missing[strings.TrimPrefix(err.Msg, undeclared)] = true + f.uses[strings.TrimPrefix(err.Msg, undeclared)] = true + continue + } + unexpected = append(unexpected, err) + } + return missing, unexpected +} + // trimAST clears any part of the AST not relevant to type checking // expressions at pos. func trimAST(file *ast.File) { @@ -353,6 +599,8 @@ func trimAST(file *ast.File) { // expensive. Try to clear them out. at, ok := n.Type.(*ast.ArrayType) if !ok { + // Composite literal. No harm removing all its fields. + n.Elts = nil break } // Removing the elements from an ellipsis array changes its type. diff --git a/internal/lsp/cache/parse_test.go b/internal/lsp/cache/parse_test.go index ff1b83f6694..cb620f27432 100644 --- a/internal/lsp/cache/parse_test.go +++ b/internal/lsp/cache/parse_test.go @@ -5,9 +5,17 @@ package cache import ( + "bytes" "go/ast" + "go/format" "go/parser" + "go/token" + "go/types" + "reflect" + "sort" "testing" + + "golang.org/x/tools/go/packages" ) func TestArrayLength(t *testing.T) { @@ -35,3 +43,175 @@ func TestArrayLength(t *testing.T) { } } } + +func TestTrim(t *testing.T) { + tests := []struct { + name string + file string + kept []string + }{ + { + name: "delete_unused", + file: ` +type x struct{} +func y() +var z int +`, + kept: []string{}, + }, + { + // From the common type in testing. + name: "unexported_embedded", + file: ` +type x struct {} +type Exported struct { x } +`, + kept: []string{"Exported", "x"}, + }, + { + // From the d type in unicode. + name: "exported_field_unexported_type", + file: ` +type x struct {} +type Exported struct { + X x +} +`, + kept: []string{"Exported", "x"}, + }, + { + // From errNotExist in io/fs. + name: "exported_var_function_call", + file: ` +func x() int { return 0 } +var Exported = x() +`, + kept: []string{"Exported", "x"}, + }, + { + // From DefaultServeMux in net/http. + name: "exported_pointer_to_unexported_var", + file: ` +var Exported = &x +var x int +`, + kept: []string{"Exported", "x"}, + }, + { + // From DefaultWriter in goldmark/renderer/html. + name: "exported_pointer_to_composite_lit", + file: ` +var Exported = &x{} +type x struct{} +`, + kept: []string{"Exported", "x"}, + }, + { + // From SelectDir in reflect. + name: "leave_constants", + file: ` +type Enum int +const ( + _ Enum = iota + EnumOne +) +`, + kept: []string{"Enum", "EnumOne"}, + }, + { + name: "constant_conversion", + file: ` +type x int +const ( + foo x = 0 +) +`, + kept: []string{"x", "foo"}, + }, + { + name: "unexported_return", + file: ` +type x int +func Exported() x {} +type y int +type Interface interface { + Exported() y +} +`, + kept: []string{"Exported", "Interface", "x", "y"}, + }, + { + name: "drop_composite_literals", + file: ` +type x int +type Exported struct { + foo x +} +var Var = Exported{foo:1} +`, + kept: []string{"Exported", "Var"}, + }, + { + name: "drop_function_literals", + file: ` +type x int +var Exported = func() { return x(0) } +`, + kept: []string{"Exported"}, + }, + { + name: "missing_receiver_panic", + file: ` + func() foo() {} +`, + kept: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "main.go", "package main\n\n"+tt.file, parser.AllErrors) + if err != nil { + t.Fatal(err) + } + filter := &unexportedFilter{uses: map[string]bool{}} + filter.Filter([]*ast.File{file}) + pkg := types.NewPackage("main", "main") + checker := types.NewChecker(&types.Config{ + DisableUnusedImportCheck: true, + }, fset, pkg, nil) + if err := checker.Files([]*ast.File{file}); err != nil { + t.Error(err) + } + names := pkg.Scope().Names() + sort.Strings(names) + sort.Strings(tt.kept) + if !reflect.DeepEqual(names, tt.kept) { + t.Errorf("package contains names %v, wanted %v", names, tt.kept) + } + }) + } +} + +func TestPkg(t *testing.T) { + t.Skip("for manual debugging") + fset := token.NewFileSet() + pkgs, err := packages.Load(&packages.Config{ + Mode: packages.NeedSyntax | packages.NeedFiles, + Fset: fset, + }, "io") + if err != nil { + t.Fatal(err) + } + if len(pkgs[0].Errors) != 0 { + t.Fatal(pkgs[0].Errors) + } + filter := &unexportedFilter{uses: map[string]bool{}} + filter.Filter(pkgs[0].Syntax) + for _, file := range pkgs[0].Syntax { + buf := &bytes.Buffer{} + format.Node(buf, fset, file) + t.Log(buf.String()) + } +} diff --git a/internal/lsp/cache/pkg.go b/internal/lsp/cache/pkg.go index 69b3e3f0493..aa075641f03 100644 --- a/internal/lsp/cache/pkg.go +++ b/internal/lsp/cache/pkg.go @@ -6,6 +6,7 @@ package cache import ( "go/ast" + "go/scanner" "go/types" "golang.org/x/mod/module" @@ -16,19 +17,19 @@ import ( // pkg contains the type information needed by the source package. type pkg struct { - m *metadata - mode source.ParseMode - goFiles []*source.ParsedGoFile - compiledGoFiles []*source.ParsedGoFile - diagnostics []*source.Diagnostic - imports map[packagePath]*pkg - version *module.Version - typeErrors []types.Error - types *types.Package - typesInfo *types.Info - typesSizes types.Sizes - hasListOrParseErrors bool - hasTypeErrors bool + m *metadata + mode source.ParseMode + goFiles []*source.ParsedGoFile + compiledGoFiles []*source.ParsedGoFile + diagnostics []*source.Diagnostic + imports map[packagePath]*pkg + version *module.Version + parseErrors []scanner.ErrorList + typeErrors []types.Error + types *types.Package + typesInfo *types.Info + typesSizes types.Sizes + hasFixedFiles bool } // Declare explicit types for package paths, names, and IDs to ensure that we @@ -146,9 +147,9 @@ func (p *pkg) Version() *module.Version { } func (p *pkg) HasListOrParseErrors() bool { - return p.hasListOrParseErrors + return len(p.m.errors) != 0 || len(p.parseErrors) != 0 } func (p *pkg) HasTypeErrors() bool { - return p.hasTypeErrors + return len(p.typeErrors) != 0 } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index cdb8fb3e637..3716996b60a 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -1394,7 +1394,6 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC continue } newGen.Inherit(v.handle) - newGen.Inherit(v.astCacheHandle) result.goFiles[k] = v } diff --git a/internal/lsp/source/completion/format.go b/internal/lsp/source/completion/format.go index 3c9c3b2725a..1d021074399 100644 --- a/internal/lsp/source/completion/format.go +++ b/internal/lsp/source/completion/format.go @@ -213,27 +213,16 @@ func (c *completer) item(ctx context.Context, cand candidate) (CompletionItem, e } uri := span.URIFromPath(pos.Filename) - // Find the source file of the candidate, starting from a package - // that should have it in its dependencies. - searchPkg := c.pkg - if cand.imp != nil && cand.imp.pkg != nil { - searchPkg = cand.imp.pkg - } - - pgf, pkg, err := source.FindPosInPackage(c.snapshot, searchPkg, obj.Pos()) + // Find the source file of the candidate. + pkg, err := source.FindPackageFromPos(ctx, c.snapshot, obj.Pos()) if err != nil { return item, nil } - posToDecl, err := c.snapshot.PosToDecl(ctx, pgf) + decl, err := c.snapshot.PosToDecl(ctx, pkg, obj.Pos()) if err != nil { return CompletionItem{}, err } - decl := posToDecl[obj.Pos()] - if decl == nil { - return item, nil - } - hover, err := source.HoverInfo(ctx, c.snapshot, pkg, obj, decl) if err != nil { event.Error(ctx, "failed to find Hover", err, tag.URI.Of(uri)) diff --git a/internal/lsp/source/hover.go b/internal/lsp/source/hover.go index 89e0903d88e..cd4e6e8d875 100644 --- a/internal/lsp/source/hover.go +++ b/internal/lsp/source/hover.go @@ -19,7 +19,6 @@ import ( "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/lsp/protocol" - "golang.org/x/tools/internal/span" errors "golang.org/x/xerrors" ) @@ -320,21 +319,12 @@ func HoverInfo(ctx context.Context, s Snapshot, pkg Package, obj types.Object, n break } - f := s.FileSet().File(obj.Pos()) - if f == nil { - break - } - - pgf, err := pkg.File(span.URIFromPath(f.Name())) - if err != nil { - return nil, err - } - posToField, err := s.PosToField(ctx, pgf) + field, err := s.PosToField(ctx, pkg, obj.Pos()) if err != nil { return nil, err } - if field := posToField[obj.Pos()]; field != nil { + if field != nil { comment := field.Doc if comment.Text() == "" { comment = field.Comment diff --git a/internal/lsp/source/identifier.go b/internal/lsp/source/identifier.go index 78abcdf24b3..41534d070af 100644 --- a/internal/lsp/source/identifier.go +++ b/internal/lsp/source/identifier.go @@ -262,7 +262,11 @@ func findIdentifier(ctx context.Context, snapshot Snapshot, pkg Package, file *a } result.Declaration.MappedRange = append(result.Declaration.MappedRange, rng) - if result.Declaration.node, err = objToDecl(ctx, snapshot, pkg, result.Declaration.obj); err != nil { + declPkg, err := FindPackageFromPos(ctx, snapshot, result.Declaration.obj.Pos()) + if err != nil { + return nil, nil + } + if result.Declaration.node, err = snapshot.PosToDecl(ctx, declPkg, result.Declaration.obj.Pos()); err != nil { return nil, err } typ := pkg.GetTypesInfo().TypeOf(result.ident) @@ -363,18 +367,6 @@ func hasErrorType(obj types.Object) bool { return types.IsInterface(obj.Type()) && obj.Pkg() == nil && obj.Name() == "error" } -func objToDecl(ctx context.Context, snapshot Snapshot, srcPkg Package, obj types.Object) (ast.Decl, error) { - pgf, _, err := FindPosInPackage(snapshot, srcPkg, obj.Pos()) - if err != nil { - return nil, err - } - posToDecl, err := snapshot.PosToDecl(ctx, pgf) - if err != nil { - return nil, err - } - return posToDecl[obj.Pos()], nil -} - // importSpec handles positions inside of an *ast.ImportSpec. func importSpec(snapshot Snapshot, pkg Package, file *ast.File, pos token.Pos) (*IdentifierInfo, error) { var imp *ast.ImportSpec diff --git a/internal/lsp/source/signature_help.go b/internal/lsp/source/signature_help.go index 90cd5549e94..9270787d686 100644 --- a/internal/lsp/source/signature_help.go +++ b/internal/lsp/source/signature_help.go @@ -93,7 +93,11 @@ FindCall: comment *ast.CommentGroup ) if obj != nil { - node, err := objToDecl(ctx, snapshot, pkg, obj) + declPkg, err := FindPackageFromPos(ctx, snapshot, obj.Pos()) + if err != nil { + return nil, 0, err + } + node, err := snapshot.PosToDecl(ctx, declPkg, obj.Pos()) if err != nil { return nil, 0, err } diff --git a/internal/lsp/source/types_format.go b/internal/lsp/source/types_format.go index 96110cd97df..fdc76f68ea1 100644 --- a/internal/lsp/source/types_format.go +++ b/internal/lsp/source/types_format.go @@ -219,12 +219,12 @@ func NewSignature(ctx context.Context, s Snapshot, pkg Package, sig *types.Signa // To do this, it looks in the AST of the file in which the object is declared. // On any errors, it always fallbacks back to types.TypeString. func FormatVarType(ctx context.Context, snapshot Snapshot, srcpkg Package, obj *types.Var, qf types.Qualifier) string { - pgf, pkg, err := FindPosInPackage(snapshot, srcpkg, obj.Pos()) + pkg, err := FindPackageFromPos(ctx, snapshot, obj.Pos()) if err != nil { return types.TypeString(obj.Type(), qf) } - expr, err := varType(ctx, snapshot, pgf, obj) + expr, err := varType(ctx, snapshot, pkg, obj) if err != nil { return types.TypeString(obj.Type(), qf) } @@ -244,12 +244,11 @@ func FormatVarType(ctx context.Context, snapshot Snapshot, srcpkg Package, obj * } // varType returns the type expression for a *types.Var. -func varType(ctx context.Context, snapshot Snapshot, pgf *ParsedGoFile, obj *types.Var) (ast.Expr, error) { - posToField, err := snapshot.PosToField(ctx, pgf) +func varType(ctx context.Context, snapshot Snapshot, pkg Package, obj *types.Var) (ast.Expr, error) { + field, err := snapshot.PosToField(ctx, pkg, obj.Pos()) if err != nil { return nil, err } - field := posToField[obj.Pos()] if field == nil { return nil, fmt.Errorf("no declaration for object %s", obj.Name()) } diff --git a/internal/lsp/source/util.go b/internal/lsp/source/util.go index a8707323189..8865a55740e 100644 --- a/internal/lsp/source/util.go +++ b/internal/lsp/source/util.go @@ -274,20 +274,19 @@ func CompareDiagnostic(a, b *Diagnostic) int { return 1 } -// FindPosInPackage finds the parsed file for a position in a given search +// FindPackageFromPos finds the parsed file for a position in a given search // package. -func FindPosInPackage(snapshot Snapshot, searchpkg Package, pos token.Pos) (*ParsedGoFile, Package, error) { +func FindPackageFromPos(ctx context.Context, snapshot Snapshot, pos token.Pos) (Package, error) { tok := snapshot.FileSet().File(pos) if tok == nil { - return nil, nil, errors.Errorf("no file for pos in package %s", searchpkg.ID()) + return nil, errors.Errorf("no file for pos %v", pos) } uri := span.URIFromPath(tok.Name()) - - pgf, pkg, err := findFileInDeps(searchpkg, uri) + pkgs, err := snapshot.PackagesForFile(ctx, uri, TypecheckWorkspace) if err != nil { - return nil, nil, err + return nil, err } - return pgf, pkg, nil + return pkgs[0], nil } // findFileInDeps finds uri in pkg or its dependencies. diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go index 51fe3eaa1e2..6612d534813 100644 --- a/internal/lsp/source/view.go +++ b/internal/lsp/source/view.go @@ -80,12 +80,12 @@ type Snapshot interface { // to quickly find corresponding *ast.Field node given a *types.Var. // We must refer to the AST to render type aliases properly when // formatting signatures and other types. - PosToField(ctx context.Context, pgf *ParsedGoFile) (map[token.Pos]*ast.Field, error) + PosToField(ctx context.Context, pkg Package, pos token.Pos) (*ast.Field, error) // PosToDecl maps certain objects' positions to their surrounding // ast.Decl. This mapping is used when building the documentation // string for the objects. - PosToDecl(ctx context.Context, pgf *ParsedGoFile) (map[token.Pos]ast.Decl, error) + PosToDecl(ctx context.Context, pkg Package, pos token.Pos) (ast.Decl, error) // DiagnosePackage returns basic diagnostics, including list, parse, and type errors // for pkg, grouped by file. @@ -419,10 +419,9 @@ const ( // This is the mode used when attempting to examine the package graph structure. ParseHeader ParseMode = iota - // ParseExported specifies that the public symbols are needed, but things like - // private symbols and function bodies are not. - // This mode is used for things where a package is being consumed only as a - // dependency. + // ParseExported specifies that the package is used only as a dependency, + // and only its exported declarations are needed. More may be included if + // necessary to avoid type errors. ParseExported // ParseFull specifies the full AST is needed.