Remove SemanticTokenService's duplicate linear-scan position resolver (issue #471) - #482
Merged
clrudolphi merged 23 commits intoAug 25, 2026
Merged
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Implement GetSemanticTokensForRangeAsync() with range-based filtering to reduce encoding cost for textDocument/semanticTokens/range requests. The method filters tags to the requested line range BEFORE encoding, avoiding the previous no-op optimization that computed the entire document. - Add ISemanticTokenService.GetSemanticTokensForRangeAsync interface method - Implement FilterToLineRange() helper to exclude out-of-range tags early - Modify Encode() to accept optional line-range parameters - Handle LSP range semantics (end position at column 0 is exclusive) Test: GetSemanticTokensForRangeAsync_excludes_tags_outside_the_requested_line_range Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ng (issue #471) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
#471) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Pass the requested range's line bounds to IGherkinInlayHintService.Build instead of building hints for the whole document and filtering afterward. This reduces the candidate set processed by the handler, improving performance on large solutions. The existing output filter remains in place for correctness at the range's edges, since Build's line-range check is coarser than the position check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add CodeLensResolveHandler, routing a resolving lens to whichever CodeLens handler produced it based on the "kind" discriminator in CodeLens.Data (stepUsage -> StepCodeLensHandler, hookMatchCount -> HookMatchCountCodeLensHandler, anything else returned unchanged). Register it for DI and wire the codeLens/resolve route right after the existing textDocument/codeLens registration. Also teach the LSP.Server.Specs harness to follow up with codeLens/resolve when a lens comes back with Command unset (the deferred-resolve path landed in the two prior tasks, but no spec exercised it end-to-end until this dispatcher existed) -- without this, the default (non-VS) harness profile now used by StepCodeLens.feature's Background left three scenarios asserting directly on the placeholder lens's null Command. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Final-review fix wave for the issue #471 range/resolve branch. The deferred codeLens path shipped gated on `!IsVisualStudio`, i.e. active for exactly the two clients that cannot handle it. VS Code's stepCodeLens.ts has no resolveCodeLens and discards lens.data; Rider's StepUsagesCodeVisionProvider.kt filters out command == null lenses. Confirmed live in VS Code during Task 9: step-usage lenses stopped rendering and hook-match lenses degraded. Replace it with ClientIdeContext.SupportsCodeLensResolve, backed by a deliberately empty opt-in allowlist, so every shipped client gets today's eager behaviour. ResolveAsync, CodeLensResolveHandler, the codeLens/resolve route and resolveProvider = true are all kept intact and tested, ready to switch on. Also in this wave: - SemanticTokenService: FilterToLineRange now resolves each tag position once and hands it to CollectLeafTokens, instead of both resolving independently (the filter had been a net add of ResolvePosition calls). Token mapping moved ahead of resolution and the end position short-circuits, so the range path is now strictly cheaper than before the filter existed. - HookMatchCountCodeLensHandler: malformed-Data/hook-not-found resolve fallback no longer emits a clickable goToMatchingScenarios command built from a fabricated file:///unknown URI. - StepCodeLensHandler.WithZeroUsages: drop three unused parameters. - Range semanticTokens responses get a ResultId distinguishable from the full-document one. - InlayHintHandler applies the same exclusive-end adjustment as SemanticTokenService before scoping Build. - Comment/doc corrections in Program.cs, ISemanticTokenService and the Specs resolve helper; field alignment and hoisted-gate style consistency. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Declaring codeLensProvider statically (90a9701) woke up vscode-languageclient's default CodeLensFeature alongside the extension's own hand-rolled providers (registerStepCodeLens, registerHookCodeLens), which register against the same document selectors. Both then independently request and render lenses, doubling every hook-match and step-usage lens in the editor. Add middleware that swallows provideCodeLenses/resolveCodeLens before they reach the built-in feature, restoring the hand-rolled providers as the sole source of lenses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Visual Studio's built-in LSP client can't map Reqnroll's custom token types to a classifier of its own -- tokens for it flow entirely through the separate reqnroll/semanticTokens push mechanism (SemanticTokensPushHandler) plus the VS extension's SemanticTokensClassificationInterceptor. Advertising full/range pull support to VS anyway doesn't help it and isn't free: its built-in client has been observed issuing its own pull requests, duplicating the expensive full-document encode the push path already paid for, for a response VS then discards. Withhold Full/Range for VS while still advertising the Legend, since SemanticTokensClassificationInterceptor reads it from the same initialize response to decode pushed tokens -- the push notification itself carries no legend of its own. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI's eslint flagged the `as never` casts on the `next` callback in both tests -- TypeScript already accepts a zero-arg function where a multi-arg one is expected, so the casts were redundant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ex (issue #471) FindUsages was an unindexed full scan over every cached feature document's every step, called once per binding by StepCodeLensHandler -- the dominant cost behind the CodeLens/refresh stalls reported in #471 on large solutions. Adds BindingId, a stable identity hashed from a binding's declaring type, method signature, step block and expression (SHA-256, truncated to 64 bits) rather than its source location -- borrowing clangd's SymbolID shape per the design comment on #471. BindingMatchService now maintains a BindingId -> steps reverse index (O(1) lookup) plus a per-file, binary-searched location index used only to translate a raw SourceLocation into a BindingId for callers that don't already hold a binding object. Shard-precise eviction on invalidate falls out of FeatureBindingMatchSet.Steps already being the shard. At the real repro's scale (~1,300 bindings/file, Reqnroll.VeryLargeFeature) binary search measures ~40x faster than a linear per-file scan, so the location index's write path batches per-file resorts once per Store() call rather than once per step to avoid reintroducing quadratic write cost on a large feature file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…sue #471) FindUnusedStepDefinitionsService, CompletionHandler and StepCodeLensHandler's eager textDocument/codeLens path all already had a ProjectStepDefinitionBinding in hand and only extracted its SourceLocation to call FindUsages -- switch them to the new BindingId overload to skip location math and the per-location cache/ post-hoc expression filter FindUnusedStepDefinitionsService needed under the old location-keyed lookup (BindingId is already per-expression-specific). StepCodeLensHandler.ResolveAsync now prefers a BindingId stashed in the lens's Data payload at creation time, falling back to the SourceLocation-based path only for a payload that predates this field. StepRenameHandler, StepReferencesHandler and FindStepUsagesHandler are unchanged -- they only have a raw cursor position, not a binding object, and get the index's speedup transparently through the unchanged SourceLocation overload. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… (issue #471) RefreshDebouncer.Schedule cancels a pending, not-yet-started run when a newer MatchCacheChangedNotification supersedes it, but SemanticTokensRefreshHandler, InlayHintRefreshHandler, and CodeLensRefreshHandler (via CodeLensRefreshRequester) all sent their actual SendRequest with CancellationToken.None instead of the token they were given -- so once a refresh request was dispatched, no later notification could cancel or collapse it. Confirmed live: three workspace/semanticTokens/refresh requests dispatched 5-10s apart, all still pending simultaneously, resolving together in the same ~2.5s window at the end. Pass the debouncer-supplied token through in all three handlers so a superseding trigger can actually abort a refresh already in flight, not just skip one that hadn't started yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…iliation (issue #471) BindingRegistryChangedHandler.Handle already sequences a full replacement correctly in one method -- RediscoverCsFilesAsync (Roslyn overlay) runs before ReparseOpenFilesAsync (feature-file reparse + MatchCacheChanged publish), so the feature-file reparse already waits for the C# overlay. But RediscoverCsFilesAsync reconciles each .cs file via the same UpdateFromSourceForProjectAsync path used for a live single-file edit, whose ApplyRoslynFileUpdateAsync unconditionally raised its own independent BindingRegistryChanged(false) whenever the Roslyn-parsed result differed from the connector's compiled-DLL result -- which it essentially always does, since reflection-based and Roslyn-source-based extraction of the same file aren't guaranteed byte-identical even with zero real edits. That fired a second, fully independent Handle() invocation that reparsed the same feature files again, redundant with the reparse the original invocation was already about to do moments later. Add a notify parameter (default true, preserving existing behaviour for live edits and file deletions) threaded from ApplyRoslynFileUpdateAsync through UpdateFromSourceForProjectAsync, and pass notify: false from RediscoverCsFilesAsync specifically -- its caller already reparses and notifies unconditionally right after it returns, so a second independent notification only duplicates that work. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ed (issue #471) textDocument/didOpen on a .cs file unconditionally reparsed and patched the registry from source, with no check for whether the project's connector discovery had already run. Confirmed live: a step-definitions file opened at startup got a full Roslyn parse, then the exact same unedited file was reconciled again by RediscoverCsFilesAsync 7 seconds later -- parsed twice for zero new information. This can't be removed outright: RunDiscoveryAsync skips firing BindingRegistryChanged when the connector's assembly hash doesn't change (e.g. no compiled DLL exists yet -- an unbuilt project), so RediscoverCsFilesAsync never runs for such a project either, leaving didOpen's parse as the only mechanism that ever populates bindings from source. VS Code hits this state routinely by design -- its extension only ever runs a design-time MSBuild evaluation, never an actual build. Add ConnectorBindingRegistryProvider.HasSuccessfulConnectorRun, tracking specifically whether the out-of-process connector has ever loaded real bindings from a compiled DLL (deliberately narrower than "Current is populated", since a Roslyn per-file patch can populate Current too). Gate the didOpen registry-update in CSharpBindingDiscoveryService.UpdateFromSourceAsync on this flag when isOpen is true; didChange (an edit) always still applies. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…issue #471) ResolveOffset scanned from line 0 on every offset-to-(line,character) resolution, called from ToLspRange()/StartLinePosition/EndLinePosition across 20 files (document outline, folding, inlay hints, semantic tokens, rename, find-usages, diagnostics). On large files this made position resolution the dominant cost -- confirmed live at 6.6s for a single reqnroll/documentSymbolHierarchical call on an 18k-line feature file. Lines are already produced in increasing Start/End order, so a binary search finds the same line in O(log n) instead of O(n), with the same out-of-bounds clamping behavior as the original scan.
…issue #471) OmniSharp's dispatcher funnels every [Serial]-tagged notification (didOpen/didChange/didSave) through one shared global FIFO lane for the whole server, and an in-flight Serial item also blocks new [Parallel] requests from starting. Awaiting the Gherkin/Roslyn parse inline inside TextDocumentSyncHandler occupied that lane for the parse's full duration. Add IFeatureParseCoordinator: didOpen/didChange now hand their parse off to a per-URI chained background task and return immediately, freeing the lane. FoldingRangeHandler and DocumentSymbolHandler -- the two pull-based handlers with no LSP refresh capability, so a stale read could never self-correct -- await the coordinator before reading buffer.Tags, preserving the correctness the synchronous design provided by accident. Also route BindingRegistryChangedHandler.ReparseOpenFilesAsync's per-buffer reparse through the same coordinator: that cascade is already reached via a detached, unawaited path (BindingRegistryProviderRouter.OnProviderChanged), so the same foldingRange/documentSymbol race already existed for .cs-triggered reparses of open feature files. Routing both trigger paths through one coordinator closes it for both.
ProjectBindingRegistry.MatchSingleContextResult tried every binding's regex against every step (O(steps x bindings)), regardless of authoring style -- Cucumber Expression, plain regex, and method-name bindings all reduce to a compiled Regex by discovery time, so this loop is the same cost class no matter how a binding was written. Add StepLiteralIndex: extracts each binding's statically-known literal text (segments of its compiled regex with zero regex syntax, tracking group/class/quantifier nesting depth so a segment's operators can't leak into an adjacent one) and indexes them in a hand-rolled Aho-Corasick automaton. GetCandidates runs one scan over a step's text and returns only bindings whose full literal requirement was found, plus every binding with no extractable literal (unfiltered, same behavior as today). Cached per registry instance via a ConditionalWeakTable keyed by instance, since ProjectBindingRegistry is an immutable record whose synthesized equality would otherwise be broken by an instance field holding a lazily-built index. Caught two real soundness bugs against the existing regression suite while building this: reusing RegexStepDefinitionExpressionAnalyzer (built for completions, not tracked group depth) let a non-capturing group's closing paren leak into trailing text as if literal when it wrapped nested capturing groups -- replaced with a purpose-built, depth-tracking extractor. And case-insensitive bindings (RegexOptions via inline (?i)) need literal matching folded to a consistent case, or a capitalized literal in the pattern would never be found in lowercase step text -- both are now covered by dedicated tests.
… (issue #471) SemanticTokenService.ResolvePosition was an independent O(document line count) linear scan, written separately from and never consolidated with GherkinRange.ResolveOffset's identical bug (fixed via binary search in #479). Since tag.Range is already a GherkinRange, and StartLinePosition/ EndLinePosition are public properties wrapping the fixed ResolveOffset, the duplicate was never structurally necessary. Delete it and call those properties directly at both call sites. Confirmed live in the #480 experimental session: reqnroll/semanticTokens and workspace/inlayHint/refresh stayed at 7.5-9.5s even after #479 fixed documentSymbolHierarchical, because this second copy of the bug was never touched by that fix.
clrudolphi
changed the base branch from
issue-471-step-literal-prefilter
to
master
August 25, 2026 19:22
…oken-resolveposition-dedup # Conflicts: # src/LSP/Reqnroll.IdeSupport.LSP.Server/Features/SemanticTokens/SemanticTokenService.cs
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.
Summary
Item 10 from #471:
SemanticTokenService.ResolvePositionwas an independent, hand-written O(document line count) linear scan — the same bugGherkinRange.ResolveOffsethad before #479 fixed it with a binary search, just never consolidated into one place.The duplication wasn't structurally necessary.
tag.Range(passed intoResolvePositionat every call site) is already aGherkinRange, andGherkinRange.StartLinePosition/EndLinePositionarepublicinstance properties that already wrap the fixedResolveOffset. So instead of porting the binary search a second time, this deletesSemanticTokenService.ResolvePositionentirely and calls those two properties directly at its two call sites (ResolveTokenTags,FilterToLineRange).Why this matters
Live-verified against
Reqnroll.VeryLargeFeaturein the #479/#480/#481 sessions:reqnroll/semanticTokensandworkspace/inlayHint/refreshstayed at 7.5–9.5s even after #479 shorteneddocumentSymbolHierarchicalfrom 6.6s to <52ms, because this second copy of the linear-scan bug was on a code path #479 never touched.git blametraces it tofb2f2add9(2026-05-25), written independently ofGherkinRange's own copy.Test plan
dotnet build— 0 warnings, 0 errorsReqnroll.IdeSupport.LSP.Server.Tests— 830 passed (same count as baseline, no regressions)Reqnroll.IdeSupport.LSP.Server.Specs— 147 passed / 18 skipped (same as baseline)🤖 Generated with Claude Code