-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Offer 'readonly' completion in type contexts #26186
Conversation
private static bool IsValidContextForType(CSharpSyntaxContext context, CancellationToken cancellationToken) | ||
{ | ||
return context.IsTypeDeclarationContext(validModifiers: SyntaxKindSet.AllTypeModifiers, | ||
validTypeDeclarations: SyntaxKindSet.ClassStructTypeDeclarations, canBePartial: false, cancellationToken); |
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.
can't have a partial readonly struct?
can you have a readonly class? should this just be valid on StructTypeDecls?
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.
Good catch. Fixed the partial case.
validTypeDeclarations
is correct though. It checks the containing type declaration. A readonly struct can be declared inside a class or a struct (there are tests for that).
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.
Gotcha. Thanks!
{ | ||
await VerifyAbsenceAsync( | ||
@"new $$"); | ||
await VerifyKeywordAsync(SourceCodeKind.Regular, @"new $$"); |
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.
when is it legal to type readonly after new?
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.
ah.. i see, i thought this was object creation
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.
but this test is still a little odd.. this code would be incorrect outside a class/struct
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.
@Neme12 It is not a requirement for completion to strictly only offer was is allowed. It's better to offer a bit too much than bit too little.
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.
@jcouv I didn't mean that you have to change the behavior and add a test to make sure it's not offered here. I just think it would be better to test this in a scenario where it makes sense (inside a class). I don't care if it's offered here or not. But I know we do care it's offered inside a class after new.
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.
Thanks for clarifying.
What's the scenario where new readonly
is useful inside a 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.
class B
{
protected struct S { }
}
class C : B
{
new readonly struct S { }
}
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.
class OuterClass
{
public struct InnerStruct
{
}
class InnerClass : OuterClass
{
public new readonly struct InnerStruct
{
}
}
}
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.
Note: this is super corner case. i personally wouldn't care much if this didn't happen, or came later.
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.
Thanks. I expect it will just work. I'll add a test
I think there is the same issue with |
Also, did you check that the "struct" keyword is offered after readonly? |
At the same time, please make sure class C
{
void M()
{
ref $$
ref readonly $$
ref var x = ref $$
}
} |
@Neme12 I'll update the PR to make sure that I don't plan to tighten the completion though ( |
16ff7bb
to
6a37973
Compare
public async Task TestAfterReadonlyInMethod() | ||
{ | ||
// struct is not useful here, but offered anyways | ||
await VerifyAbsenceAsync(SourceCodeKind.Regular, |
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.
"offered anyways" VerifyAbsence?
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.
Good catch. Thanks
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.
but in cases above in TestAfterReadonlyRef, TestAfterRef etc it should be offered right?
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.
Thanks
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.
It should be fixed now. Thanks!
4ff5794
to
ff60468
Compare
@dotnet/roslyn-ide for review. Thanks |
SyntaxKind.UnsafeKeyword | ||
SyntaxKind.UnsafeKeyword, | ||
SyntaxKind.RefKeyword, | ||
SyntaxKind.ReadOnlyKeyword |
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.
💡 Include a ,
at the end of the list, so if we ever have to add items to the end of the list the diff doesn't need to show a change on this line (see e.g. how it would have been better if the line with UnsafeKeyword
did not need to change here).
@@ -53,6 +53,7 @@ internal static partial class SyntaxTreeExtensions | |||
case SyntaxKind.VolatileKeyword: | |||
case SyntaxKind.UnsafeKeyword: | |||
case SyntaxKind.AsyncKeyword: | |||
case SyntaxKind.RefKeyword: |
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.
💭 Why are InKeyword
, OutKeyword
, and ThisKeyword
not part of 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.
those are not considered modifiers in the sense of how this function is working. This is for the modifiers you get on a member or type decl. The method could have its name update accordingly though.
retest windows_debug_unit32_prtest please |
@jinujoseph for ask-mode approval for 15.8. Thanks |
1 similar comment
@jinujoseph for ask-mode approval for 15.8. Thanks |
Customer scenario
Type "public" then expect that you are offered a completion for "readonly", so you can type "public readonly struct". Prior to this PR, the completion was not offered.
Bugs this fixes
Fixes #26120
Workarounds, if any
You can always spell out "readonly" without completion help.
Risk & Performance impact
Low. This follows the same pattern used for "public" and other similar keyword recommenders.
Is this a regression from a previous update?
No. "readonly struct" is a new feature in 15.7.
Root cause analysis
This was missed in the IDE test pass.
How was the bug found?
Reported by customer.