Skip to content

Fix Roslyn step-definition discovery to honor Cucumber Expression alternation and optional text - #486

Merged
clrudolphi merged 3 commits into
masterfrom
fix/476-cucumber-expression-alternation-optional
Aug 26, 2026
Merged

Fix Roslyn step-definition discovery to honor Cucumber Expression alternation and optional text#486
clrudolphi merged 3 commits into
masterfrom
fix/476-cucumber-expression-alternation-optional

Conversation

@clrudolphi

@clrudolphi clrudolphi commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • StepDefinitionFileParser.BuildRegex treated / (alternative text) and () (optional text) as literal characters, so a Roslyn-discovered binding's regex diverged from the connector-discovered regex (computed by Reqnroll's real runtime) for any Cucumber Expression using those constructs.
  • Now delegates to the real Cucumber.CucumberExpressions library (v17.1.0 — the exact version Reqnroll 3.2.0 itself depends on) via its public CucumberExpression class, the same one Reqnroll's runtime uses for this. This is option 2 from the original issue: LSP.Core's regex generation is guaranteed to agree with the connector-discovered path by construction, rather than by two independently-maintained implementations staying in sync. (An earlier revision of this PR hand-rolled the alternation/optional grammar instead — replaced once it became clear taking the dependency was straightforward.)
  • Two small pieces the package itself doesn't provide were ported, not re-derived, from Reqnroll's own runtime source (verified against a local Reqnroll checkout):
    • CucumberExpressionDetector — the plain-regex-vs-Cucumber-Expression heuristic. This lives in the Reqnroll runtime assembly itself, not in Cucumber.CucumberExpressions, and LSP.Core deliberately doesn't reference that assembly so discovery can run pre-build.
    • LspCucumberExpressionParameterTypeRegistry — a minimal IParameterTypeRegistry using the exact same regex fragments as Reqnroll's own registry (ParameterTypeConstants). Reqnroll's real registry builds its parameter-type set by reflecting over already-compiled binding methods, which doesn't exist yet during syntax-only discovery; unknown names (custom [StepArgumentTransformation] types, enums) fall back to a permissive .* match instead of throwing, preserving today's behavior.
  • Hardens BuildRegex against the real grammar's stricter validation (e.g. "an alternative may not be empty") throwing for malformed input the old hand-rolled version silently tolerated — caught and surfaced as an invalid (null-regex) binding rather than aborting discovery of the whole file.
  • The existing "no {param} → treat as plain regex" contract is preserved via CucumberExpressionDetector, so hand-written regex bindings using bare (...) capturing groups keep working exactly as before.

Fixes #476

Test plan

  • Unit tests in StepDefinitionFileParserTests covering: / alternation, () optional text, alternation+optional combined, backslash-escaped syntax, standard parameter types (now asserted by match behavior rather than exact regex text, since the pattern text is the library's own), {word} not matching across a space, a custom/unknown parameter type falling back to wildcard, and a malformed expression degrading to an invalid binding instead of throwing.
  • dotnet test tests/LSP/Reqnroll.IdeSupport.LSP.Core.Tests — 673 passed, 0 failed, 1 skipped (pre-existing skip).
  • Full solution build + full solution dotnet test — all green.

🤖 Generated with Claude Code

clrudolphi and others added 3 commits August 25, 2026 16:25
…ernation and optional text (#476)

BuildRegex previously escaped '/' and '()' as literal text, so a Roslyn-discovered
binding using alternative text (cat/dog) or optional text (eat(s)) never matched what
the connector's runtime-computed regex matched, causing spurious undefined-step
diagnostics that reasserted on every .cs edit. BuildRegex now parses the expression for
whitespace-scoped alternation and nested optional groups (with backslash escaping),
mirroring real Cucumber Expression semantics.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…parser (#476)

Replaces the hand-rolled alternation/optional-text parser added for #476 with the real
Cucumber.CucumberExpressions library (Cucumber.CucumberExpressions v17.1.0, the exact version
Reqnroll 3.2.0 itself depends on) via its public CucumberExpression class -- the same one
Reqnroll's runtime uses for this exact purpose -- so LSP.Core's regex generation is guaranteed to
agree with the connector-discovered path by construction rather than by a second, independently
maintained implementation staying in sync (option 2 from the original issue).

Two small pieces the package doesn't provide were ported (not re-derived) from Reqnroll's own
runtime, verified against a local Reqnroll source checkout:
- CucumberExpressionDetector: the plain-regex vs. Cucumber-Expression heuristic, which lives in
  the Reqnroll runtime assembly itself (not the CucumberExpressions package) -- LSP.Core
  deliberately doesn't reference that assembly so discovery can run pre-build.
- LspCucumberExpressionParameterTypeRegistry: a minimal IParameterTypeRegistry using the exact
  same regex fragments as Reqnroll's own registry (ParameterTypeConstants), since the real
  registry builds its parameter-type set by reflecting over already-compiled binding methods --
  information that doesn't exist yet during syntax-only discovery. Unknown names (custom
  [StepArgumentTransformation] types, enums) fall back to the permissive `.*` match instead of
  throwing, preserving the existing fallback behavior.

Also hardens BuildRegex against the real grammar's stricter validation (e.g. "an alternative may
not be empty") throwing for malformed input the old hand-rolled version silently tolerated: caught
and surfaced as an invalid (null-regex) binding rather than aborting discovery of the whole file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…parameter type

- BuildRegex caught bare Exception, which would silently turn a genuine bug (e.g. an NRE in
  LspCucumberExpressionParameterTypeRegistry) into "just an invalid binding" with no diagnostic
  trail. Narrowed to CucumberExpressionException (the grammar's own validation) and
  ArgumentException (covers RegexParseException from the plain-regex branch) so anything
  unexpected propagates instead of being swallowed.
- LspCucumberExpressionParameterTypeRegistry registered "short" as a known Cucumber parameter
  type, but verified against a local Reqnroll checkout that Reqnroll's real registry only
  aliases int/float/double/byte/long/decimal to their C# keywords -- {short} resolves solely via
  its CLR type name Int16 (RuntimeBindingType.Name => Type.Name), so it's genuinely undefined in
  a real Reqnroll project. Recognizing it here would have falsely validated {short} via Roslyn
  discovery while the connector-discovered path leaves it undefined -- the exact class of
  Roslyn-vs-connector divergence #476 exists to fix, just for a different name. Removed; it now
  falls back to the wildcard match like any other unrecognized type name.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@clrudolphi
clrudolphi merged commit 5bd7803 into master Aug 26, 2026
17 checks passed
@clrudolphi
clrudolphi deleted the fix/476-cucumber-expression-alternation-optional branch August 26, 2026 14:28
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.

Roslyn step-definition discovery ignores Cucumber Expression alternation (/) and optional text (()), diverging from connector-discovered bindings

1 participant