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

Implement keyword recommenders for allows ref struct constraint #73463

Merged

Conversation

AlekseyTs
Copy link
Contributor

No description provided.

@AlekseyTs AlekseyTs requested review from a team as code owners May 14, 2024 03:31
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label May 14, 2024

namespace Microsoft.CodeAnalysis.CSharp.Completion.KeywordRecommenders;

internal class AllowsKeywordRecommender : AbstractSyntacticSingleKeywordRecommender
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
internal class AllowsKeywordRecommender : AbstractSyntacticSingleKeywordRecommender
internal sealed class AllowsKeywordRecommender : AbstractSyntacticSingleKeywordRecommender

if (token.Kind() == SyntaxKind.RefKeyword &&
token.Parent is RefStructConstraintSyntax refStructConstraint && refStructConstraint.RefKeyword == token &&
refStructConstraint.Parent is AllowsConstraintClauseSyntax allowsClause &&
allowsClause.Parent is TypeParameterConstraintClauseSyntax)
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi May 14, 2024

Choose a reason for hiding this comment

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

you can trim off this last check. #WontFix

{
if (!constraintClause.Constraints
.OfType<ClassOrStructConstraintSyntax>()
.Any(c => c.ClassOrStructKeyword.Kind() == SyntaxKind.ClassKeyword))
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi May 14, 2024

Choose a reason for hiding this comment

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

we'd be fine with this being dropped. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we'd be fine with this being dropped.

Following what we do for new constraint

public async Task TestAfterAllowsRefAfterClassTypeParameterConstraint()
{
await VerifyKeywordAsync(
@"class C<T> where T : class, allows ref $$");
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi May 14, 2024

Choose a reason for hiding this comment

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

could these all be raw strings plz. or indent 4 from await. we're trying to move away entirely from tests that are 0-column aligned. #Resolved

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

lgtm with minor nits, simplification suggestions, and requests about test formatting.

Copy link
Member

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

lgtm with minor nits, simplification suggestions, and requests about test formatting.

class C1<T>
where T : allows ref struct;
class C2<T, S>
where T : allows ref struct , where S : struct;
Copy link
Member

@jjonescz jjonescz May 14, 2024

Choose a reason for hiding this comment

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

The space before the comma looks unexpected. Shouldn't it be:

Suggested change
where T : allows ref struct , where S : struct;
where T : allows ref struct, where S : struct;
``` #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The space before the comma looks unexpected.

It is a syntax error, a comma cannot follow allows ref struct constraint

private static bool IsAllowsRefStructConstraintContext(CSharpSyntaxContext context)
{
// where T : |
if (context.SyntaxTree.IsTypeParameterConstraintStartContext(context.Position, context.LeftToken))
Copy link
Member

@jjonescz jjonescz May 14, 2024

Choose a reason for hiding this comment

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

I wouldn't expect this check to be inside method IsAllowsRefStructConstraintContext, consider returning it where it was previously. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll rename the method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers New Feature - RefStructInterfaces untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants