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

Fix pattern based arguments in ForEachLoopOperationInfo without binding assumptions #59887

Merged
merged 13 commits into from
Mar 7, 2022

Conversation

bernd5
Copy link
Contributor

@bernd5 bernd5 commented Mar 2, 2022

While looking for a new public IArgumentOperation API for pattern based calls (Dispose / GetEnumerator / MoveNext) I have seen that the non public class ForEachLoopOperationInfo is not filled correclty with IArgumentOperation.

This PR makes use of the new MethodArgumentInfo and fixes the missing IArgumentOperations in ForEachLoopOperationInfo (see the test cases).

This PR removes explicitly the IsExtensionMethod check - this should and is handled during binding - but not in the IOperationsFactory.

@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Mar 2, 2022
@bernd5 bernd5 changed the title Bugfix: fill ForEachLoopOperationInfo with all bound arguments Fix pattern based arguments in ForEachLoopOperationInfo without binding assumptions Mar 2, 2022
@bernd5 bernd5 marked this pull request as ready for review March 2, 2022 19:25
@bernd5 bernd5 requested a review from a team as a code owner March 2, 2022 19:25
@bernd5
Copy link
Contributor Author

bernd5 commented Mar 2, 2022

@333fred can you have a look?

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.

Done review pass (commit 3)

@bernd5
Copy link
Contributor Author

bernd5 commented Mar 3, 2022

@333fred good point - it is good to check especially the line values

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.

LGTM (commit 10). @AlekseyTs @dotnet/roslyn-compiler for a second review.

@333fred 333fred merged commit b8bd698 into dotnet:main Mar 7, 2022
@ghost ghost added this to the Next milestone Mar 7, 2022
@333fred
Copy link
Member

333fred commented Mar 7, 2022

Thanks for the contribution @bernd5!

@bernd5 bernd5 deleted the fix_ForEachLoopOperationInfoArguments2 branch March 9, 2022 07:05
@allisonchou allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants