Skip to content
Closed
Show file tree
Hide file tree
Changes from 3 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 @@ -168,5 +168,116 @@ public MyClass1()
}";
await VerifyCustomCommitProviderAsync(markupBeforeCommit, ItemToCommit, expectedCodeAfterCommit);
}

[WpfTheory]
[InlineData("class")]
[InlineData("enum")]
[InlineData("interface")]
[InlineData("record")]
[InlineData("struct")]
[InlineData("record class")]
[InlineData("record struct")]
[Trait(Traits.Feature, Traits.Features.Completion)]
public async Task InsertConstructorSnippetHasNestedTypeTest(string keyword)
{
var markupBeforeCommit =
$$"""
class Outer
{
$$
{{keyword}} Inner
{
}
}
""";

var expectedCodeAfterCommit =
$$"""
class Outer
{
public Outer()
{
$$
}
{{keyword}} Inner
{
}
}
""";

await VerifyCustomCommitProviderAsync(markupBeforeCommit, ItemToCommit, expectedCodeAfterCommit);
}

[WpfFact, Trait(Traits.Feature, Traits.Features.Completion)]
public async Task InsertConstructorSnippetHasNestedClassGluedOuterTest()
{
var markupBeforeCommit_GluedToOuter =
@"class Outer
{$$
class Inner
{
}
}";
// The position ($$) is in a strange position because code does not format after insertion.
var expectedCodeAfterCommit_GluedToOuter =
@"class Outer
{ public Outer()
{

$$ }
class Inner
{
}
}";
await VerifyCustomCommitProviderAsync(markupBeforeCommit_GluedToOuter, ItemToCommit, expectedCodeAfterCommit_GluedToOuter);
}

[WpfTheory]
[InlineData("class")]
[InlineData("record")]
[InlineData("struct")]
[InlineData("record class")]
[InlineData("record struct")]
[Trait(Traits.Feature, Traits.Features.Completion)]
public async Task InsertConstructorSnippetInConstructableTypeTest(string keyword)
{
var markupBeforeCommit =
$$"""
{{keyword}} MyName
{
$$
}
""";

var expectedCodeAfterCommit =
$$"""
{{keyword}} MyName
{
public MyName()
{
$$
}
}
""";

await VerifyCustomCommitProviderAsync(markupBeforeCommit, ItemToCommit, expectedCodeAfterCommit);
}

[WpfTheory]
[InlineData("enum")]
[InlineData("interface")]
[Trait(Traits.Feature, Traits.Features.Completion)]
public async Task InsertConstructorSnippetInNotConstructableTypeTest(string keyword)
{
var markupBeforeCommit =
$$"""
{{keyword}} MyName
{
$$
}
""";

await VerifyItemIsAbsentAsync(markupBeforeCommit, ItemToCommit);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -93,5 +93,11 @@ protected override async Task<Document> AddIndentationToDocumentAsync(Document d
var newRoot = root.ReplaceNode(constructorDeclaration, newConstructorDeclaration);
return document.WithSyntaxRoot(newRoot);
}

protected override bool IsConstructableTypeDeclaration(ISyntaxFacts syntaxFacts, SyntaxNode node)
{
return syntaxFacts.IsTypeDeclaration(node)
&& SyntaxKindSet.ClassStructRecordTypeDeclarations.Contains(node.Kind());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,25 @@ protected override async Task<TextChange> GenerateSnippetTextChangeAsync(Documen
var syntaxFacts = document.GetRequiredLanguageService<ISyntaxFactsService>();
var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
var nodeAtPosition = root.FindNode(TextSpan.FromBounds(position, position));
var containingType = nodeAtPosition.FirstAncestorOrSelf<SyntaxNode>(syntaxFacts.IsTypeDeclaration);

// Skip inner class in nested class if position is out of it.
// For example "class Outer{\r\n ctor@ \r\n class Inner {} }"
// Accept even if "static class" (CS0710).
var containingType = nodeAtPosition.FirstAncestorOrSelf<SyntaxNode>((node) =>
{
return syntaxFacts.IsTypeDeclaration(node)
&& IsConstructableTypeDeclaration(syntaxFacts, node)
&& node.Span.Contains(position);
Copy link
Member

Choose a reason for hiding this comment

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

I think the Contains check would work. But we have FindTokenOnLeftOfPosition you could use for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@genlu Could you please explain a little more how to use FindTokenOnLeftPosition instead of Span.Contains?
In the following pattern, I tried, but I get a CloseBraceToken of InnerClass

$$"""
class Outer
{
    class Inner
    {
    }

    $$
}
""";

Copy link
Member

@genlu genlu May 15, 2023

Choose a reason for hiding this comment

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

This helper is what I had in mind. But it is C# specific.

Then there's this, but it seems the behavior might be the same as the code that causing problem in the first place. maybe try to change the code in CSharpSyntaxFacts.GetContainingTypeDeclaration to do something similar to GetContainingTypeOrEnumDeclarations and see what breaks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@genlu I changed it to process the parent node if CloseBraceToken is found, instead of using Span.Contains.

I tried using semantic model, but gave up on it because it only fails if the ctor is entered at the end of the file.

});

Contract.ThrowIfNull(containingType);
var constructorDeclaration = generator.ConstructorDeclaration(
containingTypeName: syntaxFacts.GetIdentifierOfTypeDeclaration(containingType).ToString(),
accessibility: Accessibility.Public);
return new TextChange(TextSpan.FromBounds(position, position), constructorDeclaration.NormalizeWhitespace().ToFullString());
}

// Move this to ISyntaxFacts if approved.
protected abstract bool IsConstructableTypeDeclaration(ISyntaxFacts syntaxFacts, SyntaxNode node);
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? There's already a check in CSharpConstructorSnippetProvider.IsValidSnippetLocationAsync

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@genlu Isn't IsValidSnippetLocationAsync check that the constructor can be placed where 'ctor' is entered?
I think that function is not suitable to get the target class name.

Is it acceptable that the decisions required within AbstractConstructorSnippetProvider depend on the processing implemented in the classes that inherited AbstractConstructorSnippetProvider?
I can't think of any other way to determine the constructable type from within an abstract class.

Copy link
Member

Choose a reason for hiding this comment

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

You are right, IsValidSnippetLocationAsync](https://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/Snippets/SnippetProviders/AbstractSnippetProvider.cs,73) is used to decide whether a snippet should placed at the location. So if the check in IsValidSnippetLocationAsync fails, we won't even show the snippet thus it will never get to the point where code change is being calculated.

So to answer your question: yes, it's not only acceptable, it's actually supposed to work this way by design.

}
}