Add a literal prefilter for step-definition matching (issue #471) - #481
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.
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
Regexby discovery time, on both the connector (real Reqnroll runtime, viaReqnroll.Bindings.Provider.BindingProviderService) and Roslyn (in-process, ports Reqnroll's ownStepDefinitionRegexCalculator) discovery paths. See issue #471, item 3.ProjectBindingRegistry.MatchSingleContextResulttries every binding's regex against every step (StepDefinitions.Select(sd => sd.Match(...))) — O(steps × bindings), the likely dominant cost behindinternal/bindingRegistryReconcile's ~10s onReqnroll.VeryLargeFeature's scale (1,000+ bindings per file).StepDefinitionFileParser's local port of Reqnroll's regex calculator).What changed
StepLiteralIndex: extracts each binding's statically-known literal text from its compiled regex (not the pre-compilation expression — an earlier design that split on{param}placeholders got Cucumber Expression alternation/optional-text wrong, since neither is literal) via a purpose-built depth-tracking scanner, and indexes the union in a hand-rolled Aho-Corasick automaton.GetCandidates(stepText)runs one scan and returns only bindings whose full literal requirement was found, plus every binding with no extractable literal (unfiltered — same as today, just no speedup for it).ProjectBindingRegistry.MatchSingleContextResultcallsLiteralIndex.GetCandidates(stepText)instead of iterating the fullStepDefinitionsarray. Cached per registry instance via aConditionalWeakTable(arecord's synthesized equality would otherwise be silently broken by an instance field holding a lazily-built index).Two real soundness bugs caught building this — not simulated, found by the existing regression suite
RegexStepDefinitionExpressionAnalyzer(built for completions sampling, not this) doesn't track group nesting depth — a non-capturing group wrapping nested capturing groups ((?:(cool)|(bad)), exactly what Cucumber Expression alternation compiles to when combined with another parameter) left its closing paren unaccounted for, silently absorbed into trailing text as literal. Caught byProjectBindingRegistryMatchTests.Matches_parameters_with_multiple_capture_groups_used_by_cukeex_string_param. Fixed by replacing the reused analyzer with a purpose-built, depth-tracking extractor scoped to this correctness-critical use.(?i)inline modifier) need literal matching folded to a consistent case — a capitalized literal like"First"in the pattern would never be found via ordinal matching against lowercase step text. Caught byProjectBindingRegistryAmbiguousTests.Error_shows_display_expression_not_raw_regex_for_method_name_style_bindings. Fixed by lowercasing both literals and step text (case-insensitive only ever widens the candidate set, so it's safe even for genuinely case-sensitive bindings).Both are now covered by dedicated
StepLiteralIndexTestsin addition to the existing tests that caught them.Test plan
StepLiteralIndexTests(14 tests): narrowing, no-false-negative soundness for wildcard/method-name/short-literal bindings, alternation and optional-text (both slash-syntax and plain-regex(foo|bar)), the nested-non-capturing-group regression, the case-insensitivity regression, shared-literal bindings, empty registry, null-regex bindingdotnet testonReqnroll.IdeSupport.LSP.Core.Tests— 657 passed (643 baseline + 14 new), 1 pre-existing skip, zero changes to any existing test — the prefilter is behavior-preservingdotnet testonReqnroll.IdeSupport.LSP.Server.Tests— 830 passed, no regressionsdotnet testonReqnroll.IdeSupport.LSP.Server.Specs— 147 passed, 18 pre-existing environment skips, no regressionsLive verification of the actual
internal/bindingRegistryReconcilelatency improvement onReqnroll.VeryLargeFeaturestill outstanding.