From 8455129b82443d350285b4829b54be7b376deabf Mon Sep 17 00:00:00 2001 From: Chris Rudolphi <1702962+clrudolphi@users.noreply.github.com> Date: Fri, 3 Jul 2026 15:43:44 -0500 Subject: [PATCH] Distinguish ambiguous steps from "not found" in step rename FindBindingsAtFeatureStep skipped ambiguous steps (HasAmbiguous, no single HasDefined match) before ever checking cursor position, so both the VS "Rename Step" command and VS Code's native F2 saw an ambiguous step as if nothing were found at all. - reqnroll/renameTargets now reports IsAmbiguous so RenameStepCommand can show "Rename is not supported for an ambiguously bound step." instead of the misleading "No step definition found" (#34). - textDocument/prepareRename now throws a distinct, deliberately worded error for the ambiguous case instead of the generic "not available" message (#36). Fixes #34, Fixes #36 Co-Authored-By: Claude Sonnet 5 --- .../Features/Rename/RenameTargetsResponse.cs | 9 ++ .../Features/Rename/StepRenameHandler.cs | 38 +++++-- .../RenameStep/RenameStepCommand.cs | 7 ++ .../RenameStep/RenameStepService.cs | 24 +++-- .../RenameStep/RenameTargetsResult.cs | 7 ++ .../Features/Rename/StepRenameHandlerTests.cs | 101 ++++++++++++++++++ .../RenameStep/RenameTargetsMappingTests.cs | 14 +++ 7 files changed, 184 insertions(+), 16 deletions(-) diff --git a/src/LSP/Reqnroll.IdeSupport.LSP.Server/Features/Rename/RenameTargetsResponse.cs b/src/LSP/Reqnroll.IdeSupport.LSP.Server/Features/Rename/RenameTargetsResponse.cs index 4ead236..788751b 100644 --- a/src/LSP/Reqnroll.IdeSupport.LSP.Server/Features/Rename/RenameTargetsResponse.cs +++ b/src/LSP/Reqnroll.IdeSupport.LSP.Server/Features/Rename/RenameTargetsResponse.cs @@ -10,6 +10,15 @@ public sealed class RenameTargetsResponse { [JsonProperty("targets")] public List Targets { get; set; } = new(); + + /// + /// True when is empty because the cursor is on a step that matches + /// more than one binding, none of which resolve to a single definite match — as opposed to + /// no binding being found at all. Lets clients show "rename is not supported for an + /// ambiguous step" instead of a misleading "no step definition found" message. + /// + [JsonProperty("isAmbiguous")] + public bool IsAmbiguous { get; set; } } /// One renameable binding attribute at the queried position. diff --git a/src/LSP/Reqnroll.IdeSupport.LSP.Server/Features/Rename/StepRenameHandler.cs b/src/LSP/Reqnroll.IdeSupport.LSP.Server/Features/Rename/StepRenameHandler.cs index 0b375dc..00cb6d7 100644 --- a/src/LSP/Reqnroll.IdeSupport.LSP.Server/Features/Rename/StepRenameHandler.cs +++ b/src/LSP/Reqnroll.IdeSupport.LSP.Server/Features/Rename/StepRenameHandler.cs @@ -146,9 +146,15 @@ public StepRenameHandler( // subsequent textDocument/rename would fail with "Internal Error". if (path.EndsWith(".feature", StringComparison.OrdinalIgnoreCase)) { - var featureBindings = FindBindingsAtFeatureStep(uri, path, request.Position, out var stepRange); + var featureBindings = FindBindingsAtFeatureStep(uri, path, request.Position, out var stepRange, out var isAmbiguous); if (featureBindings.Count == 0) { + if (isAmbiguous) + { + _logger.LogVerbose("StepRenameHandler: prepareRename — step is ambiguously bound"); + throw new InvalidOperationException("Rename is not supported for an ambiguously bound step."); + } + _logger.LogVerbose("StepRenameHandler: prepareRename — no defined binding at feature step position"); return Task.FromResult(null); } @@ -499,9 +505,9 @@ public StepRenameHandler( private async Task HandleRenameTargetsFromFeatureAsync( DocumentUri uri, string path, Position position, CancellationToken cancellationToken) { - var matchedBindings = FindBindingsAtFeatureStep(uri, path, position: position); + var matchedBindings = FindBindingsAtFeatureStep(uri, path, position, out _, out var isAmbiguous); if (matchedBindings.Count == 0) - return new RenameTargetsResponse(); + return new RenameTargetsResponse { IsAmbiguous = isAmbiguous }; var response = new RenameTargetsResponse(); int idx = 0; @@ -526,7 +532,7 @@ public StepRenameHandler( /// private List FindBindingsAtFeatureStep( DocumentUri uri, string path, Position position) => - FindBindingsAtFeatureStep(uri, path, position, out _); + FindBindingsAtFeatureStep(uri, path, position, out _, out _); /// /// Finds all bindings that match the feature step at the given cursor position, and the @@ -537,9 +543,22 @@ private List FindBindingsAtFeatureStep( /// the edit is applied) should use this overload. /// private List FindBindingsAtFeatureStep( - DocumentUri uri, string path, Position position, out LspRange? matchedRange) + DocumentUri uri, string path, Position position, out LspRange? matchedRange) => + FindBindingsAtFeatureStep(uri, path, position, out matchedRange, out _); + + /// + /// Finds all bindings that match the feature step at the given cursor position, the + /// matched step's own text span via , and whether the step + /// at the cursor is ambiguously bound (matches more than one binding, none of which is a + /// single defined match) via . An ambiguous step never + /// contributes to the returned bindings list — callers use to + /// tell that apart from "nothing matched here" when the list comes back empty. + /// + private List FindBindingsAtFeatureStep( + DocumentUri uri, string path, Position position, out LspRange? matchedRange, out bool isAmbiguous) { matchedRange = null; + isAmbiguous = false; var uriStr = uri.ToString(); var owners = _scopeManager.ResolveOwners(uri); @@ -556,7 +575,7 @@ private List FindBindingsAtFeatureStep( foreach (var step in matchSet.Steps) { - if (step.Result is null || !step.Result.HasDefined) + if (step.Result is null) continue; // Check if cursor falls within the step's range @@ -570,6 +589,13 @@ private List FindBindingsAtFeatureStep( if (position.Character >= stepStartChar && position.Character <= stepEndChar) { matchedRange ??= step.Range.ToLspRange(); + + if (step.Result.HasAmbiguous) + isAmbiguous = true; + + if (!step.Result.HasDefined) + continue; + foreach (var item in step.Result.Items) { if (item.MatchedStepDefinition != null) diff --git a/src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/RenameStepCommand.cs b/src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/RenameStepCommand.cs index 1068183..cd47b21 100644 --- a/src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/RenameStepCommand.cs +++ b/src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/RenameStepCommand.cs @@ -79,6 +79,13 @@ public override async Task ExecuteCommandAsync(IClientContext context, Cancellat if (targets is null || targets.Targets.Count == 0) { + if (targets?.IsAmbiguous == true) + { + _fileLogger.LogInfo("RenameStepCommand: step at cursor position is ambiguously bound."); + VsUtils.ShowStatusBarMessage("Reqnroll: Rename is not supported for an ambiguously bound step."); + return; + } + _fileLogger.LogInfo("RenameStepCommand: no renameable targets at cursor position."); VsUtils.ShowStatusBarMessage("Reqnroll: No step definition found to rename at this position."); return; diff --git a/src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/RenameStepService.cs b/src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/RenameStepService.cs index 7c844bd..1b96651 100644 --- a/src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/RenameStepService.cs +++ b/src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/RenameStepService.cs @@ -73,23 +73,27 @@ public RenameStepService(LspInterception.LspInterceptingPipe pipe, TraceSource t if (result is not JObject obj) return null; + var isAmbiguous = obj["isAmbiguous"]?.Value() ?? false; var targets = obj["targets"] as JArray; - if (targets is null || targets.Count == 0) + if ((targets is null || targets.Count == 0) && !isAmbiguous) return null; - var response = new RenameTargetsResult(); - foreach (var t in targets) + var response = new RenameTargetsResult { IsAmbiguous = isAmbiguous }; + if (targets is not null) { - if (t is not JObject item) continue; - response.Targets.Add(new RenameTargetItem + foreach (var t in targets) { - Label = item["label"]?.Value() ?? "", - Expression = item["expression"]?.Value() ?? "", - AttributeIndex = item["attributeIndex"]?.Value() ?? 0 - }); + if (t is not JObject item) continue; + response.Targets.Add(new RenameTargetItem + { + Label = item["label"]?.Value() ?? "", + Expression = item["expression"]?.Value() ?? "", + AttributeIndex = item["attributeIndex"]?.Value() ?? 0 + }); + } } - return response.Targets.Count > 0 ? response : null; + return response.Targets.Count > 0 || response.IsAmbiguous ? response : null; } /// diff --git a/src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/RenameTargetsResult.cs b/src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/RenameTargetsResult.cs index c59f527..483c528 100644 --- a/src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/RenameTargetsResult.cs +++ b/src/VisualStudio/Reqnroll.IdeSupport.VisualStudio.Extension/RenameStep/RenameTargetsResult.cs @@ -12,6 +12,13 @@ namespace Reqnroll.IdeSupport.VisualStudio.Extension.RenameStep; internal sealed class RenameTargetsResult { public List Targets { get; } = new(); + + /// + /// True when is empty because the cursor is on a step that matches + /// more than one binding, none of which resolve to a single definite match — as opposed to + /// no binding being found at all. + /// + public bool IsAmbiguous { get; init; } } /// One renameable binding attribute at the queried position. diff --git a/tests/LSP/Reqnroll.IdeSupport.LSP.Server.Tests/Features/Rename/StepRenameHandlerTests.cs b/tests/LSP/Reqnroll.IdeSupport.LSP.Server.Tests/Features/Rename/StepRenameHandlerTests.cs index 5b9b639..c53f2af 100644 --- a/tests/LSP/Reqnroll.IdeSupport.LSP.Server.Tests/Features/Rename/StepRenameHandlerTests.cs +++ b/tests/LSP/Reqnroll.IdeSupport.LSP.Server.Tests/Features/Rename/StepRenameHandlerTests.cs @@ -490,6 +490,107 @@ public async Task RenameTargets_from_feature_no_match_returns_empty_response() response!.Targets.Should().BeEmpty(); } + // ── Ambiguous step rename (#34/#36): an ambiguous step must be distinguished from "nothing + // matched here" so callers can show a specific message instead of a misleading generic one. + + private static FeatureBindingMatchSet MakeAmbiguousFeatureMatchSet( + string featureUri, ProjectStepDefinitionBinding binding1, ProjectStepDefinitionBinding binding2, + string scenarioBlock, string stepText, int stepLine, int stepChar) + { + var text = $"Feature: F\nScenario: S\n\t{scenarioBlock} {stepText}\n"; + var snapshot = new LspTextSnapshot(featureUri, 1, text); + var stepPrefix = $"\t{scenarioBlock} "; + var startOffset = text.IndexOf(stepPrefix + stepText) + stepPrefix.Length; + var range = GherkinRange.FromPoint(snapshot, startOffset: startOffset, length: stepText.Length); + var match = new StepBindingMatch( + featureUri, + range, + MatchResult.CreateMultiMatch(new[] + { + MatchResultItem.CreateMatch(binding1, ParameterMatch.NotMatch).CloneToAmbiguousItem(), + MatchResultItem.CreateMatch(binding2, ParameterMatch.NotMatch).CloneToAmbiguousItem() + })); + + return new FeatureBindingMatchSet( + featureUri, + new ProjectOwner("/workspace/MyProject.csproj", ".NETCoreApp,Version=v8.0"), + 1, 1, + new[] { match }); + } + + [Fact] + public async Task PrepareRename_from_feature_ambiguous_step_throws_distinct_message() + { + var featureUri = DocumentUri.FromFileSystemPath("/workspace/test.feature"); + var binding1 = MakeBinding( + ScenarioBlock.Then, new Regex("^to be or not to be$"), + specifiedExpression: "to be or not to be", line: 8, method: "Steps.M1()"); + var binding2 = MakeBinding( + ScenarioBlock.Then, new Regex("^to be or not to be$"), + specifiedExpression: "to be or not to be", line: 20, method: "Steps.M2()"); + + var project = MakeTestProject(); + _scopeManager.ResolveOwners(featureUri).Returns(new[] { project }); + _scopeManager.GetProjectForUri(featureUri).Returns(project); + + var matchSet = MakeAmbiguousFeatureMatchSet( + featureUri.ToString(), binding1, binding2, + "Then", "to be or not to be", stepLine: 2, stepChar: 5); + _matchService.TryGet(Arg.Any(), out Arg.Any()) + .Returns(ci => + { + ci[1] = matchSet; + return true; + }); + + var act = async () => await CreateSut().HandlePrepareRenameAsync( + new PrepareRenameParams + { + TextDocument = new TextDocumentIdentifier { Uri = featureUri }, + Position = new Position(2, 10) + }, + CancellationToken.None); + + (await act.Should().ThrowAsync()) + .WithMessage("*ambiguously bound*"); + } + + [Fact] + public async Task RenameTargets_from_feature_ambiguous_step_sets_IsAmbiguous() + { + var featureUri = DocumentUri.FromFileSystemPath("/workspace/test.feature"); + var binding1 = MakeBinding( + ScenarioBlock.Then, new Regex("^to be or not to be$"), + specifiedExpression: "to be or not to be", line: 8, method: "Steps.M1()"); + var binding2 = MakeBinding( + ScenarioBlock.Then, new Regex("^to be or not to be$"), + specifiedExpression: "to be or not to be", line: 20, method: "Steps.M2()"); + + _scopeManager.ResolveOwners(featureUri).Returns(new[] { MakeTestProject() }); + + var matchSet = MakeAmbiguousFeatureMatchSet( + featureUri.ToString(), binding1, binding2, + "Then", "to be or not to be", stepLine: 2, stepChar: 5); + _matchService.TryGet(Arg.Any(), out Arg.Any()) + .Returns(ci => + { + ci[1] = matchSet; + return true; + }); + + var response = await CreateSut().HandleRenameTargetsAsync( + new TextDocumentPositionParams + { + TextDocument = new TextDocumentIdentifier { Uri = featureUri }, + Position = new Position(2, 14) + }, + CancellationToken.None); + + response.Should().NotBeNull(); + response!.Targets.Should().BeEmpty(); + response.IsAmbiguous.Should().BeTrue(); + } + [Fact] public async Task Rename_from_feature_edits_both_feature_text_and_csharp_attribute() { diff --git a/tests/VisualStudio/Reqnroll.VisualStudio.Tests/RenameStep/RenameTargetsMappingTests.cs b/tests/VisualStudio/Reqnroll.VisualStudio.Tests/RenameStep/RenameTargetsMappingTests.cs index 532b36c..c875c9c 100644 --- a/tests/VisualStudio/Reqnroll.VisualStudio.Tests/RenameStep/RenameTargetsMappingTests.cs +++ b/tests/VisualStudio/Reqnroll.VisualStudio.Tests/RenameStep/RenameTargetsMappingTests.cs @@ -57,6 +57,20 @@ public void Targets_are_mapped_with_label_expression_and_attribute_index() result.Targets[1].AttributeIndex.Should().Be(2); } + [Fact] + public void An_ambiguous_result_with_no_targets_returns_a_flagged_result() + { + var result = RenameStepService.MapTargets(new JObject + { + ["targets"] = new JArray(), + ["isAmbiguous"] = true, + }); + + result.Should().NotBeNull(); + result!.Targets.Should().BeEmpty(); + result.IsAmbiguous.Should().BeTrue(); + } + [Fact] public void Missing_fields_default_and_non_object_entries_are_skipped() {