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

Handle ref returning receivers in dynamic calls #70676

Merged
merged 6 commits into from
Nov 9, 2023

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Nov 2, 2023

Fixes #68063.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 2, 2023
@jjonescz jjonescz marked this pull request as ready for review November 2, 2023 16:32
@jjonescz jjonescz requested a review from a team as a code owner November 2, 2023 16:32
jaredpar
jaredpar previously approved these changes Nov 2, 2023
@jjonescz jjonescz requested a review from a team November 3, 2023 08:15
@@ -549,6 +549,9 @@ internal static RefKind GetReceiverRefKind(BoundExpression loweredReceiver)

switch (loweredReceiver.Kind)
{
case BoundKind.Call:
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 3, 2023

Choose a reason for hiding this comment

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

case BoundKind.Call:

Are we missing handling for other interesting scenarios? For example, I do not see explicit case for fields. We probably could run into a sequence (for example if we need to reorder arguments for a call), etc.
It looks like the purpose of this method is to determine whether we can take a reference to the receiver without storing the value in a temp. It feels fragile to have a helper that is used only for the purpose of dynamic rewrite, especially taking future language changes into account. Scenarios involving dynamic rewrite are easy to miss during testing. Therefore, I think we should either switch to a different existing helper, which is used more often (Binder.HasHome?), or we could try always pass value type receiver by reference and let emit layer to figure out if it needs a temp for that (it looks like that shouldn't change semantics of the operation). #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we missing handling for other interesting scenarios?

Good catch, thanks.

we could try always pass value type receiver by reference and let emit layer to figure out if it needs a temp for that

That seems like it would work except it's hitting the following assert which I'm not sure how to easily change (apart from removing it).

Debug.Assert(argument.Type.IsDynamic(), "passing args byref should not clone them into temps");

It would also change IL (introducing temp for rvalue struct receivers), so I'm exploring the first option instead (using Binder.HasHome).

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

var hasHome = Binder.HasHome(loweredReceiver,
Binder.AddressKind.Writeable,
_factory.CurrentFunction,
_factory.Compilation.IsPeVerifyCompatEnabled,
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 6, 2023

Choose a reason for hiding this comment

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

_factory.Compilation.IsPeVerifyCompatEnabled

I think we don't need to depend on this setting here. The most permissive value is probably going to be the closest match with the previous behavior. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Or, at least, with intent.

{
dynamic d = 1;
GetS().M(d);
System.Console.Write(GetS().X);
Copy link
Contributor

@AlekseyTs AlekseyTs Nov 6, 2023

Choose a reason for hiding this comment

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

System.Console.Write(GetS().X);

It looks like this line isn't meaningful. It is guaranteed to produce the same output regardless of what the previous line is doing. #Closed

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 4)

@jjonescz jjonescz dismissed jaredpar’s stale review November 6, 2023 17:47

Substantial changes

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

@jjonescz jjonescz requested review from jaredpar and a team November 7, 2023 09:24
@jjonescz jjonescz enabled auto-merge (squash) November 9, 2023 10:36
@jjonescz jjonescz merged commit 2d08090 into dotnet:main Nov 9, 2023
25 checks passed
@jjonescz jjonescz deleted the 68063-DynamicOpFactoryRefKind branch November 9, 2023 11:56
@ghost ghost added this to the Next milestone Nov 9, 2023
@RikkiGibson RikkiGibson modified the milestones: Next, 17.9 P2 Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers 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.

It looks like LoweredDynamicOperationFactory.GetReceiverRefKind hasn't been adjusted for ref returns
4 participants