From 744a854b8ecefaf768395263b45fce00a51d5ec5 Mon Sep 17 00:00:00 2001 From: Jacopo Bacchelli Date: Thu, 30 Jul 2026 07:55:56 -0700 Subject: [PATCH 1/3] 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/3] 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 From ea4f543e2fb59554120c0c78bb87f2a0bab4c25e Mon Sep 17 00:00:00 2001 From: Jacopo Bacchelli Date: Tue, 4 Aug 2026 09:49:46 -0700 Subject: [PATCH 3/3] Recover cgo occurrences whose range was widened by name mangling cgo rewrites `C.puts` to `_Cfunc_puts` before the type checker sees it, and an occurrence's range is sized from the identifier the checker reports. Once the `//line` directives map the position back to the original source the start column is right, but the range is five characters too wide and can run past the end of the real line. Those occurrences were rejected as out of bounds even though they were perfectly well located. Give an out-of-bounds range one repair attempt before dropping it: measure the identifier actually present at that column in the origin source and re-emit with that width. The repair is only accepted if the result is in bounds, so it never invents a location -- a synthesized position such as cgo's `defer C.f(x)` wrapper, which lands at end-of-line where there is no identifier, finds nothing to measure and is still dropped. Measured on gravitational/teleport (2767 Go files, 12 cgo packages), same commit and same toolchain, counting `_C[2]func_` references and the Go->C call edges they resolve to: unpatched scip-go 44 refs / 8 edges -- but every one anchored in ~/.cache/go-build, so unusable before this commit 25 refs / 6 edges -- all on real source after this commit 27 refs / 7 edges -- all on real source The two recovered references are `C.gnu_get_libc_version` and `C.resetInterruptSignalHandler`; the second adds a bridged edge in a package that previously contributed none. Multi-line calls are covered too: `C.fill(cs,\n &out,\n C.int(1))` is recovered with a range covering exactly `C.fill`. Two known gaps remain, both distinct from what this fixes: - `C.free` accounts for 16 of the still-missing teleport references. They sit inside `defer C.free(...)`, whose rewritten wrapper reports end-of-line positions with no identifier to measure. They are libc calls with no C definition in the indexed surface, so they never produced edges regardless. - One call site, teleport's 5-argument `C.mygetpwnam_r(...)` spanning several lines with pointer arguments, is still absent. It is not rejected by the bounds check -- no drop is recorded for it -- so it never reaches the occurrence-emitting path at all, and the cause is elsewhere. Reproducible with a `getpwnam_r`-style wrapper called with `&pwd` and an `unsafe.Pointer`-derived cast. A related wart this deliberately does not touch: a mangled range that happens to stay in bounds is kept unrepaired and is therefore too wide -- `C.add` is emitted covering `C.add(C.in`. Re-measuring every range would fix that, but package references are sized from the import path rather than the source token (emitImportReference), so a blanket re-measure would truncate `github.com/foo/bar` to `github`. Fixing it properly needs the range builder to know whether the name it was handed is the source spelling. --- internal/document/document.go | 67 +++++++++++++++++++ internal/document/repair_range_test.go | 91 ++++++++++++++++++++++++++ internal/visitors/visitor_file.go | 39 +++++++---- 3 files changed, 184 insertions(+), 13 deletions(-) create mode 100644 internal/document/repair_range_test.go diff --git a/internal/document/document.go b/internal/document/document.go index 3c8b39a..68c37ab 100644 --- a/internal/document/document.go +++ b/internal/document/document.go @@ -10,6 +10,7 @@ import ( "go/types" "log/slog" "os" + "regexp" "strconv" "strings" "sync" @@ -81,6 +82,12 @@ type Document struct { originAbs string lineLen []int + // lines is the document's source split by line, loaded lazily by lineText: + // only documents that need a range repair pay to keep their source resident. + // linesLoaded distinguishes "not tried yet" from "tried and unreadable". + lines []string + linesLoaded bool + // 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 @@ -117,6 +124,66 @@ func (d *Document) InBounds(r scip.Range) bool { return sc <= d.lineLen[sl] && ec <= d.lineLen[el] } +// cgoSourceName matches the text cgo rewrote into a mangled identifier: a +// `C.foo` selector, or a bare identifier for manglings that drop the prefix. +// Anchored, so it only matches at the column the range starts on. +var cgoSourceName = regexp.MustCompile(`^(?:C\.)?[\p{L}_][\p{L}\p{Nd}_]*`) + +// RepairRange rebuilds an out-of-bounds range from the source text it actually +// covers, returning the replacement and true when one is available. +// +// A range's width comes from the identifier the type checker sees, and cgo's +// identifiers are mangled: `C.puts` is rewritten to `_Cfunc_puts`, so a range +// anchored at the right column is emitted five characters too wide and can spill +// past the end of the real line. The position is fine; only the width is wrong. +// Measuring the identifier that is genuinely at that column recovers the +// occurrence instead of discarding it. +// +// Only a single-line range starting inside real source can be repaired. A +// synthesized position -- cgo's `defer C.f(x)` wrapper, which lands at +// end-of-line where there is no identifier to measure -- matches nothing and is +// still dropped, so this never invents a location. +func (d *Document) RepairRange(r scip.Range) (scip.Range, bool) { + if r.Start.Line != r.End.Line || r.Start.Line < 0 || r.Start.Character < 0 { + return r, false + } + line, ok := d.lineText(int(r.Start.Line)) + if !ok || int(r.Start.Character) >= len(line) { + return r, false + } + name := cgoSourceName.FindString(line[r.Start.Character:]) + if name == "" { + return r, false + } + repaired := scip.Range{ + Start: r.Start, + End: scip.Position{ + Line: r.Start.Line, + Character: r.Start.Character + int32(len(name)), + }, + } + if !d.InBounds(repaired) { + return r, false + } + return repaired, true +} + +// lineText returns 0-indexed line l of this document's source. The source is +// read on first use and cached, so documents that never need a repair keep only +// their line lengths. +func (d *Document) lineText(l int) (string, bool) { + if !d.linesLoaded { + d.linesLoaded = true + if b, err := os.ReadFile(d.originAbs); err == nil { + d.lines = strings.Split(string(b), "\n") + } + } + if l < 0 || l >= len(d.lines) { + return "", false + } + return strings.TrimSuffix(d.lines[l], "\r"), true +} + // 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) { diff --git a/internal/document/repair_range_test.go b/internal/document/repair_range_test.go new file mode 100644 index 0000000..80e7a9b --- /dev/null +++ b/internal/document/repair_range_test.go @@ -0,0 +1,91 @@ +package document + +import ( + "os" + "path/filepath" + "testing" + + "github.com/scip-code/scip/bindings/go/scip" +) + +// docFor writes src to a temp file and returns a Document over it, the way +// NewDocument would. +func docFor(t *testing.T, src string) *Document { + t.Helper() + path := filepath.Join(t.TempDir(), "geometry.go") + if err := os.WriteFile(path, []byte(src), 0o600); err != nil { + t.Fatal(err) + } + return &Document{originAbs: path, lineLen: loadLineLengths(path)} +} + +// The line cgo mangles: `C.puts` is rewritten to `_Cfunc_puts`, so the range is +// sized 11 instead of 6 and runs past the end of the real line. +const putsSrc = "package example\n\nfunc f(cs *C.char) {\n C.puts(cs)\n}\n" + +func TestRepairRangeRecoversMangledCgoName(t *testing.T) { + d := docFor(t, putsSrc) + // Line 3 is " C.puts(cs)" (14 bytes); the mangled range ends at 15. + oob := rng(3, 4, 3, 15) + if d.InBounds(oob) { + t.Fatal("test setup: range should be out of bounds") + } + + got, ok := d.RepairRange(oob) + if !ok { + t.Fatal("RepairRange should recover a mangled cgo name") + } + if want := rng(3, 4, 3, 10); got != want { + t.Errorf("RepairRange = %v, want %v (covering `C.puts`)", got, want) + } + if !d.InBounds(got) { + t.Error("repaired range must be in bounds") + } +} + +func TestRepairRangeRejectsUnmeasurablePositions(t *testing.T) { + d := docFor(t, putsSrc) + + cases := []struct { + name string + r scip.Range + }{ + // cgo's `defer C.f(x)` wrapper lands at end-of-line, where there is no + // identifier to measure -- inventing a range there would be a guess. + {"start at end of line", rng(3, 14, 3, 25)}, + {"start past end of line", rng(3, 40, 3, 51)}, + {"line past EOF", rng(99, 0, 99, 5)}, + {"multi-line range", rng(3, 4, 4, 2)}, + {"negative line", rng(-1, 0, -1, 5)}, + {"negative column", rng(3, -1, 3, 5)}, + // Column 0 of ` C.puts(cs)` is a space: no identifier starts there. + {"start on non-identifier", rng(3, 0, 3, 40)}, + } + for _, tc := range cases { + if _, ok := d.RepairRange(tc.r); ok { + t.Errorf("%s: RepairRange(%v) should not repair", tc.name, tc.r) + } + } +} + +func TestRepairRangeUnreadableSource(t *testing.T) { + // A document whose source can't be read has nothing to measure against. + d := &Document{originAbs: filepath.Join(t.TempDir(), "missing.go")} + if _, ok := d.RepairRange(rng(0, 0, 0, 5)); ok { + t.Error("unreadable source should not repair") + } +} + +func TestRepairRangeHandlesNonASCIIIdentifiers(t *testing.T) { + // Ranges are byte offsets, and Go identifiers may be non-ASCII, so the + // repaired width must be measured in bytes. + d := docFor(t, "package example\n\nvar été = 1\n") + // Line 2 is "var été = 1"; `été` starts at byte 4 and is 5 bytes long. + got, ok := d.RepairRange(rng(2, 4, 2, 99)) + if !ok { + t.Fatal("should repair a non-ASCII identifier") + } + if want := rng(2, 4, 2, 9); got != want { + t.Errorf("RepairRange = %v, want %v", got, want) + } +} diff --git a/internal/visitors/visitor_file.go b/internal/visitors/visitor_file.go index 02ebad4..0eebdc5 100644 --- a/internal/visitors/visitor_file.go +++ b/internal/visitors/visitor_file.go @@ -291,19 +291,32 @@ func (v *fileVisitor) emitImportReference( } // 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 { +// to, along with the range to emit. A nil document means drop the occurrence. +// +// 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) is dropped. +// +// A range that escapes the origin's source gets one repair attempt first: cgo +// widens ranges by mangling identifiers (`C.puts` -> `_Cfunc_puts`), which pushes +// a correctly-positioned occurrence past end-of-line, and Document.RepairRange +// re-measures it against the real source. Only when that fails is the occurrence +// 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, scip.Range) { doc := v.docs[CleanResolve(pos.Filename)] - if doc == nil || !doc.InBounds(rng) { - return nil + if doc == nil { + return nil, rng + } + if doc.InBounds(rng) { + return doc, rng + } + if repaired, ok := doc.RepairRange(rng); ok { + return doc, repaired } - return doc + return nil, rng } // newDefinition emits a scip.Occurence ONLY. This will not emit a @@ -311,7 +324,7 @@ func (v *fileVisitor) targetDoc(pos token.Position, rng scip.Range) *document.Do func (v *fileVisitor) newDefinition( pos token.Position, symbol string, rng scip.Range, enclRng *scip.Range, deprecated bool, ) { - doc := v.targetDoc(pos, rng) + doc, rng := v.targetDoc(pos, rng) if doc == nil { return } @@ -334,7 +347,7 @@ func (v *fileVisitor) newDefinition( func (v *fileVisitor) newReference( pos token.Position, symbol string, rng scip.Range, deprecated bool, ) { - doc := v.targetDoc(pos, rng) + doc, rng := v.targetDoc(pos, rng) if doc == nil { return }