Skip to content

Fix step-usage CodeLens rendering below the method instead of above the attribute - #484

Merged
clrudolphi merged 3 commits into
masterfrom
issue-471-stepcodelens-attribute-line
Aug 25, 2026
Merged

Fix step-usage CodeLens rendering below the method instead of above the attribute#484
clrudolphi merged 3 commits into
masterfrom
issue-471-stepcodelens-attribute-line

Conversation

@clrudolphi

Copy link
Copy Markdown
Collaborator

Summary

Regression found while testing master in VS Code after the #471 performance-fix stack landed: the step-usage CodeLens for a binding now renders after the method declaration instead of above it alongside the built-in "N references" lens.

Root cause: StepCodeLensHandler positions its lens using SourceLocation.SourceFileLine — the method identifier's line for Roslyn-discovered bindings, or a PDB sequence-point line (often a line or more into the method body) for connector-discovered ones. Neither is the attribute's own line. A separate, AST-backfilled AttributeSourceLine already exists on ProjectStepDefinitionBinding and is used elsewhere (BindingLocationMatcher, RenameBindingResolver, CSharpAttributeLiteralResolver), but this handler never adopted it.

This was masked as long as every .cs didOpen always re-ran the Roslyn parser (whose method-identifier line reads close enough to "right"). Item 7 in #478 started skipping that redundant reparse once the connector had already succeeded — a genuine performance win — but the side effect is that the registry now keeps the connector's less precise PDB-derived location instead, and the imprecision that was always latent in the connector path became visible.

Fix

Prefer binding.AttributeSourceLine when known (both discovery paths backfill it), falling back to the existing SourceFileLine/Column for the rare case where the backfill itself failed. Also fixes a stale doc comment claiming AttributeSourceLine is always null for connector-discovered bindings — it's been backfilled via a Roslyn re-parse in ConnectorDiscoveryService for a while.

Test plan

  • New regression test: Handle_prefers_AttributeSourceLine_over_the_method_identifier_or_PDB_line
  • dotnet build — 0 warnings, 0 errors
  • Reqnroll.IdeSupport.LSP.Server.Tests — 831 passed (830 baseline + 1 new)
  • Reqnroll.IdeSupport.LSP.Core.Tests — 658 passed
  • Reqnroll.IdeSupport.LSP.Server.Specs — 165 passed
  • Live verification in VS Code — outstanding

🤖 Generated with Claude Code

…he attribute

StepCodeLensHandler positioned its lens using SourceLocation.SourceFileLine,
which is the method identifier's line for Roslyn-discovered bindings, or a
PDB sequence-point line (often a line or more into the method body) for
connector-discovered ones -- never the attribute's own line. A separate,
AST-backfilled AttributeSourceLine already exists and is used by
BindingLocationMatcher/RenameBindingResolver/CSharpAttributeLiteralResolver,
but this handler never adopted it.

This was masked as long as every .cs didOpen always re-ran the Roslyn
parser (whose method-identifier line reads close enough to "right"). Once
item 7 (#478) started skipping that redundant reparse when the connector
had already succeeded, the registry kept the connector's less precise
PDB-derived location instead, and the lens visibly moved to a line at or
after the method declaration.

Fix: prefer binding.AttributeSourceLine when known, falling back to the
existing SourceFileLine/Column for bindings where the backfill itself
failed. Also corrects a stale doc comment claiming AttributeSourceLine is
always null for connector-discovered bindings -- it's been backfilled via
a Roslyn re-parse in ConnectorDiscoveryService for a while.
…ribute

The previous commit anchored the lens on binding.AttributeSourceLine, which
rendered correctly in Visual Studio but one line too high in VS Code:
VS Code's CodeLens always renders as a floating row above its anchor line
rather than overlaid on it, so anchoring on the attribute's own line pushed
the visual position a line further up than intended.

The conventional CodeLens anchor for a "N references"-style lens -- the one
every client (VS Code, VS, Rider) already uses for the built-in C#
references lens -- is the method declaration's own line, not the
attribute's. That's exactly what SourceLocation.SourceFileLine already
means for Roslyn-discovered bindings; it was only ever imprecise for
connector-discovered ones, whose wire-format location is a raw PDB
sequence point that can land a line or more into the method body.

Fix the actual imprecision instead of routing around it: ConnectorDiscoveryService
now backfills the exact AST-based method-identifier location for
connector-discovered bindings too (BindingImporter.TryGetMethodIdentifierLocation),
mirroring what StepDefinitionFileParser already does for Roslyn discovery.
StepCodeLensHandler goes back to anchoring on SourceLocation.SourceFileLine
directly, which is now precise for both discovery paths.
@clrudolphi

Copy link
Copy Markdown
Collaborator Author

Update: the attribute-line fix (first commit) rendered correctly in Visual Studio but one line too high in VS Code, per live feedback — VS Code's CodeLens always renders as a floating row above its anchor line, while VS apparently overlays it on the target line directly. Anchoring on the attribute's own line therefore works for VS but overshoots by one line in VS Code.

Pushed a second commit that takes the more robust route: instead of picking a different anchor line to work around each client's rendering convention, it fixes the actual imprecision at the source. SourceLocation.SourceFileLine is the conventional CodeLens anchor (the method declaration's own line — same as the built-in C# references lens every client already renders this way for) and was already precise for Roslyn-discovered bindings; it was only ever imprecise for connector-discovered ones, whose wire-format location is a raw PDB sequence point that can land inside the method body. ConnectorDiscoveryService now backfills the exact AST-based method-identifier location for those too, matching what Roslyn discovery already does — so StepCodeLensHandler goes back to anchoring on SourceFileLine directly, precise for both paths now, and consistent with client CodeLens conventions instead of fighting them.

All three test suites green: LSP.Core.Tests 660, LSP.Server.Tests 830, LSP.Server.Specs 147.

Unit-level (BindingImporter.ImportStepDefinition applies the override) and
integration-level (ConnectorDiscoveryService.RunDiscovery replaces a
deliberately-wrong PDB line with the AST-derived method-identifier line)
tests confirming the fix from the previous commit actually takes effect
end-to-end, not just that the AST lookup itself returns the right answer
in isolation.
@clrudolphi
clrudolphi merged commit 066d1fe into master Aug 25, 2026
17 checks passed
@clrudolphi
clrudolphi deleted the issue-471-stepcodelens-attribute-line branch August 25, 2026 20:27
clrudolphi added a commit that referenced this pull request Aug 27, 2026
…matched real wire data

ConnectorDiscoveryService.BuildRegistry backfills the AST-precise method-identifier
location for connector-discovered bindings (issue #484), replacing the connector's raw
PDB sequence-point location (which lands inside the method body, not on the declaration
line -- the "renders below the method" symptom).

That backfill compared sd.Method against Roslyn's bare MethodDeclarationSyntax.Identifier.Text,
but DiscoveryResultTransformer.GetMethodReference (connector side) actually emits
"{DeclaringTypeName}.{MethodName}({ParamTypeNames})", e.g. "Steps.SetFirstNumber(Int32)" --
never a bare name. The comparison could never match on real data, so every
connector-discovered binding silently fell back to the imprecise PDB line. #484's own
regression test passed only because its fixture used an artificially bare Method value
that doesn't occur in production, masking the bug (the PR's "Live verification in VS Code"
checklist item was also left unchecked).

- Add BindingImporter.ExtractBareMethodName to strip the wire-format reference down to
  the bare identifier before the name comparison, with pass-through for an already-bare
  name so existing callers/fixtures are unaffected.
- Add logging (verbose/warning) at each point the backfill can fail, so a stuck lens can
  be diagnosed from the server log going forward.
- Rewrite the demonstrating regression test to assert the fix instead of the bug; add
  direct unit tests for ExtractBareMethodName.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant