-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix 'ctor' snippet in nested class #68177
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
Conversation
| %keyword% Inner | ||
| { | ||
| } | ||
| }".Replace("%keyword%", keyword); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use string interpolation in all tests. This %keyword% notation is really confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DoctorKrolic , Should I change to Interpolation?
$@"class Outer
{{
$$
{ keyword } Inner
{{
}}
}}";There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it looks if not better then at least more familiar. If you want to improve it even more use raw string literals (I would preffer this).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DoctorKrolic I pushed modified code.
$$"""
class Outer
{
$$
{{keyword}} Inner
{
}
}
""";| } | ||
|
|
||
| // Move this to ISyntaxFacts if approved. | ||
| protected abstract bool IsConstructableTypeDeclaration(ISyntaxFacts syntaxFacts, SyntaxNode node); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| { | ||
| return syntaxFacts.IsTypeDeclaration(node) | ||
| && IsConstructableTypeDeclaration(syntaxFacts, node) | ||
| && node.Span.Contains(position); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
{
}
$$
}
""";There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Fixes #68176
Fixes AB#1818584
Ignore the input if it is entered in the trivia position of the inner class.
Add Test code.