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

Add extract method support for ref struct interfaces #73619

Merged
merged 3 commits into from
May 22, 2024

Conversation

ToddGrun
Copy link
Contributor

The method extraction code uses ITypeParameterSymbol's constraints to generate the new method's text. This codepath had not yet been modified to support ITypeParameterSymbol.AllowsRefLikeType

This is in support of the "allows ref struct" on interfaces feature outlined here: #72124

This ref structs for interfaces feature was merged via this PR: #73567

The method extraction code uses ITypeParameterSymbol's constraints to generate the new method's text. This codepath had not yet been modified to support ITypeParameterSymbol.AllowsRefLikeType

This is in support of the "allows ref struct" on interfaces feature outlined here: #72124

This ref structs for interfaces feature was merged via this PR: #73567
@ToddGrun ToddGrun requested a review from AlekseyTs May 21, 2024 20:39
@ToddGrun ToddGrun requested a review from a team as a code owner May 21, 2024 20:39
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 21, 2024
@@ -18,6 +18,7 @@
bool hasConstructorConstraint,
bool hasReferenceConstraint,
bool hasValueConstraint,
bool allowsRefLikeType,
Copy link
Contributor

@AlekseyTs AlekseyTs May 21, 2024

Choose a reason for hiding this comment

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

bool allowsRefLikeType,

It feels fragile to add a new bool parameter between existing bool parameters. Easy to make a mistake while passing arguments. I would add it as the last bool parameter, right before the ordinal. #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.

re-ordered for the new parameter to be the last bool param

@@ -337,10 +338,11 @@
bool hasReferenceConstraint = false,
bool hasUnmanagedConstraint = false,
bool hasValueConstraint = false,
bool allowsRefLikeType = false,
Copy link
Contributor

@AlekseyTs AlekseyTs May 21, 2024

Choose a reason for hiding this comment

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

bool allowsRefLikeType = false,

Similar comment about order of parameters #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

Especially that in CodeGenerationTypeParameterSymbol it comes before hasUnmanagedConstraint parameter.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 1)

@@ -322,7 +322,8 @@ public static ITypeParameterSymbol CreateTypeParameterSymbol(string name, int or
attributes: default, varianceKind: VarianceKind.None,
name: name, constraintTypes: [],
hasConstructorConstraint: false, hasReferenceConstraint: false, hasValueConstraint: false,
hasUnmanagedConstraint: false, hasNotNullConstraint: false, ordinal: ordinal);
allowsRefLikeType: false, hasUnmanagedConstraint: false, hasNotNullConstraint: false,
Copy link
Contributor Author

@ToddGrun ToddGrun May 21, 2024

Choose a reason for hiding this comment

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

allowsRefLikeType: false,

missed this one, fixing #Resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE 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.

4 participants