feat(trace): wire up --mode dispatch and ship tree-sitter by default - #260
Open
kryptt wants to merge 3 commits into
Open
feat(trace): wire up --mode dispatch and ship tree-sitter by default#260kryptt wants to merge 3 commits into
kryptt wants to merge 3 commits into
Conversation
Pre-existing bug: the `treesitter` build tag gated all tree-sitter code, so stock `go build` produced a binary that could only do regex symbol extraction. The `--mode fast`/`--mode precise` flag was a cosmetic label on TraceResult.Mode that never actually selected an extractor — both code paths called `trace.NewRegexExtractor()` regardless. F# (yoanbernabeu#152) landed via tree-sitter but the dispatch to it was dead. This change ships tree-sitter in the default build and makes the flag mean what it says. * Drop `//go:build treesitter` from extractor_ts.go and its tests. `go build` now compiles grammars for Go, JS, JSX, TS, TSX, Python, PHP, C#, F# always. Binary grows from ~23 MB to ~48 MB. * Introduce `trace.CompoundExtractor`, which implements `SymbolExtractor` by routing per-file between the tree-sitter and regex extractors according to a Mode (auto / fast / precise). Tests in compound_test.go cover the dispatch matrix. * Add `trace.ParseMode` (normalizes user input; unknown values fall back to auto with a one-line warning to stderr) and `trace/languages.go` (`HasTreeSitterGrammar`, `TreeSitterExtensions` — single source of truth for which extensions have a compiled-in grammar). * `cli/watch.go` (both single-project and workspace runners) replaces `trace.NewRegexExtractor()` with `trace.NewCompoundExtractor(...)`. The relevant function signatures change `*trace.RegexExtractor` to `trace.SymbolExtractor` to widen to the interface. * `cli/trace.go` default for `--mode` flips from `fast` to `auto`; help text expanded to describe all three modes. * `config.TraceConfig.Mode` default flips from `fast` to `auto`. No behavior change for the 9 already-supported languages at the symbol level — they go from regex-quality to tree-sitter-quality, which is strictly more accurate (e.g. Go method receivers, Python class docstrings) without removing anything regex caught. Other extensions keep their existing regex behavior. Refs: extends yoanbernabeu#152 (F#) and yoanbernabeu#176 (Lua fast-mode) by making the dispatch actually live.
Five small follow-ups from a self-review of the previous commit:
* Consolidate the tree-sitter language registry. The extension list in
languages.go and the extension→grammar map inside NewTreeSitterExtractor
were two sources of truth that could drift. Merge into a single
treeSitterLanguages map[string]func() *sitter.Language at the package
level in languages.go. HasTreeSitterGrammar, TreeSitterExtensions, and
NewTreeSitterExtractor all derive from it. Adding a new language is
now one entry in that map plus the switch arm in walkNodeForSymbols.
* Lazy tree-sitter init. NewCompoundExtractor no longer constructs the
TreeSitterExtractor up front. ModeFast users (and ModeAuto users who
only touch regex-only extensions) never pay the parser-allocation
cost. Construction is deferred to the first file that needs it.
* Drop the (always-nil) error from NewCompoundExtractor's return
signature. The error claim was misleading; the only possible failure
mode (lazy TS init) now surfaces from per-file route() calls.
* Make ParseMode a pure normalizer. It now returns (Mode, ok bool)
instead of writing the "unknown value" warning to os.Stderr itself.
The CLI call sites in cli/watch.go log the warning where the I/O
concern belongs and where the project name is in scope (helpful for
workspace mode).
* Library-friendly error message. The route() error no longer leaks
the "--mode precise:" CLI flag name. Replaced with "trace: precise
mode requires a tree-sitter grammar for extension %q (file %s); none
compiled in" so downstream library consumers see a meaningful message.
Also: expanded compound_test.go with TestCompound_FastMode_NoTreeSitterAllocation,
TestCompound_AutoMode_LazyTreeSitter, TestHasTreeSitterGrammar_FromRegistry,
and TestTreeSitterExtensions_SortedAndDerived to lock in these invariants.
Test fixture switched from .pas to a more durable .lua/.rs pair.
No external behavior change. Same 48 MB binary, same dispatch behavior,
just less duplication and a smaller surface to maintain when PR 2 lands
the language additions.
A focused duplication-detection pass with `dupl -t 50` surfaced two
clone groups introduced by the previous commits. Both are now
collapsed:
* cli/watch.go:1006-1010 vs cli/watch.go:2769-2773 — the two
NewCompoundExtractor call sites were independently doing
ParseMode → log warning on unknown value → NewCompoundExtractor.
Factored into buildSymbolExtractor(rawMode, context), which keeps
the warning helpful (mentions the project root or workspace
project name + lists valid values) and reduces each call site to
a one-liner. The fact that the workspace site now also lists the
valid values in its warning is a small UX improvement that fell
out of the unification.
* trace/compound_test.go:170-175 vs 258-263 — two copies of the
"is this slice strictly ascending?" check. Factored into a
t.Helper-marked assertStrictlySorted(t, label, got). The strict
inequality is the right contract for both call sites because
both inputs come from set-then-sort and cannot contain duplicates.
Other dupl hits are either pre-existing in upstream code (the
RPG-init blocks in cli/watch.go, the per-language extractGoSymbol /
extractJSSymbol / etc. shapes in trace/extractor_ts.go) or
structurally forced by the SymbolExtractor interface (the route+
delegate body of ExtractSymbols / ExtractReferences / ExtractAll
on CompoundExtractor — different return types preclude a clean
helper without generics). Those stay as-is.
Verified: go vet ./... clean, go test ./... clean,
dupl -t 30 on the three new files reports only the structurally
forced interface-shaped clone in compound.go.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
There are two related bugs in current
mainaround thetreesitterintegration:--modedoes nothing.cli/trace.goexposes--mode fast/--mode precise. The value is stored onTraceResult.Modeand surfaces in JSON output, but no code consults it to pick an extractor. Bothcli/watch.go:1006andcli/watch.go:2765hardcodetrace.NewRegexExtractor();cli/trace.goreads the symbol store built by watch and never instantiates an extractor. The user-visible result: passing--mode preciseproduces the same symbols--mode fastwould.Default users never get tree-sitter. Every
trace/extractor_ts.go, everytrace/extractor_ts_*_test.go, and the only call site throughfsharp/is gated behind//go:build treesitter. Stockgo buildandmake build(no flags, noBUILD_TAGS) produce a binary that lacks the entire tree-sitter path. F# (PR feat(trace): add F# language support with Ionide tree-sitter grammar #152) and the rest of the precise-mode work landed but are dead code in any binary not built with the tag explicitly.This PR fixes both at once.
Solution
1. Tree-sitter is always compiled in
Drop
//go:build treesitterfromtrace/extractor_ts.goand its three test files. The grammars become unconditional dependencies viagithub.com/smacker/go-tree-sitter(already ingo.sum) and the vendoredfsharp/package. Binary grows from ~23 MB to ~48 MB on x86_64-linux. Comparable to other code-search tools (ast-grep,rg).2. New
CompoundExtractordispatches per-mode`trace/compound.go` adds
CompoundExtractor, which implements the existingSymbolExtractorinterface by routing each file betweenTreeSitterExtractorandRegexExtractor:```go
case ModeFast: // -> always RegexExtractor
case ModePrecise: // -> TreeSitterExtractor; error on unsupported extension
default: // ModeAuto: TS if grammar exists, else regex
```
`trace/languages.go` exposes
HasTreeSitterGrammar(ext) boolandTreeSitterExtensions() []stringas the single source of truth for which extensions are tree-sitter-backed in this build. Adding a new tree-sitter grammar in a future PR is one entry in that map plus the parser registration inNewTreeSitterExtractor— the dispatch picks it up automatically.3.
--mode autois the new defaultCLI flag default in
cli/trace.goflips from\"fast\"to\"auto\". The same flip happens inconfig.DefaultConfig().Trace.Mode. The help text now describes all three modes. `trace.ParseMode` normalizes empty/whitespace input to `auto` and emits a one-line stderr warning for unknown values.4. Watch flow uses the compound extractor
Both
runWatch(single-project) and the workspace-mode runner now calltrace.NewCompoundExtractor(trace.ParseMode(cfg.Trace.Mode))instead oftrace.NewRegexExtractor(). Function signatures that took `*trace.RegexExtractor` widen to `trace.SymbolExtractor` — the interface that already exists. No new dependency injection plumbing.What the user sees
.go,.js,.jsx,.ts,.tsx,.py,.php,.cs,.fs,.fsx,.fsiwithout changing config. For everything else, regex behavior is unchanged.Test plan
New `trace/compound_test.go` covers the dispatch matrix:
ExtractSymbolson a sample.gofile producing the expected named symbolsMode()returns the configured string forTraceResult.ModedisplaySupportedLanguages()returns the sorted union of both underlying extractorsThe existing `trace/extractor_ts_csharp_test.go`, `trace/extractor_ts_fsharp_test.go`, and `trace/extractor_ts_docstring_test.go` (previously build-tag-gated) now run unconditionally and continue to pass.
`go test ./...` is clean.
Things to think about during review
make build-minimaltarget that re-introduces the build tag in the opposite direction (default = full; opt-out = small). Happy to do that in a follow-up if you want.indexer/scanner.SupportedExtensions), plus a vendored Wilfredtree-sitter-elispfor.el.Closes nothing directly, but unblocks the latent intent of #152 and #176 (Lua fast-mode) by making the dispatch actually live.