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.

Sign up for GitHub

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

Jump to bottom

Support abstract methods and records in complete statement command #48128

Merged
merged 12 commits into from
Apr 8, 2021
Merged

Support abstract methods and records in complete statement command #48128

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b0e0b81
Add tests for simple positional record
Youssef1313 Sep 28, 2020
8abe812
Update CSharpCompleteStatementCommandHandlerTests.cs
Youssef1313 Sep 28, 2020
f5b2665
Update CSharpCompleteStatementCommandHandlerTests.cs
Youssef1313 Sep 28, 2020
ce97cf1
Handle records and abstract methods in complete statement command
Youssef1313 Oct 2, 2020
63dec08
Address PR feedback
Youssef1313 Jan 20, 2021
24538b2
Add more test
Youssef1313 Jan 20, 2021
60813c5
Update src/EditorFeatures/CSharp/CompleteStatement/CompleteStatementC…
Youssef1313 Feb 18, 2021
ba0caa3
Fix test
Youssef1313 Feb 18, 2021
1303a00
Fix IDE0011
Youssef1313 Feb 18, 2021
4a71491
Update CompleteStatementCommandHandler.cs
Youssef1313 Feb 23, 2021
62b40d0
Support partial method in complete statement
Youssef1313 Apr 1, 2021
8f022fa
Update CSharpCompleteStatementCommandHandlerTests.cs
Youssef1313 Apr 1, 2021
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
28 changes: 27 additions & 1 deletion src/EditorFeatures/CSharp/CompleteStatement/CompleteStatementCommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ private static void MoveCaretToSemicolonPosition(
return;
}
else if (syntaxFacts.IsStatement(currentNode)
|| currentNode.IsKind(SyntaxKind.FieldDeclaration, SyntaxKind.DelegateDeclaration, SyntaxKind.ArrowExpressionClause))
|| CanHaveSemicolon(currentNode))
{
MoveCaretToFinalPositionInStatement(currentNode, args, caret, isInsideDelimiters);
return;
Expand All @@ -203,6 +203,30 @@ private static void MoveCaretToSemicolonPosition(
}
}

private static bool CanHaveSemicolon(SyntaxNode currentNode)
{
if (currentNode.IsKind(SyntaxKind.FieldDeclaration, SyntaxKind.DelegateDeclaration, SyntaxKind.ArrowExpressionClause))
{
return true;
}

if (currentNode is RecordDeclarationSyntax { OpenBraceToken: { IsMissing: true } })
{
return true;
}

if (currentNode is MethodDeclarationSyntax method)
{
if (method.Modifiers.Any(SyntaxKind.AbstractKeyword) || method.Modifiers.Any(SyntaxKind.ExternKeyword) ||
method.IsParentKind(SyntaxKind.InterfaceDeclaration))
Comment on lines +220 to +221
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❔ Is there any harm in making this unconditional?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Note that partial methods also can be declared without a body.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sharwell I think it will unnecessarily move the caret to the end for typos, which can be annoying.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Note that partial methods also can be declared without a body.

That's a good case. Going to fix it and add a test.

{
return true;
}
}

return false;
}

private static bool IsInConditionOfDoStatement(SyntaxNode currentNode, SnapshotPoint caret)
{
if (!currentNode.IsKind(SyntaxKind.DoStatement, out DoStatementSyntax doStatement))
Expand Down Expand Up @@ -257,6 +281,8 @@ private static bool TryGetCaretPositionToMove(SyntaxNode statementNode, Snapshot
case SyntaxKind.FieldDeclaration:
case SyntaxKind.DelegateDeclaration:
case SyntaxKind.ArrowExpressionClause:
case SyntaxKind.MethodDeclaration:
case SyntaxKind.RecordDeclaration:
// These statement types end in a semicolon.
// if the original caret was inside any delimiters, `caret` will be after the outermost delimiter
targetPosition = caret;
Expand Down
26 changes: 23 additions & 3 deletions ...EditorFeatures/CSharpTest/CompleteStatement/CSharpCompleteStatementCommandHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,15 @@ internal static int MethodM(int a, int b)
#region ParameterList

[WpfTheory, Trait(Traits.Feature, Traits.Features.CompleteStatement)]
[InlineData("extern void M(object o$$)", "extern void M(object o)")]
[InlineData("abstract void M(object o$$)", "abstract void M(object o)")]
[InlineData("abstract void M($$object o)", "abstract void M(object o)")]
[InlineData("abstract void M(object o = default(object$$))", "abstract void M(object o = default(object))")]
[InlineData("abstract void M(object o = default($$object))", "abstract void M(object o = default(object))")]
[InlineData("abstract void M(object o = $$default(object))", "abstract void M(object o = default(object))")]
[InlineData("public record C(int X, $$int Y)", "public record C(int X, int Y)")]
[InlineData("public record C(int X, int$$ Y)", "public record C(int X, int Y)")]
[InlineData("public record C(int X, int Y$$)", "public record C(int X, int Y)")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see tests added for records, but nothing new for the 'abstract method' code you added. do we need tests for that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CyrusNajmabadi Abstract methods already have tests. But the test was disabled. I added record tests (in addition to the existing for abstract methods), and enabled the test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok!

public void ParameterList_CouldBeHandled(string signature, string expectedSignature)
Youssef1313 marked this conversation as resolved.
Show resolved Hide resolved
{
var code = $@"
Expand All @@ -61,9 +65,25 @@ public class Class1
{expectedSignature};$$
}}";

// These cases are not currently handled. If support is added in the future, 'expected' should be correct.
_ = expected;
VerifyNoSpecialSemicolonHandling(code);
VerifyTypingSemicolon(code, expected);
}

[WpfFact, Trait(Traits.Feature, Traits.Features.CompleteStatement)]
public void ParameterList_InterfaceMethod()
{
var code = @"
public interface I
{
public void M(object o$$)
}";

var expected = @"
public interface I
{
public void M(object o);$$
}";

VerifyTypingSemicolon(code, expected);
}

[WpfTheory, Trait(Traits.Feature, Traits.Features.CompleteStatement)]
Expand Down