Skip to content

Make IRecursivePatternOperation public #36709

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 1, 2019
Merged

Make IRecursivePatternOperation public #36709

merged 3 commits into from
Jul 1, 2019

Conversation

@gafter
Copy link
Member

@gafter gafter commented Jun 24, 2019

Fixes #35843

@gafter gafter added this to the 16.3 milestone Jun 24, 2019
@gafter gafter requested review from a team and 333fred June 24, 2019 18:55
@gafter gafter self-assigned this Jun 24, 2019
@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Jun 24, 2019

Awesome thanks! i can remove some ugly reflection code now :)

CyrusNajmabadi
CyrusNajmabadi approved these changes Jun 24, 2019
View reviewed changes
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.

Approve
Submit feedback approving these changes.

@333fred
Copy link
Member

333fred commented Jun 24, 2019 

    RecursivePattern = 0x67,

Wait, this was public? Oops.

Refers to: src/Compilers/Core/Portable/Operations/OperationKind.cs:237 in a84dc72. [](commit_id = a84dc72, deletion_comment = False)

333fred
333fred approved these changes Jun 24, 2019
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

API looks good to me, but we need to ensure that we're ok with it. Sent an email internally.

mavasani
mavasani reviewed Jun 24, 2019
View reviewed changes
src/Compilers/Core/Portable/PublicAPI.Unshipped.txt
Microsoft.CodeAnalysis.Operations.IPropertySubpatternOperation
Microsoft.CodeAnalysis.Operations.IPropertySubpatternOperation.Member.get -> Microsoft.CodeAnalysis.IOperation
Microsoft.CodeAnalysis.Operations.IPropertySubpatternOperation.Pattern.get -> Microsoft.CodeAnalysis.Operations.IPatternOperation
Microsoft.CodeAnalysis.Operations.IRecursivePatternOperation
Copy link
Contributor

@mavasani mavasani Jun 24, 2019

Choose a reason for hiding this comment

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

We should remove this IDE reflection based hack that was added for this API: http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/Extensions/OperationExtensions.cs,64

Copy link
Contributor

@mavasani mavasani Jun 24, 2019

Choose a reason for hiding this comment

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

case IOperation iop when iop.GetType().GetInterfaces().Any(i => i.Name == "IRecursivePatternOperation"):

mavasani
mavasani approved these changes Jun 24, 2019
View reviewed changes
Copy link
Contributor

@mavasani mavasani left a comment

Choose a reason for hiding this comment

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

LGTM, @gafter let me know if you can update the PR to remove the reflection based workaround in the IDE layer for this API. Otherwise, please file an IDE issue to track that change. Thanks!

@333fred
Copy link
Member

333fred commented Jun 25, 2019

@dotnet/roslyn-compiler for a second review.

@gafter gafter mentioned this pull request Jun 28, 2019
Closed
@gafter
Copy link
Member Author

gafter commented Jun 28, 2019

@dotnet/roslyn-compiler May I please have a second review from the compiler team?

@jcouv
Copy link
Member

jcouv commented Jun 28, 2019

Looking

jcouv
jcouv approved these changes Jun 28, 2019
View reviewed changes
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 3)

@jcouv jcouv self-assigned this Jun 28, 2019
@gafter gafter merged commit 3bd6f79 into dotnet:master Jul 1, 2019
mavasani added a commit to mavasani/roslyn that referenced this pull request Jul 10, 2019
@mavasani
Remove reflection based workaround for IRecursivePatternOperation
1813a4d 
The API was made public in dotnet#36709, so we don't need a reflection based workaround

Fixes dotnet#36866
@mavasani mavasani mentioned this pull request Jul 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tmat tmat tmat approved these changes

@333fred 333fred 333fred approved these changes

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi approved these changes

@jcouv jcouv jcouv approved these changes

+1 more reviewer

@mavasani mavasani mavasani approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@gafter gafter

@jcouv jcouv

Labels

Area-Compilers Feature - Pattern Matching Pattern Matching

Projects

None yet

Milestone

16.3

Development

Successfully merging this pull request may close these issues.

Make IRecursivePatternOperation public

6 participants

@gafter @CyrusNajmabadi @333fred @jcouv @tmat @mavasani