From 744a854b8ecefaf768395263b45fce00a51d5ec5 Mon Sep 17 00:00:00 2001 From: Jacopo Bacchelli Date: Thu, 30 Jul 2026 07:55:56 -0700 Subject: [PATCH 1/2] Anchor cgo occurrences to real .go source via //line directives scip-go emitted one SCIP Document per *compiled* Go file, keyed by pkg.Fset.File(f.Package).Name(). For cgo packages that physical file is cgo's generated output under the build cache (foo.cgo1.go, _cgo_gotypes.go), so every occurrence in a cgo file -- including each `C.` call site -- was attributed to an ephemeral GOCACHE path instead of the user's real .go source. Occurrence *ranges* were already computed with Fset.Position(), which honors the //line directives cgo emits, so line/column were correct -- only the document they were attached to was wrong. Fix: key documents (and route each occurrence) by the //line-adjusted origin file, and drop occurrences that don't map to real source: - loader: request NeedFiles so pkg.GoFiles is populated. - visitors.OriginFile(pkg, pos): the cleaned, //line-adjusted path for a position. visitors.RealGoFiles(pkg): the package's on-disk source set. - VisitPackageSyntax / index.Index / ListMissing: key documents by OriginFile instead of the physical compiled path, and skip files whose origin is not real source (e.g. cgo's _cgo_gotypes.go glue). - fileVisitor: drop occurrences whose //line-adjusted position resolves to another file, or to a line/column outside the origin. cgo rewrites such as `defer C.f(x)` and inserted thunks like _cgoCheckPointer otherwise yield out-of-bounds ranges that downstream SCIP consumers reject. Non-cgo packages are unaffected (origin == physical file), so existing snapshots are unchanged. For cgo, `C.` references now resolve to the real .go at the call site. --- internal/index/scip.go | 35 +++++++++---- internal/loader/loader.go | 1 + internal/visitors/visitor_file.go | 82 ++++++++++++++++++++++++++++--- internal/visitors/visitors.go | 41 ++++++++++++++-- 4 files changed, 136 insertions(+), 23 deletions(-) diff --git a/internal/index/scip.go b/internal/index/scip.go index b42e9ed..50c4b63 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) } } } @@ -131,8 +136,11 @@ 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 } @@ -146,6 +154,7 @@ func Index(writer func(proto.Message) error, opts config.IndexOpts) error { file, pkgSymbols, globalSymbols, + origin, ) // Traverse the file @@ -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..3d5db79 100644 --- a/internal/visitors/visitor_file.go +++ b/internal/visitors/visitor_file.go @@ -6,6 +6,9 @@ import ( "go/token" "go/types" "log/slog" + "os" + "path/filepath" + "strings" "github.com/scip-code/scip-go/internal/document" "github.com/scip-code/scip-go/internal/lookup" @@ -21,6 +24,7 @@ func NewFileVisitor( file *ast.File, pkgSymbols *lookup.Package, globalSymbols *lookup.Global, + originPath string, ) *fileVisitor { caseClauses := map[token.Pos]types.Object{} for implicit, obj := range pkg.TypesInfo.Implicits { @@ -38,6 +42,8 @@ func NewFileVisitor( doc: doc, pkg: pkg, file: file, + originPath: originPath, + originLineLen: loadLineLengths(originPath), locals: map[token.Pos]lookup.Local{}, pkgSymbols: pkgSymbols, globalSymbols: globalSymbols, @@ -46,6 +52,24 @@ func NewFileVisitor( } } +// 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 (e.g. `defer C.f(x)`) and inserts +// glue whose //line-adjusted position lands past the original line/EOF; such +// occurrences 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 +} + // fileVisitor visits an entire file, but it must be called // after StructVisitor. // @@ -58,6 +82,17 @@ type fileVisitor struct { pkg *packages.Package file *ast.File + // originPath is the cleaned, //line-adjusted source file this document + // represents. Occurrences whose adjusted position resolves elsewhere (cgo + // glue, compiler-inserted thunks with no //line) are dropped rather than + // mis-attributed to this file. See visitors.OriginFile. + originPath string + + // originLineLen holds the byte length of each line of originPath (0-indexed), + // or nil if it couldn't be read. Used to drop occurrences whose //line range + // falls outside the real source. See loadLineLengths. + originLineLen []int + // local definition position to symbol and its type information locals map[token.Pos]lookup.Local @@ -117,8 +152,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 +183,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 +230,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 +249,7 @@ func (v *fileVisitor) Visit(n ast.Node) ast.Visitor { } v.newDefinition( + startPosition, symName, scipRange(startPosition, endPosition, def), v.enclosingRange(node), @@ -255,7 +292,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,14 +319,40 @@ 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) +} + +// inOrigin reports whether an occurrence at pos with range rng belongs to, and +// fits within, this document's source file. cgo (and other //line-annotated +// generated code) can place occurrences at positions that resolve to a +// different generated file, or to a line/column past the real source (e.g. a +// rewritten `defer C.f(x)` or an inserted `_cgoCheckPointer`). Such occurrences +// cannot be faithfully represented and are dropped rather than emitted with an +// out-of-bounds range (which downstream SCIP consumers reject). +func (v *fileVisitor) inOrigin(pos token.Position, rng scip.Range) bool { + if filepath.Clean(pos.Filename) != v.originPath { + return false + } + // Fall back to the filename check alone if the origin couldn't be read. + if v.originLineLen == nil { + return true + } + sl, el := int(rng.Start.Line), int(rng.End.Line) + if sl < 0 || sl >= len(v.originLineLen) || el < 0 || el >= len(v.originLineLen) { + return false + } + return int(rng.Start.Character) <= v.originLineLen[sl] && + int(rng.End.Character) <= v.originLineLen[el] } // 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, ) { + if !v.inOrigin(pos, rng) { + return + } occ := &scip.Occurrence{ TypedRange: rng.AsTypedRange(), Symbol: symbol, @@ -305,8 +368,11 @@ func (v *fileVisitor) newDefinition( } func (v *fileVisitor) newReference( - symbol string, rng scip.Range, deprecated bool, + pos token.Position, symbol string, rng scip.Range, deprecated bool, ) { + if !v.inOrigin(pos, rng) { + return + } occ := &scip.Occurrence{ TypedRange: rng.AsTypedRange(), Symbol: symbol, diff --git a/internal/visitors/visitors.go b/internal/visitors/visitors.go index 525092c..3de2be2 100644 --- a/internal/visitors/visitors.go +++ b/internal/visitors/visitors.go @@ -15,6 +15,32 @@ 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 filepath.Clean(pkg.Fset.Position(pos).Filename) +} + +// RealGoFiles is the set of a package's on-disk source files (cleaned paths). +// Occurrences whose OriginFile is not in this set come from generated glue with +// no real source (e.g. cgo's `_cgo_gotypes.go`, or compiler-inserted thunks +// lacking a `//line`), and are 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[filepath.Clean(f)] = struct{}{} + } + return set +} + func VisitPackageSyntax( moduleRoot string, pkg *packages.Package, @@ -22,16 +48,21 @@ 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) + // 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) - - // Save document for pass 2 - pathToDocuments[abs] = doc + if _, ok := goFiles[origin]; ok { + pathToDocuments[origin] = doc + } } globalSymbols.Add(pkgSymbols) From 6155e695e1b3667f7d355bd72a104b867f8e3fa8 Mon Sep 17 00:00:00 2001 From: Jacopo Bacchelli Date: Thu, 30 Jul 2026 09:21:56 -0700 Subject: [PATCH 2/2] Route occurrences to their //line origin instead of dropping The previous commit attributed each document to its file's //line origin and dropped any occurrence that didn't fit that one file. That is safe for cgo and for all //line-free code, but a generated file whose //line points at a *different* real source file would lose those occurrences. Generalize it: accumulate occurrences on the document of each occurrence's own //line-resolved origin (via a shared path->document map), emitting one document per source file at the end. An occurrence is dropped only when its origin is not a real source document (cgo's _cgo_gotypes.go glue, a yacc .y, a build-cache path) or its range does not fit that source. - Paths are compared symlink-resolved (CleanResolve), so a module reached through a symlinked directory no longer drops real files as "not a GoFile". - InBounds now also rejects negative line/column (a "//line file:N" directive with no column collapses to column 0, i.e. scip -1) and reversed ranges, so routing can never emit a malformed occurrence. - Document owns occurrence/symbol accumulation, bounds checking, and ToScip rendering; the file visitor routes via targetDoc and attaches symbols in Finish. Verified: existing snapshots are byte-identical (no regression); protobuf/normal Go output is byte-identical to upstream; cgo Go->C refs still anchor to the real .go; a //line-to-another-real-.go file now routes there; malformed (negative column) occurrences are dropped rather than emitted. InBounds unit-tested. --- internal/document/document.go | 87 +++++++++++++++++++++ internal/document/inbounds_test.go | 56 ++++++++++++++ internal/index/scip.go | 28 +++---- internal/visitors/visitor_file.go | 119 ++++++++++------------------- internal/visitors/visitors.go | 32 +++++--- 5 files changed, 221 insertions(+), 101 deletions(-) create mode 100644 internal/document/inbounds_test.go 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 50c4b63..747db72 100644 --- a/internal/index/scip.go +++ b/internal/index/scip.go @@ -125,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() { @@ -144,26 +143,23 @@ func Index(writer func(proto.Message) error, opts config.IndexOpts) error { 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, - origin, + 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) @@ -172,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 diff --git a/internal/visitors/visitor_file.go b/internal/visitors/visitor_file.go index 3d5db79..02ebad4 100644 --- a/internal/visitors/visitor_file.go +++ b/internal/visitors/visitor_file.go @@ -6,9 +6,6 @@ import ( "go/token" "go/types" "log/slog" - "os" - "path/filepath" - "strings" "github.com/scip-code/scip-go/internal/document" "github.com/scip-code/scip-go/internal/lookup" @@ -24,7 +21,7 @@ func NewFileVisitor( file *ast.File, pkgSymbols *lookup.Package, globalSymbols *lookup.Global, - originPath string, + docs map[string]*document.Document, ) *fileVisitor { caseClauses := map[token.Pos]types.Object{} for implicit, obj := range pkg.TypesInfo.Implicits { @@ -33,66 +30,40 @@ 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, - originPath: originPath, - originLineLen: loadLineLengths(originPath), locals: map[token.Pos]lookup.Local{}, pkgSymbols: pkgSymbols, globalSymbols: globalSymbols, - occurrences: occurrences, caseClauses: caseClauses, } } -// 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 (e.g. `defer C.f(x)`) and inserts -// glue whose //line-adjusted position lands past the original line/EOF; such -// occurrences 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 -} - // fileVisitor visits an entire file, but it must be called // after StructVisitor. // // 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 - // originPath is the cleaned, //line-adjusted source file this document - // represents. Occurrences whose adjusted position resolves elsewhere (cgo - // glue, compiler-inserted thunks with no //line) are dropped rather than - // mis-attributed to this file. See visitors.OriginFile. - originPath string - - // originLineLen holds the byte length of each line of originPath (0-indexed), - // or nil if it couldn't be read. Used to drop occurrences whose //line range - // falls outside the real source. See loadLineLengths. - originLineLen []int - // local definition position to symbol and its type information locals map[token.Pos]lookup.Local @@ -102,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 @@ -322,27 +290,20 @@ func (v *fileVisitor) emitImportReference( v.newReference(position, sym, symbols.RangeFromName(position, importedPackage.PkgPath, true), false) } -// inOrigin reports whether an occurrence at pos with range rng belongs to, and -// fits within, this document's source file. cgo (and other //line-annotated -// generated code) can place occurrences at positions that resolve to a -// different generated file, or to a line/column past the real source (e.g. a -// rewritten `defer C.f(x)` or an inserted `_cgoCheckPointer`). Such occurrences -// cannot be faithfully represented and are dropped rather than emitted with an -// out-of-bounds range (which downstream SCIP consumers reject). -func (v *fileVisitor) inOrigin(pos token.Position, rng scip.Range) bool { - if filepath.Clean(pos.Filename) != v.originPath { - return false - } - // Fall back to the filename check alone if the origin couldn't be read. - if v.originLineLen == nil { - return true - } - sl, el := int(rng.Start.Line), int(rng.End.Line) - if sl < 0 || sl >= len(v.originLineLen) || el < 0 || el >= len(v.originLineLen) { - return 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 int(rng.Start.Character) <= v.originLineLen[sl] && - int(rng.End.Character) <= v.originLineLen[el] + return doc } // newDefinition emits a scip.Occurence ONLY. This will not emit a @@ -350,7 +311,8 @@ func (v *fileVisitor) inOrigin(pos token.Position, rng scip.Range) bool { func (v *fileVisitor) newDefinition( pos token.Position, symbol string, rng scip.Range, enclRng *scip.Range, deprecated bool, ) { - if !v.inOrigin(pos, rng) { + doc := v.targetDoc(pos, rng) + if doc == nil { return } occ := &scip.Occurrence{ @@ -358,19 +320,22 @@ func (v *fileVisitor) newDefinition( 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( pos token.Position, symbol string, rng scip.Range, deprecated bool, ) { - if !v.inOrigin(pos, rng) { + doc := v.targetDoc(pos, rng) + if doc == nil { return } occ := &scip.Occurrence{ @@ -381,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) @@ -415,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 3de2be2..f7b2b33 100644 --- a/internal/visitors/visitors.go +++ b/internal/visitors/visitors.go @@ -26,17 +26,31 @@ import ( // 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 filepath.Clean(pkg.Fset.Position(pos).Filename) + return CleanResolve(pkg.Fset.Position(pos).Filename) } -// RealGoFiles is the set of a package's on-disk source files (cleaned paths). -// Occurrences whose OriginFile is not in this set come from generated glue with -// no real source (e.g. cgo's `_cgo_gotypes.go`, or compiler-inserted thunks -// lacking a `//line`), and are dropped rather than mis-attributed. +// 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[filepath.Clean(f)] = struct{}{} + set[CleanResolve(f)] = struct{}{} } return set } @@ -59,7 +73,7 @@ func VisitPackageSyntax( // 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) + doc := visitSyntax(pkg, pkgSymbols, f, relative, origin) if _, ok := goFiles[origin]; ok { pathToDocuments[origin] = doc } @@ -68,8 +82,8 @@ func VisitPackageSyntax( 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