Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not return empty nodes in the refactoring helpers service. #58830

Merged
merged 6 commits into from
Jan 14, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2743,5 +2743,69 @@ public C(object a, object b, object c)
CodeActionEquivalenceKey = nameof(FeaturesResources.Add_null_checks_for_all_parameters)
}.RunAsync();
}

[WorkItem(58811, "https://github.com/dotnet/roslyn/issues/58811")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestMissingParameter1()
{
var source = @"
using System;

class C
{
public C(string s,[||]{|CS1031:{|CS1001:)|}|}
{
}
}";
await VerifyCS.VerifyRefactoringAsync(source, source);
}

[WorkItem(58811, "https://github.com/dotnet/roslyn/issues/58811")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestMissingParameter2()
{
var source = @"
using System;

class C
{
public C(string s,[||] {|CS1031:{|CS1001:)|}|}
{
}
}";
await VerifyCS.VerifyRefactoringAsync(source, source);
}

[WorkItem(58811, "https://github.com/dotnet/roslyn/issues/58811")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestMissingParameter3()
{
var source = @"
using System;

class C
{
public C(string s, [||]{|CS1031:{|CS1001:)|}|}
{
}
}";
await VerifyCS.VerifyRefactoringAsync(source, source);
}

[WorkItem(58811, "https://github.com/dotnet/roslyn/issues/58811")]
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsInitializeParameter)]
public async Task TestMissingParameter4()
{
var source = @"
using System;

class C
{
public C(string s, [||] {|CS1031:{|CS1001:)|}|}
{
}
}";
await VerifyCS.VerifyRefactoringAsync(source, source);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,26 @@ C LocalFunction(C c)

[Fact]
[WorkItem(35525, "https://github.com/dotnet/roslyn/issues/35525")]
public async Task TestInEmptySyntaxNode()
public async Task TestInEmptySyntaxNode_AllowEmptyNodesTrue1()
{
var testText = @"
class C
{
void M()
{
N(0, [||]{|result:|});
}

int N(int a, int b, int c)
{
}
}";
await TestAsync<ArgumentSyntax>(testText, allowEmptyNodes: true);
}

[Fact]
[WorkItem(35525, "https://github.com/dotnet/roslyn/issues/35525")]
public async Task TestInEmptySyntaxNode_AllowEmptyNodesTrue2()
{
var testText = @"
class C
Expand All @@ -337,8 +356,47 @@ int N(int a, int b, int c)
{
}
}";
await TestAsync<ArgumentSyntax>(testText);
await TestAsync<ArgumentSyntax>(testText, allowEmptyNodes: true);
}

[Fact]
[WorkItem(35525, "https://github.com/dotnet/roslyn/issues/35525")]
public async Task TestInEmptySyntaxNode_AllowEmptyNodesFalse1()
{
var testText = @"
class C
{
void M()
{
N(0, [||], 0));
}

int N(int a, int b, int c)
{
}
}";
await TestMissingAsync<ArgumentSyntax>(testText, allowEmptyNodes: false);
}

[Fact]
[WorkItem(35525, "https://github.com/dotnet/roslyn/issues/35525")]
public async Task TestInEmptySyntaxNode_AllowEmptyNodesFalse2()
{
var testText = @"
class C
{
void M()
{
N(0, {|result:N(0, [||], 0)|});
}

int N(int a, int b, int c)
{
}
}";
await TestAsync<ArgumentSyntax>(testText, allowEmptyNodes: false);
}

#endregion

#region Selections
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,13 @@ public abstract class RefactoringHelpersTestBase<TWorkspaceFixture> : TestBase
private protected ReferenceCountedDisposable<TWorkspaceFixture> GetOrCreateWorkspaceFixture()
=> _fixtureHelper.GetOrCreateFixture();

protected Task TestAsync<TNode>(string text) where TNode : SyntaxNode => TestAsync<TNode>(text, Functions<TNode>.True);
protected Task TestAsync<TNode>(string text, bool allowEmptyNodes = false) where TNode : SyntaxNode
=> TestAsync(text, Functions<TNode>.True, allowEmptyNodes);

protected async Task TestAsync<TNode>(string text, Func<TNode, bool> predicate) where TNode : SyntaxNode
protected async Task TestAsync<TNode>(string text, Func<TNode, bool> predicate, bool allowEmptyNodes = false) where TNode : SyntaxNode
{
text = GetSelectionAndResultSpans(text, out var selection, out var result);
var resultNode = await GetNodeForSelectionAsync<TNode>(text, selection, predicate).ConfigureAwait(false);
var resultNode = await GetNodeForSelectionAsync(text, selection, predicate, allowEmptyNodes).ConfigureAwait(false);

Assert.NotNull(resultNode);
Assert.Equal(result, resultNode.Span);
Expand All @@ -58,12 +59,14 @@ protected async Task TestNotUnderselectedAsync<TNode>(string text) where TNode :
Assert.False(CodeRefactoringHelpers.IsNodeUnderselected(resultNode, selection));
}

protected Task TestMissingAsync<TNode>(string text) where TNode : SyntaxNode => TestMissingAsync<TNode>(text, Functions<TNode>.True);
protected async Task TestMissingAsync<TNode>(string text, Func<TNode, bool> predicate) where TNode : SyntaxNode
protected Task TestMissingAsync<TNode>(string text, bool allowEmptyNodes = false) where TNode : SyntaxNode
=> TestMissingAsync(text, Functions<TNode>.True, allowEmptyNodes);

protected async Task TestMissingAsync<TNode>(string text, Func<TNode, bool> predicate, bool allowEmptyNodes = false) where TNode : SyntaxNode
{
text = GetSelectionSpan(text, out var selection);

var resultNode = await GetNodeForSelectionAsync<TNode>(text, selection, predicate).ConfigureAwait(false);
var resultNode = await GetNodeForSelectionAsync<TNode>(text, selection, predicate, allowEmptyNodes).ConfigureAwait(false);
Assert.Null(resultNode);
}

Expand Down Expand Up @@ -98,12 +101,12 @@ private static string GetSelectionAndResultSpans(string text, out TextSpan selec
return text;
}

private async Task<TNode> GetNodeForSelectionAsync<TNode>(string text, TextSpan selection, Func<TNode, bool> predicate) where TNode : SyntaxNode
private async Task<TNode> GetNodeForSelectionAsync<TNode>(string text, TextSpan selection, Func<TNode, bool> predicate, bool allowEmptyNodes = false) where TNode : SyntaxNode
{
using var workspaceFixture = GetOrCreateWorkspaceFixture();

var document = workspaceFixture.Target.UpdateDocument(text, SourceCodeKind.Regular);
var relevantNodes = await document.GetRelevantNodesAsync<TNode>(selection, CancellationToken.None).ConfigureAwait(false);
var relevantNodes = await document.GetRelevantNodesAsync<TNode>(selection, allowEmptyNodes, CancellationToken.None).ConfigureAwait(false);

return relevantNodes.FirstOrDefault(predicate);
}
Expand Down
41 changes: 22 additions & 19 deletions src/Features/Core/Portable/CodeRefactoringHelpers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,60 +15,63 @@ internal static class CodeRefactoringHelpers
{
/// <summary>
/// <para>
/// Determines if a <paramref name="node"/> is underselected given <paramref name="selection"/>.
/// Determines if a <paramref name="node"/> is under-selected given <paramref name="selection"/>.
/// </para>
/// <para>
/// Underselection is defined as omitting whole nodes from either the beginning or the end. It can be used e.g. to detect that
/// following selection `1 + [|2 + 3|]` is underselecting the whole expression node tree.
/// Under-selection is defined as omitting whole nodes from either the beginning or the end. It can be used e.g.
/// to detect that following selection `1 + [|2 + 3|]` is under-selecting the whole expression node tree.
/// </para>
/// <para>
/// Returns false if only and precisely one <see cref="SyntaxToken"/> is selected. In that case the <paramref name="selection"/>
/// is treated more as a caret location.
/// Returns false if only and precisely one <see cref="SyntaxToken"/> is selected. In that case the <paramref
/// name="selection"/> is treated more as a caret location.
/// </para>
/// <para>
/// It's intended to be used in conjunction with <see cref="IRefactoringHelpersService.GetRelevantNodesAsync{TSyntaxNode}(Document, TextSpan, CancellationToken)"/>
/// that, for non-empty selections, returns the smallest encompassing node. A node that can, for certain refactorings, be too large given user-selection even though
/// it is the smallest that can be retrieved.
/// It's intended to be used in conjunction with <see
/// cref="IRefactoringHelpersService.GetRelevantNodesAsync{TSyntaxNode}(Document, TextSpan, bool,
/// CancellationToken)"/> that, for non-empty selections, returns the smallest encompassing node. A node that
/// can, for certain refactorings, be too large given user-selection even though it is the smallest that can be
/// retrieved.
/// </para>
/// <para>
/// When <paramref name="selection"/> doesn't intersect the node in any way it's not considered to be underselected.
/// When <paramref name="selection"/> doesn't intersect the node in any way it's not considered to be
/// under-selected.
/// </para>
/// <para>
/// Null node is always considered underselected.
/// Null node is always considered under-selected.
/// </para>
/// </summary>
public static bool IsNodeUnderselected(SyntaxNode? node, TextSpan selection)
{
// Selection is null -> it's always underselected
// REASON: Easier API use -> underselected node, don't work on it further
// Selection is null -> it's always under-selected
// REASON: Easier API use -> under-selected node, don't work on it further
if (node == null)
{
return true;
}

// Selection or node is empty -> can't be underselected
// Selection or node is empty -> can't be under-selected
if (selection.IsEmpty || node.Span.IsEmpty)
{
return false;
}

// Selection is larger than node.Span -> can't be underselecting
// Selection is larger than node.Span -> can't be under-selecting
if (selection.Contains(node.Span))
{
return false;
}

// Selection doesn't intersect node -> can't be underselecting.
// Selection doesn't intersect node -> can't be under-selecting.
// RATIONALE: If there's no intersection then we got the node in some other way, e.g.
// extracting it after user selected `;` at the end of an expression statement
// `foo()[|;|]` for `foo()` node.
// `goo()[|;|]` for `goo()` node.
if (!node.FullSpan.OverlapsWith(selection))
{
return false;
}

// Only precisely one token of the node is selected -> treat is as empty selection -> not
// underselected. The rationale is that if only one Token is selected then the selection
// under-selected. The rationale is that if only one Token is selected then the selection
// wasn't about precisely getting the one node and nothing else & therefore we should treat
// it as empty selection.
if (node.FullSpan.Contains(selection.Start))
Expand All @@ -85,10 +88,10 @@ public static bool IsNodeUnderselected(SyntaxNode? node, TextSpan selection)
RoslynDebug.Assert(beginningNode is object);
RoslynDebug.Assert(endNode is object);

// Node is underselected if either the first (lowest) child doesn't contain start of selection
// Node is under-selected if either the first (lowest) child doesn't contain start of selection
// of the last child doesn't intersect with the end.

// Node is underselected if either the first (lowest) child ends before the selection has started
// Node is under-selected if either the first (lowest) child ends before the selection has started
// or the last child starts after the selection ends (i.e. one of them is completely on the outside of selection).
// It's a crude heuristic but it allows omitting parts of nodes or trivial tokens from the beginning/end
// but fires up e.g.: `1 + [|2 + 3|]`.
Expand Down
Loading