Avoid reusing temps whose refs might be captured

[jjonescz]

Fixes #67435.

The idea is to have a "heuristic" to detect whether a method might capture the references passed to it. If such method call is detected, and a temporary reference is being emitted, we avoid freeing the temp so it lives during the whole method instead of just the expression.

The heuristic is implemented by CodeGenerator.MightEscapeTemporaryRefs. It runs on the lowered nodes (because it's the emit layer which decides to emit a temporary). It might have false positives (some calls like M(rvalue, out _) might be marked by the heuristic as dangerous but they are not), but it shouldn't have false negatives.

Without a heuristic, we would need to avoid reusing many more temps, which would be a regression (at least in IL size). But perhaps that's negligible and it would be better to avoid this complexity? I'm not sure.

[AlekseyTs]

MightEscapeTemporaryRefs

I will let Ref-Safety experts to make a call on completeness/correctness of the code in this method. However, it would be good to add comments about explaining the logic. #Resolved

[jaredpar]

Agree having a strategy comment would help. Not intuitive at a glance why we're using the approach of counting refs here.

[jjonescz]

The ref counters have comments and then they are used to compute the return value, also commented in the shouldReturnTrue local function. But I will add another higher-level comment.

[AlekseyTs]

Done with review pass (commit 1), tests are not looked at.

[AlekseyTs]

Done with review pass (commit 6)

[AlekseyTs]

LGTM (commit 9). However, this should not be considered as a sign-off on MightEscapeTemporaryRefs method. I think we should get a sign-off for its logic from two "ref safety" experts, at least.

[AlekseyTs]

_addressedLocals?.Add(localDef);

Should user defined locals be a subject for this treatment? Presumably ref safety for them was verified during binding. Let's add a unit test with a scenario affecting a user local.

[jaredpar]

my intuition was that this should impact temps only so share the concern about user defined locals.

[jjonescz]

This does not have any effect on user-defined locals - they are ignored by CodeGenerator.FreeLocal, i.e., they are never reused anyway, regardless of whether we add them to the _addressedLocals and _nonReusableLocals lists. But I could filter them here as well, i.e., avoid adding them to those lists in the first place if you think that would be better.

Let's add a unit test with a scenario affecting a user local.

Not sure which exact scenario you are looking for. There is for example RefEscapingTests.RefTemp_NoTemp added as part of this PR.

[AlekseyTs]

But I could filter them here as well, i.e., avoid adding them to those lists in the first place if you think that would be better.

Yes, I think it would be better to be explicit about handling of user-defined locals.

[jjonescz]

@dotnet/roslyn-compiler for a second review, thanks