diff --git a/internal/document/document.go b/internal/document/document.go index e9f6a3a..3c8b39a 100644 --- a/internal/document/document.go +++ b/internal/document/document.go @@ -38,11 +38,14 @@ func IsDocDeprecated(docs []string) bool { func NewDocument( relative string, + originAbs string, pkg *packages.Package, pkgSymbols *lookup.Package, ) *Document { return &Document{ RelativePath: relative, + originAbs: originAbs, + lineLen: loadLineLengths(originAbs), pkg: pkg, pkgSymbols: pkgSymbols, @@ -70,6 +73,90 @@ type Document struct { // pkgSymbols maps positions to symbol names within // this document. pkgSymbols *lookup.Package + + // originAbs is the cleaned, symlink-resolved absolute path of the source file + // this document represents; lineLen is the byte length of each of its lines + // (0-indexed), or nil if it couldn't be read. Together they let AppendOccurrence + // callers reject occurrences whose //line-adjusted range escapes the real file. + originAbs string + lineLen []int + + // occurrences accumulates every occurrence routed to this document. Usually + // that is only its own file's, but a generated file may attribute occurrences + // here via //line directives (e.g. cgo's rewritten source). extraSymbols + // accumulates SymbolInformation from the file(s) that map here. + occurrences []*scip.Occurrence + extraSymbols []*scip.SymbolInformation +} + +// InBounds reports whether r is a well-formed range that fits within this +// document's source file. It rejects: +// - negative line/column (a `//line file:N` directive with no column collapses +// positions to column 0 -> scip -1, which is malformed); +// - lines past EOF or columns past the line (cgo's `defer C.f(x)` rewrites and +// inserted `_cgoCheckPointer` thunks land here); +// - reversed ranges (end before start). +// +// Such occurrences cannot be faithfully represented and are dropped rather than +// emitted with a bogus location (which downstream SCIP consumers reject). A +// document whose source could not be read admits everything (no over-dropping). +func (d *Document) InBounds(r scip.Range) bool { + sl, sc, el, ec := int(r.Start.Line), int(r.Start.Character), int(r.End.Line), int(r.End.Character) + if sl < 0 || sc < 0 || el < 0 || ec < 0 { + return false + } + if el < sl || (el == sl && ec < sc) { + return false + } + if d.lineLen == nil { + return true + } + if sl >= len(d.lineLen) || el >= len(d.lineLen) { + return false + } + return sc <= d.lineLen[sl] && ec <= d.lineLen[el] +} + +// AppendOccurrence records occ against this document. Called from the single +// file-walking goroutine, so no synchronization is required. +func (d *Document) AppendOccurrence(occ *scip.Occurrence) { + d.occurrences = append(d.occurrences, occ) +} + +// AddSymbols records SymbolInformation contributed by a file that maps here. +func (d *Document) AddSymbols(syms []*scip.SymbolInformation) { + d.extraSymbols = append(d.extraSymbols, syms...) +} + +// ToScip renders the accumulated occurrences and symbols as a scip.Document. +func (d *Document) ToScip() *scip.Document { + occurrences := d.occurrences + if d.PackageOccurrence != nil { + occurrences = append([]*scip.Occurrence{d.PackageOccurrence}, occurrences...) + } + return &scip.Document{ + Language: "go", + RelativePath: d.RelativePath, + Occurrences: occurrences, + Symbols: d.extraSymbols, + } +} + +// loadLineLengths returns the byte length of each line of path (0-indexed), or +// nil if it can't be read. Used to bounds-check occurrence ranges against the +// real source (cgo rewrites some constructs to positions past the original +// line/EOF; those can't be faithfully represented and are dropped). +func loadLineLengths(path string) []int { + b, err := os.ReadFile(path) + if err != nil { + return nil + } + lines := strings.Split(string(b), "\n") + lengths := make([]int, len(lines)) + for i, line := range lines { + lengths[i] = len(strings.TrimSuffix(line, "\r")) + } + return lengths } func (d *Document) GetSymbol(pos token.Pos) (string, bool) { diff --git a/internal/document/inbounds_test.go b/internal/document/inbounds_test.go new file mode 100644 index 0000000..53fa362 --- /dev/null +++ b/internal/document/inbounds_test.go @@ -0,0 +1,56 @@ +package document + +import ( + "testing" + + "github.com/scip-code/scip/bindings/go/scip" +) + +func rng(sl, sc, el, ec int32) scip.Range { + return scip.Range{ + Start: scip.Position{Line: sl, Character: sc}, + End: scip.Position{Line: el, Character: ec}, + } +} + +func TestInBounds(t *testing.T) { + // Two lines, byte lengths 5 and 10. + d := &Document{lineLen: []int{5, 10}} + + cases := []struct { + name string + r scip.Range + want bool + }{ + {"in bounds, single line", rng(0, 0, 0, 5), true}, + {"end column at EOL", rng(1, 0, 1, 10), true}, + {"spans two lines", rng(0, 1, 1, 2), true}, + {"start column past EOL", rng(0, 6, 0, 6), false}, + {"end column past EOL", rng(1, 0, 1, 11), false}, + {"line past EOF", rng(2, 0, 2, 0), false}, + {"negative start column", rng(0, -1, 0, -1), false}, + {"negative line", rng(-1, 0, -1, 0), false}, + {"reversed same line", rng(0, 4, 0, 1), false}, + {"reversed across lines", rng(1, 0, 0, 0), false}, + } + for _, tc := range cases { + if got := d.InBounds(tc.r); got != tc.want { + t.Errorf("%s: InBounds(%v) = %v, want %v", tc.name, tc.r, got, tc.want) + } + } +} + +func TestInBoundsUnknownSourceAdmitsWellFormed(t *testing.T) { + // A document whose source couldn't be read (lineLen == nil) must not + // over-drop: it admits any well-formed range but still rejects malformed ones. + d := &Document{} + if !d.InBounds(rng(999, 0, 999, 3)) { + t.Error("nil lineLen should admit a well-formed range") + } + if d.InBounds(rng(0, -1, 0, 0)) { + t.Error("nil lineLen should still reject a negative column") + } + if d.InBounds(rng(2, 0, 1, 0)) { + t.Error("nil lineLen should still reject a reversed range") + } +} diff --git a/internal/index/scip.go b/internal/index/scip.go index b42e9ed..747db72 100644 --- a/internal/index/scip.go +++ b/internal/index/scip.go @@ -70,10 +70,15 @@ func ListMissing(opts config.IndexOpts) (missing []string, err error) { } for _, pkg := range projectPackages { + goFiles := visitors.RealGoFiles(pkg) for _, f := range pkg.Syntax { - docName := pkg.Fset.File(f.Package).Name() - if _, ok := pathToDocuments[docName]; !ok { - missing = append(missing, docName) + origin := visitors.OriginFile(pkg, f.Package) + if _, isReal := goFiles[origin]; !isReal { + // Generated file with no real-source origin (e.g. cgo glue). + continue + } + if _, ok := pathToDocuments[origin]; !ok { + missing = append(missing, origin) } } } @@ -120,7 +125,6 @@ func Index(writer func(proto.Message) error, opts config.IndexOpts) error { var count uint64 var wg sync.WaitGroup - var writeErr error wg.Add(1) go func() { @@ -131,30 +135,31 @@ func Index(writer func(proto.Message) error, opts config.IndexOpts) error { pkgSymbols := globalSymbols.GetPackage(pkg) for _, file := range pkg.Syntax { - doc := pathToDocument[pkg.Fset.File(file.Package).Name()] + origin := visitors.OriginFile(pkg, file.Package) + doc := pathToDocument[origin] if doc == nil { + // No document: a generated file (e.g. cgo's + // _cgo_gotypes.go) whose occurrences are compiler glue. continue } - // If possible, any state required for created a scip document - // should be contained in the visitor. This makes sure that we can - // garbage collect everything that's there after each loop, - // rather than holding on to every occurrence and piece of data + // The visitor routes each occurrence to the document of its + // //line-adjusted origin (usually this file, but a generated file + // can attribute some occurrences to another real source file), so + // it needs the whole document map, not just this file's document. visitor := visitors.NewFileVisitor( doc, pkg, file, pkgSymbols, globalSymbols, + pathToDocument, ) - // Traverse the file + // Traverse the file (routing occurrences), then attach this + // file's symbols to its document. ast.Walk(visitor, file) - - // Write the document - if writeErr = writer(visitor.ToScipDocument()); writeErr != nil { - return - } + visitor.Finish() } atomic.AddUint64(&count, 1) @@ -163,8 +168,12 @@ func Index(writer func(proto.Message) error, opts config.IndexOpts) error { output.WithProgressParallel(&wg, "Visiting Project Files", &count, uint64(pkgLen)) - if writeErr != nil { - return writeErr + // Emit one document per source file -- occurrences routed from every file + // that maps here are now accumulated -- in a stable, path-sorted order. + for _, origin := range slices.Sorted(maps.Keys(pathToDocument)) { + if err := writer(pathToDocument[origin].ToScip()); err != nil { + return err + } } // Emit external symbols for remote types that implement local interfaces @@ -227,14 +236,20 @@ func indexVisitPackages( Text: "package " + pkg.Name, }, } - firstFile := pkg.Syntax[0] - firstDoc := pathToDocuments[pkg.Fset.File(firstFile.Package).Name()] - firstDoc.SetSymbolInformation(firstFile.Name.NamePos, symInfo) - + // Attach the package symbol to the first real document and a + // package occurrence to each. Generated files (e.g. cgo's + // _cgo_gotypes.go) have no document, so skip them. + pkgDeclared := false for _, f := range pkg.Syntax { - doc := pathToDocuments[pkg.Fset.File(f.Package).Name()] + doc := pathToDocuments[visitors.OriginFile(pkg, f.Package)] + if doc == nil { + continue + } + if !pkgDeclared { + doc.SetSymbolInformation(f.Name.NamePos, symInfo) + pkgDeclared = true + } position := pkg.Fset.Position(f.Name.NamePos) - doc.PackageOccurrence = &scip.Occurrence{ TypedRange: symbols.RangeFromName(position, f.Name.Name, false).AsTypedRange(), Symbol: pkgSymbol, diff --git a/internal/loader/loader.go b/internal/loader/loader.go index 39b812e..ab258e0 100644 --- a/internal/loader/loader.go +++ b/internal/loader/loader.go @@ -22,6 +22,7 @@ import ( type PackageLookup map[newtypes.PackageID]*packages.Package var loadMode = packages.NeedExportFile | + packages.NeedFiles | packages.NeedImports | packages.NeedSyntax | packages.NeedTypes | diff --git a/internal/visitors/visitor_file.go b/internal/visitors/visitor_file.go index 9e5fb63..02ebad4 100644 --- a/internal/visitors/visitor_file.go +++ b/internal/visitors/visitor_file.go @@ -21,6 +21,7 @@ func NewFileVisitor( file *ast.File, pkgSymbols *lookup.Package, globalSymbols *lookup.Global, + docs map[string]*document.Document, ) *fileVisitor { caseClauses := map[token.Pos]types.Object{} for implicit, obj := range pkg.TypesInfo.Implicits { @@ -29,19 +30,14 @@ func NewFileVisitor( } } - // Package occurrence always goes into the list of occurrences for a document - occurrences := []*scip.Occurrence{ - doc.PackageOccurrence, - } - return &fileVisitor{ doc: doc, + docs: docs, pkg: pkg, file: file, locals: map[token.Pos]lookup.Local{}, pkgSymbols: pkgSymbols, globalSymbols: globalSymbols, - occurrences: occurrences, caseClauses: caseClauses, } } @@ -51,9 +47,19 @@ func NewFileVisitor( // // Iterates over a file, type fileVisitor struct { - // Document to append occurrences to + // doc is the document for this file's own origin; SymbolInformation defined in + // the file is attached here in Finish. Occurrences are routed per-occurrence + // to the document of their //line-adjusted origin (see docs) -- usually doc, + // but a generated file (e.g. cgo's rewritten source) can attribute some of its + // occurrences to a different real source file. doc *document.Document + // docs maps a resolved-origin absolute path to its document (the same map the + // index builds -- one entry per real source file). Occurrences are routed here + // by origin; an occurrence whose origin has no entry (cgo glue, a yacc `.y`, a + // build-cache path) is dropped rather than mis-attributed. + docs map[string]*document.Document + // Current file information pkg *packages.Package file *ast.File @@ -67,9 +73,6 @@ type fileVisitor struct { // field definition position to symbol for the entire compliation globalSymbols *lookup.Global - // occurrences in this file - occurrences []*scip.Occurrence - // caseClauses maps particular positions to different types for case clauses caseClauses map[token.Pos]types.Object @@ -117,8 +120,9 @@ func (v *fileVisitor) Visit(n ast.Node) ast.Visitor { if node.Name != nil && node.Name.Name != "." && node.Name.Name != "_" { if sym, ok := v.globalSymbols.GetPkgSymbol(importedPackage); ok { - v.newReference(sym, symbols.RangeFromName( - v.pkg.Fset.Position(node.Name.Pos()), node.Name.Name, false), false) + namePos := v.pkg.Fset.Position(node.Name.Pos()) + v.newReference(namePos, sym, symbols.RangeFromName( + namePos, node.Name.Name, false), false) } } @@ -147,7 +151,7 @@ func (v *fileVisitor) Visit(n ast.Node) ast.Visitor { } symRange := scipRange(startPosition, endPosition, sel) - v.newReference(sym, symRange, false) + v.newReference(startPosition, sym, symRange, false) // Then walk the selection ast.Walk(v, node.Sel) @@ -194,7 +198,7 @@ func (v *fileVisitor) Visit(n ast.Node) ast.Visitor { // Short circuit on case clauses if obj, ok := v.caseClauses[node.Pos()]; ok { symName := v.createNewLocalSymbol(obj.Pos(), obj) - v.newDefinition(symName, scipRange(startPosition, endPosition, obj), nil, false) + v.newDefinition(startPosition, symName, scipRange(startPosition, endPosition, obj), nil, false) return nil } @@ -213,6 +217,7 @@ func (v *fileVisitor) Visit(n ast.Node) ast.Visitor { } v.newDefinition( + startPosition, symName, scipRange(startPosition, endPosition, def), v.enclosingRange(node), @@ -255,7 +260,7 @@ func (v *fileVisitor) Visit(n ast.Node) ast.Visitor { deprecated = document.IsDocDeprecated(symInfo.Documentation) } - v.newReference(symbol, scipRange(startPosition, endPosition, ref), deprecated) + v.newReference(startPosition, symbol, scipRange(startPosition, endPosition, ref), deprecated) } if def == nil && ref == nil { @@ -282,31 +287,57 @@ func (v *fileVisitor) emitImportReference( return } - v.newReference(sym, symbols.RangeFromName(position, importedPackage.PkgPath, true), false) + v.newReference(position, sym, symbols.RangeFromName(position, importedPackage.PkgPath, true), false) +} + +// targetDoc resolves the document an occurrence at pos with range rng belongs +// to, or nil if it should be dropped. An occurrence's true home is its +// //line-adjusted origin file: usually the file being walked, but a generated +// file (cgo, ...) can attribute an occurrence to a *different* real source file, +// in which case it is routed there rather than dropped. An occurrence whose +// origin is not a real source document (cgo glue, a yacc `.y`, a build-cache +// path), or whose range escapes that document's source, is dropped rather than +// emitted with a bogus location (which downstream SCIP consumers reject). +func (v *fileVisitor) targetDoc(pos token.Position, rng scip.Range) *document.Document { + doc := v.docs[CleanResolve(pos.Filename)] + if doc == nil || !doc.InBounds(rng) { + return nil + } + return doc } // newDefinition emits a scip.Occurence ONLY. This will not emit a // new symbol. You must do that using DeclareNewSymbol[ForPos] func (v *fileVisitor) newDefinition( - symbol string, rng scip.Range, enclRng *scip.Range, deprecated bool, + pos token.Position, symbol string, rng scip.Range, enclRng *scip.Range, deprecated bool, ) { + doc := v.targetDoc(pos, rng) + if doc == nil { + return + } occ := &scip.Occurrence{ TypedRange: rng.AsTypedRange(), Symbol: symbol, SymbolRoles: int32(scip.SymbolRole_Definition), } - if enclRng != nil { + // Keep the enclosing range only if it fits the same source (a cgo-expanded + // body can push it past EOF even when the name range is fine). + if enclRng != nil && doc.InBounds(*enclRng) { occ.TypedEnclosingRange = enclRng.AsTypedEnclosingRange() } if deprecated { occ.Diagnostics = deprecatedDiagnostics() } - v.occurrences = append(v.occurrences, occ) + doc.AppendOccurrence(occ) } func (v *fileVisitor) newReference( - symbol string, rng scip.Range, deprecated bool, + pos token.Position, symbol string, rng scip.Range, deprecated bool, ) { + doc := v.targetDoc(pos, rng) + if doc == nil { + return + } occ := &scip.Occurrence{ TypedRange: rng.AsTypedRange(), Symbol: symbol, @@ -315,13 +346,16 @@ func (v *fileVisitor) newReference( if deprecated { occ.Diagnostics = deprecatedDiagnostics() } - v.occurrences = append(v.occurrences, occ) + doc.AppendOccurrence(occ) } -func (v *fileVisitor) ToScipDocument() *scip.Document { +// Finish attaches the file's SymbolInformation (package-level symbols defined in +// the file, plus locals) to the file's own document. Occurrences are routed to +// their origin documents during the walk; call Finish once after the walk. +func (v *fileVisitor) Finish() { documentFile := v.pkg.Fset.File(v.file.Pos()) if documentFile == nil { - panic("that shouldn't happend") + return } documentSymbols := v.pkgSymbols.SymbolsForFile(documentFile) @@ -349,12 +383,7 @@ func (v *fileVisitor) ToScipDocument() *scip.Document { documentSymbols = append(documentSymbols, symbolInfo) } - return &scip.Document{ - Language: "go", - RelativePath: v.doc.RelativePath, - Occurrences: v.occurrences, - Symbols: documentSymbols, - } + v.doc.AddSymbols(documentSymbols) } func (v *fileVisitor) enclosingRange(n *ast.Ident) *scip.Range { diff --git a/internal/visitors/visitors.go b/internal/visitors/visitors.go index 525092c..f7b2b33 100644 --- a/internal/visitors/visitors.go +++ b/internal/visitors/visitors.go @@ -15,6 +15,46 @@ import ( "golang.org/x/tools/go/packages" ) +// OriginFile returns the `//line`-adjusted source path for pos, cleaned. +// +// cgo (and any generated code carrying `//line` directives) is compiled from +// files that the go command rewrites into the build cache -- e.g. a cgo file +// `foo.go` becomes `foo.cgo1.go` under GOCACHE. `Fset.File(pos).Name()` returns +// that physical cache path, but `Fset.Position(pos)` honors the `//line` +// directives cgo emits and resolves back to the real `.go` source. Keying +// documents by this origin keeps occurrences anchored to source the repo +// actually contains, instead of an ephemeral cache path. For ordinary files the +// origin is the file itself, so non-generated packages are unaffected. +func OriginFile(pkg *packages.Package, pos token.Pos) string { + return CleanResolve(pkg.Fset.Position(pos).Filename) +} + +// CleanResolve returns path with symlinks resolved and cleaned. Resolving +// symlinks keeps `//line`-derived origins comparable to pkg.GoFiles even when a +// module is reached through a symlinked directory (otherwise a real source file +// could be dropped as "not a GoFile"). Falls back to Clean when the path can't +// be resolved -- e.g. a `//line` target that names a file not on disk (a yacc +// `.y` grammar, or a build-cache path) -- which then simply won't match any +// GoFile and is dropped, as intended. +func CleanResolve(path string) string { + if resolved, err := filepath.EvalSymlinks(path); err == nil { + return resolved + } + return filepath.Clean(path) +} + +// RealGoFiles is the set of a package's on-disk source files (resolved paths). +// An occurrence whose origin is not in this set comes from generated glue with +// no real source (cgo's `_cgo_gotypes.go`, a yacc `.y`, compiler-inserted thunks +// lacking a `//line`); it is dropped rather than mis-attributed. +func RealGoFiles(pkg *packages.Package) map[string]struct{} { + set := make(map[string]struct{}, len(pkg.GoFiles)) + for _, f := range pkg.GoFiles { + set[CleanResolve(f)] = struct{}{} + } + return set +} + func VisitPackageSyntax( moduleRoot string, pkg *packages.Package, @@ -22,23 +62,28 @@ func VisitPackageSyntax( globalSymbols *lookup.Global, ) { pkgSymbols := lookup.NewPackageSymbols(pkg) + goFiles := RealGoFiles(pkg) // Iterate over all the files, collect any global symbols for _, f := range pkg.Syntax { - abs := pkg.Fset.File(f.Package).Name() - relative, _ := filepath.Rel(moduleRoot, abs) + origin := OriginFile(pkg, f.Package) + relative, _ := filepath.Rel(moduleRoot, origin) - doc := visitSyntax(pkg, pkgSymbols, f, relative) - - // Save document for pass 2 - pathToDocuments[abs] = doc + // Always visit to collect package-level symbols, but only keep a + // document for files that map to real source. Generated files (e.g. + // cgo's `_cgo_gotypes.go`) resolve to a non-source origin; their + // occurrences are compiler glue and must not become a document. + doc := visitSyntax(pkg, pkgSymbols, f, relative, origin) + if _, ok := goFiles[origin]; ok { + pathToDocuments[origin] = doc + } } globalSymbols.Add(pkgSymbols) } -func visitSyntax(pkg *packages.Package, pkgSymbols *lookup.Package, f *ast.File, relative string) *document.Document { - doc := document.NewDocument(relative, pkg, pkgSymbols) +func visitSyntax(pkg *packages.Package, pkgSymbols *lookup.Package, f *ast.File, relative, originAbs string) *document.Document { + doc := document.NewDocument(relative, originAbs, pkg, pkgSymbols) // TODO: Maybe we should do this before? we have traverse all // the fields first before, but now I think it's fine right here