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

Remove unnecessary tracking of unmatched parameters in method invocation escape analysis #62847

Merged
merged 2 commits into from
Jul 26, 2022

Conversation

cston
Copy link
Member

@cston cston commented Jul 21, 2022

Fixes #62834.

@cston cston marked this pull request as ready for review July 22, 2022 00:51
@cston cston requested a review from a team as a code owner July 22, 2022 00:51
@cston
Copy link
Member Author

cston commented Jul 22, 2022

@dotnet/roslyn-compiler, please review.

@RikkiGibson RikkiGibson self-assigned this Jul 22, 2022
@@ -1881,21 +1853,40 @@ void updateEscapeTo(BoundExpression argument, RefKind refKind, uint scopeOfTheCo
};
}

#if DEBUG
private static bool AllParametersHaveArguments(
Copy link
Contributor

@RikkiGibson RikkiGibson Jul 22, 2022

Choose a reason for hiding this comment

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

It looks like we don't create arguments for params arrays during binding. #49602

Consider testing a case like:

using System;

Span<int> M1(Span<int> input, params object[] arr) { return input; }

Span<int> M2()
{
    return M1(stackalloc int[] { 1, 2, 3 });
}

We might want to adjust this step to ensure that all parameters which may factor into escape analysis have arguments. Or just specifically permit a parameter with IsParams == true && Type.TypeKind == TypeKind.Array here, since that's likely the only kind we will miss.
#Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thanks.

@cston
Copy link
Member Author

cston commented Jul 26, 2022

@dotnet/roslyn-compiler, please review a small change to escape analysis, to simplify changes going forward.

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 2)

@jcouv jcouv self-assigned this Jul 26, 2022
@cston cston merged commit dd6f30e into dotnet:main Jul 26, 2022
@cston cston deleted the unmatched-params branch July 26, 2022 22:16
@ghost ghost added this to the Next milestone Jul 26, 2022
@allisonchou allisonchou modified the milestones: Next, 17.4 P1 Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove tracking of unmatched in parameters in GetInvocationEscapeScope() and CheckInvocationEscape()
5 participants