Skip to content

Commit 36b4bc5

Browse files
mavasanimavasani
authored
Merge pull request #39091 from petrroll/underselectionTestBug
Fix: Underselection test bug
2 parents e2879fc + 7383620 commit 36b4bc5

File tree

4 files changed

+149
-40
lines changed

4 files changed

+149
-40
lines changed

‎src/EditorFeatures/CSharpTest/RefactoringHelpers/RefactoringHelpersTests.cs‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -513,6 +513,80 @@ void M()
513513

514514
#endregion
515515

516+
#region IsUnderselected
517+
[Fact]
518+
[WorkItem(38708, "https://github.com/dotnet/roslyn/issues/38708")]
519+
public async Task TestUnderselectionOnSemicolon()
520+
{
521+
var testText = @"
522+
class Program
523+
{
524+
static void Main()
525+
{
526+
{|result:Main()|}[|;|]
527+
}
528+
}";
529+
await TestNotUnderselectedAsync<ExpressionSyntax>(testText);
530+
}
531+
532+
[Fact]
533+
[WorkItem(38708, "https://github.com/dotnet/roslyn/issues/38708")]
534+
public async Task TestUnderselectionBug1()
535+
{
536+
var testText = @"
537+
class Program
538+
{
539+
public static void Method()
540+
{
541+
//[|>
542+
var str = {|result:"" <|] aaa""|};
543+
}
544+
}";
545+
await TestNotUnderselectedAsync<ExpressionSyntax>(testText);
546+
}
547+
548+
[Fact]
549+
[WorkItem(38708, "https://github.com/dotnet/roslyn/issues/38708")]
550+
public async Task TestUnderselectionBug2()
551+
{
552+
var testText = @"
553+
class C {
554+
public void M()
555+
{
556+
Console.WriteLine(""Hello world"");[|
557+
{|result:Console.WriteLine(new |]C())|};
558+
}
559+
}";
560+
await TestNotUnderselectedAsync<ExpressionSyntax>(testText);
561+
}
562+
563+
[Fact]
564+
[WorkItem(38708, "https://github.com/dotnet/roslyn/issues/38708")]
565+
public async Task TestUnderselection()
566+
{
567+
var testText = @"
568+
class C {
569+
public void M()
570+
{
571+
bool a = {|result:[|true || false || true|]|};
572+
}";
573+
await TestNotUnderselectedAsync<BinaryExpressionSyntax>(testText);
574+
}
575+
576+
[Fact]
577+
[WorkItem(38708, "https://github.com/dotnet/roslyn/issues/38708")]
578+
public async Task TestUnderselection2()
579+
{
580+
var testText = @"
581+
class C {
582+
public void M()
583+
{
584+
bool a = true || [|false || true|] || true;
585+
}";
586+
await TestUnderselectedAsync<BinaryExpressionSyntax>(testText);
587+
}
588+
#endregion
589+
516590
#region Attributes
517591
[Fact]
518592
[WorkItem(37584, "https://github.com/dotnet/roslyn/issues/37584")]

‎src/EditorFeatures/TestUtilities/RefactoringHelpers/RefactoringHelpersTestBase.cs‎

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,19 +39,36 @@ public override void Dispose()
3939
protected async Task TestAsync<TNode>(string text, Func<TNode, bool> predicate) where TNode : SyntaxNode
4040
{
4141
text = GetSelectionAndResultSpans(text, out var selection, out var result);
42-
var resultNode = await GetNodeForSelection<TNode>(text, selection, predicate).ConfigureAwait(false);
42+
var resultNode = await GetNodeForSelectionAsync<TNode>(text, selection, predicate).ConfigureAwait(false);
4343

4444
Assert.NotNull(resultNode);
4545
Assert.Equal(result, resultNode.Span);
4646
}
4747

48+
protected async Task TestUnderselectedAsync<TNode>(string text) where TNode : SyntaxNode
49+
{
50+
text = GetSelectionSpan(text, out var selection);
51+
var resultNode = await GetNodeForSelectionAsync<TNode>(text, selection, Functions<TNode>.True).ConfigureAwait(false);
52+
53+
Assert.NotNull(resultNode);
54+
Assert.True(CodeRefactoringHelpers.IsNodeUnderselected(resultNode, selection));
55+
}
56+
57+
protected async Task TestNotUnderselectedAsync<TNode>(string text) where TNode : SyntaxNode
58+
{
59+
text = GetSelectionAndResultSpans(text, out var selection, out var result);
60+
var resultNode = await GetNodeForSelectionAsync<TNode>(text, selection, Functions<TNode>.True).ConfigureAwait(false);
61+
62+
Assert.Equal(result, resultNode.Span);
63+
Assert.False(CodeRefactoringHelpers.IsNodeUnderselected(resultNode, selection));
64+
}
4865

4966
protected Task TestMissingAsync<TNode>(string text) where TNode : SyntaxNode => TestMissingAsync<TNode>(text, Functions<TNode>.True);
5067
protected async Task TestMissingAsync<TNode>(string text, Func<TNode, bool> predicate) where TNode : SyntaxNode
5168
{
5269
text = GetSelectionSpan(text, out var selection);
5370

54-
var resultNode = await GetNodeForSelection<TNode>(text, selection, predicate).ConfigureAwait(false);
71+
var resultNode = await GetNodeForSelectionAsync<TNode>(text, selection, predicate).ConfigureAwait(false);
5572
Assert.Null(resultNode);
5673
}
5774

@@ -86,7 +103,7 @@ private static string GetSelectionAndResultSpans(string text, out TextSpan selec
86103
return text;
87104
}
88105

89-
private async Task<TNode> GetNodeForSelection<TNode>(string text, TextSpan selection, Func<TNode, bool> predicate) where TNode : SyntaxNode
106+
private async Task<TNode> GetNodeForSelectionAsync<TNode>(string text, TextSpan selection, Func<TNode, bool> predicate) where TNode : SyntaxNode
90107
{
91108
var document = fixture.UpdateDocument(text, SourceCodeKind.Regular);
92109
var relevantNodes = await document.GetRelevantNodesAsync<TNode>(selection, CancellationToken.None).ConfigureAwait(false);

‎src/Features/Core/Portable/CodeRefactoringHelpers.cs‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -30,45 +30,66 @@ internal static class CodeRefactoringHelpers
3030
/// it is the smallest that can be retrieved.
3131
/// </para>
3232
/// <para>
33+
/// When <paramref name="selection"/> doesn't intersect the node in any way it's not considered to be underselected.
34+
/// </para>
35+
/// <para>
3336
/// Null node is always considered underselected.
3437
/// </para>
3538
/// </summary>
3639
public static bool IsNodeUnderselected(SyntaxNode? node, TextSpan selection)
3740
{
41+
// Selection is null -> it's always underselected
42+
// REASON: Easier API use -> underselected node, don't work on it further
3843
if (node == null)
3944
{
4045
return true;
4146
}
4247

48+
// Selection or node is empty -> can't be underselected
4349
if (selection.IsEmpty || node.Span.IsEmpty)
4450
{
4551
return false;
4652
}
4753

48-
// If selection is larger than node.Span -> can't be underselecting
54+
// Selection is larger than node.Span -> can't be underselecting
4955
if (selection.Contains(node.Span))
5056
{
5157
return false;
5258
}
5359

54-
// Only precisely one token is selected -> treat is as empty selection -> not under-selected.
55-
// The rationale is that if a only one Token is selected then the selection wasn't about
56-
// precisely getting the one node and nothing else & therefore we should treat it as empty
57-
// selection.
58-
var selectionStartToken = node.FindToken(selection.Start);
59-
if (selection.IsAround(selectionStartToken))
60+
// Selection doesn't intersect node -> can't be underselecting.
61+
// RATIONALE: If there's no intersection then we got the node in some other way, e.g.
62+
// extracting it after user selected `;` at the end of an expression statement
63+
// `foo()[|;|]` for `foo()` node.
64+
if (!node.FullSpan.OverlapsWith(selection))
6065
{
6166
return false;
6267
}
6368

69+
// Only precisely one token of the node is selected -> treat is as empty selection -> not
70+
// underselected. The rationale is that if only one Token is selected then the selection
71+
// wasn't about precisely getting the one node and nothing else & therefore we should treat
72+
// it as empty selection.
73+
if (node.FullSpan.Contains(selection.Start))
74+
{
75+
var selectionStartToken = node.FindToken(selection.Start);
76+
if (selection.IsAround(selectionStartToken))
77+
{
78+
return false;
79+
}
80+
}
81+
6482
var beginningNode = node.FindToken(node.Span.Start).Parent;
6583
var endNode = node.FindToken(node.Span.End - 1).Parent;
6684

6785
// Node is underselected if either the first (lowest) child doesn't contain start of selection
6886
// of the last child doesn't intersect with the end.
87+
88+
// Node is underselected if either the first (lowest) child ends before the selection has started
89+
// or the last child starts after the selection ends (i.e. one of them is completely on the outside of selection).
6990
// It's a crude heuristic but it allows omitting parts of nodes or trivial tokens from the beginning/end
7091
// but fires up e.g.: `1 + [|2 + 3|]`.
71-
return !beginningNode.Span.IntersectsWith(selection.Start) || !endNode.Span.IntersectsWith(selection.End - 1);
92+
return beginningNode.Span.End <= selection.Start || endNode.Span.Start >= selection.End;
7293
}
7394

7495

‎src/Test/Utilities/Portable/MarkedSource/MarkupTestFile.cs‎

Lines changed: 26 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,12 @@ private static void Parse(
5858

5959
// A stack of span starts along with their associated annotation name. [||] spans simply
6060
// have empty string for their annotation name.
61-
var spanStartStack = new Stack<Tuple<int, string>>();
61+
var spanStartStack = new Stack<(int matchIndex, string name)>();
62+
var namedSpanStartStack = new Stack<(int matchIndex, string name)>();
6263

6364
while (true)
6465
{
65-
var matches = new List<Tuple<int, string>>();
66+
var matches = new List<(int matchIndex, string name)>();
6667
AddMatch(input, PositionString, currentIndexInInput, matches);
6768
AddMatch(input, SpanStartString, currentIndexInInput, matches);
6869
AddMatch(input, SpanEndString, currentIndexInInput, matches);
@@ -71,7 +72,7 @@ private static void Parse(
7172
var namedSpanStartMatch = s_namedSpanStartRegex.Match(input, currentIndexInInput);
7273
if (namedSpanStartMatch.Success)
7374
{
74-
matches.Add(Tuple.Create(namedSpanStartMatch.Index, namedSpanStartMatch.Value));
75+
matches.Add((namedSpanStartMatch.Index, namedSpanStartMatch.Value));
7576
}
7677

7778
if (matches.Count == 0)
@@ -80,10 +81,10 @@ private static void Parse(
8081
break;
8182
}
8283

83-
var orderedMatches = matches.OrderBy((t1, t2) => t1.Item1 - t2.Item1).ToList();
84+
var orderedMatches = matches.OrderBy((t1, t2) => t1.matchIndex - t2.matchIndex).ToList();
8485
if (orderedMatches.Count >= 2 &&
85-
spanStartStack.Count > 0 &&
86-
matches[0].Item1 == matches[1].Item1 - 1)
86+
(spanStartStack.Count > 0 || namedSpanStartStack.Count > 0) &&
87+
matches[0].matchIndex == matches[1].matchIndex - 1)
8788
{
8889
// We have a slight ambiguity with cases like these:
8990
//
@@ -92,8 +93,8 @@ private static void Parse(
9293
// Is it starting a new match, or ending an existing match. As a workaround, we
9394
// special case these and consider it ending a match if we have something on the
9495
// stack already.
95-
if ((matches[0].Item2 == SpanStartString && matches[1].Item2 == SpanEndString && spanStartStack.Peek().Item2 == string.Empty) ||
96-
(matches[0].Item2 == SpanStartString && matches[1].Item2 == NamedSpanEndString && spanStartStack.Peek().Item2 != string.Empty))
96+
if ((matches[0].name == SpanStartString && matches[1].name == SpanEndString && !spanStartStack.IsEmpty()) ||
97+
(matches[0].name == SpanStartString && matches[1].name == NamedSpanEndString && !namedSpanStartStack.IsEmpty()))
9798
{
9899
orderedMatches.RemoveAt(0);
99100
}
@@ -102,8 +103,8 @@ private static void Parse(
102103
// Order the matches by their index
103104
var firstMatch = orderedMatches.First();
104105

105-
var matchIndexInInput = firstMatch.Item1;
106-
var matchString = firstMatch.Item2;
106+
var matchIndexInInput = firstMatch.matchIndex;
107+
var matchString = firstMatch.name;
107108

108109
var matchIndexInOutput = matchIndexInInput - inputOutputOffset;
109110
outputBuilder.Append(input.Substring(currentIndexInInput, matchIndexInInput - currentIndexInInput));
@@ -123,7 +124,7 @@ private static void Parse(
123124
break;
124125

125126
case SpanStartString:
126-
spanStartStack.Push(Tuple.Create(matchIndexInOutput, string.Empty));
127+
spanStartStack.Push((matchIndexInOutput, string.Empty));
127128
break;
128129

129130
case SpanEndString:
@@ -132,31 +133,22 @@ private static void Parse(
132133
throw new ArgumentException(string.Format("Saw {0} without matching {1}", SpanEndString, SpanStartString));
133134
}
134135

135-
if (spanStartStack.Peek().Item2.Length > 0)
136-
{
137-
throw new ArgumentException(string.Format("Saw {0} without matching {1}", NamedSpanStartString, NamedSpanEndString));
138-
}
139-
140136
PopSpan(spanStartStack, tempSpans, matchIndexInOutput);
141137
break;
142138

143139
case NamedSpanStartString:
144140
var name = namedSpanStartMatch.Groups[1].Value;
145-
spanStartStack.Push(Tuple.Create(matchIndexInOutput, name));
141+
namedSpanStartStack.Push((matchIndexInOutput, name));
146142
break;
147143

148144
case NamedSpanEndString:
149-
if (spanStartStack.Count == 0)
145+
if (namedSpanStartStack.Count == 0)
150146
{
151147
throw new ArgumentException(string.Format("Saw {0} without matching {1}", NamedSpanEndString, NamedSpanStartString));
152148
}
153149

154-
if (spanStartStack.Peek().Item2.Length == 0)
155-
{
156-
throw new ArgumentException(string.Format("Saw {0} without matching {1}", SpanStartString, SpanEndString));
157-
}
158150

159-
PopSpan(spanStartStack, tempSpans, matchIndexInOutput);
151+
PopSpan(namedSpanStartStack, tempSpans, matchIndexInOutput);
160152
break;
161153

162154
default:
@@ -169,6 +161,11 @@ private static void Parse(
169161
throw new ArgumentException(string.Format("Saw {0} without matching {1}", SpanStartString, SpanEndString));
170162
}
171163

164+
if (namedSpanStartStack.Count > 0)
165+
{
166+
throw new ArgumentException(string.Format("Saw {0} without matching {1}", NamedSpanEndString, NamedSpanEndString));
167+
}
168+
172169
// Append the remainder of the string.
173170
outputBuilder.Append(input.Substring(currentIndexInInput));
174171
output = outputBuilder.ToString();
@@ -187,22 +184,22 @@ private static V GetOrAdd<K, V>(IDictionary<K, V> dictionary, K key, Func<K, V>
187184
}
188185

189186
private static void PopSpan(
190-
Stack<Tuple<int, string>> spanStartStack,
187+
Stack<(int matchIndex, string name)> spanStartStack,
191188
IDictionary<string, ArrayBuilder<TextSpan>> spans,
192189
int finalIndex)
193190
{
194-
var spanStartTuple = spanStartStack.Pop();
191+
var (matchIndex, name) = spanStartStack.Pop();
195192

196-
var span = TextSpan.FromBounds(spanStartTuple.Item1, finalIndex);
197-
GetOrAdd(spans, spanStartTuple.Item2, _ => ArrayBuilder<TextSpan>.GetInstance()).Add(span);
193+
var span = TextSpan.FromBounds(matchIndex, finalIndex);
194+
GetOrAdd(spans, name, _ => ArrayBuilder<TextSpan>.GetInstance()).Add(span);
198195
}
199196

200-
private static void AddMatch(string input, string value, int currentIndex, List<Tuple<int, string>> matches)
197+
private static void AddMatch(string input, string value, int currentIndex, List<(int, string)> matches)
201198
{
202199
var index = input.IndexOf(value, currentIndex, StringComparison.Ordinal);
203200
if (index >= 0)
204201
{
205-
matches.Add(Tuple.Create(index, value));
202+
matches.Add((index, value));
206203
}
207204
}
208205

0 commit comments

Comments
 (0)