Clear hoisted locals when disposing iterator and async-iterator state machines

[jcouv]

Fixes #75666

Background

We already have logic to clear unmanaged hoisted locals when we exit scopes. But that logic has a number of limitations:

1. we don't clear top-level locals on normal exit
2. we don't clear any locals on yield break
3. we don't clear any locals on early exit (from yield return, ie. foreach (var i in coll) { break; })
4. we don't clear any locals on throw

static IEnumerable<int> Produce()
{
    {
        string hoistedNestedLocal = "";
        // existing clearing logic doesn't work for:
        // `yield break`
        // `yield return` and the caller breaks `foreach (var i in coll) { break; }`
        // `throw`
    }

    string hoistedTopLevelLocal = "";
    // existing clearing local doesn't work for:
    // normal exit (because a `yield break` is injected, see `FlowAnalysisPass.AppendImplicitReturn`, so the cleanup is added after a `yield break`)
    // `yield break`
    // `yield return` and the caller breaks `foreach (var i in coll) { break; }`
    // `throw`
}

So the current design only works for nested locals on normal exit from their scope.

Change

This PR clears the hoisted fields for all the locals (top-level and nested) in the part of the code that handles disposal: in Dispose method for iterators and at the end of MoveNext for async-iterators.
There is no need to add such clearing for async state machines, as it is non-trivial for a user to get a direct reference to those.

The proposed design fixes scenarios 1, 2, 3, and most of 4 (a throw during disposal still results in uncleared state).
This remains "best effort", as we don't want to add the overhead of an additional try/finally to handle that last scenario.

Look at the diff between commit 1 (baseline) and commit 2 (code change) to see the expectedOutput and IL changes.

[AlekseyTs]

We need to clean nested hoisted local variables too (not just top-level ones)

This isn't obvious. It looks like there are two call sites of this method and it is not clear why we want to have this behavior for all of them. Instead of completely ignoring the passed argument, consider adjusting the call sites to construct the right set of locals to clear. We can discuss offline in more details. #Closed

[AlekseyTs]

This behavior could be a source of bugs in the future, since the method doesn't do what a reasonable dev would expect it to do.

[AlekseyTs]

GenerateAllHoistedLocalsCleanup

This code looks very different from original implementation of GenerateHoistedLocalsCleanup. Is it based on some other existing code? Also, isn't there a more direct and robust way to enumerate fields used for hoisting? I mean without going through locals, interpreting their replacements, etc. If not, perhaps we can build the list in advance, when the fields are added. #Closed

[AlekseyTs]

GenerateAllHoistedLocalsCleanup(),

Are there tests observing effect of this on IL? #Closed

[jcouv]

Yes, see the diff between commits 1 and 2 for VerifyIL differences.
This change affects all iterator scenarios without try/finally (such as AddVariableCleanup_StringLocal), whereas the change below affects scenarios with try/finally (such as AddVariableCleanup_NestedStringLocal_InTryFinally)

[AlekseyTs]

GenerateAllHoistedLocalsCleanup(),

Are there tests observing effect of this on IL? #Closed

[jcouv]

Yes. See AddVariableCleanup_NestedStringLocal_InTryFinally

[AlekseyTs]

Done with review pass (commit 5)

[jcouv]

    protected BoundCatchBlock GenerateExceptionHandling(LocalSymbol exceptionLocal, ImmutableArray<StateMachineFieldSymbol> hoistedLocals)

📝 consider renaming or making a local function, consider commenting. This is only for the "master catch" #Closed

---

Refers to: src/Compilers/CSharp/Portable/Lowering/AsyncRewriter/AsyncMethodToStateMachineRewriter.cs:227 in 386a056. [](commit_id = 386a056, deletion_comment = False)

[AlekseyTs]

GenerateHoistedLocalsCleanupForExit

Consider removing "HoistedLocals" from the name to completely avoid any confusion that the clean up is limited to the locals in the rootHoistedLocals parameter.

        protected virtual BoundStatement GenerateCleanupForExit(ImmutableArray<StateMachineFieldSymbol> rootHoistedLocals)
``` #Resolved

[AlekseyTs]

Add

I think we have an AddRange helper to add all items from nonReusableFieldsForCleanup in one call #Resolved

[AlekseyTs]

bool isManaged = field.Type.IsManagedType(ref useSiteInfo);

This is a second change like that in this PR. Is it a fix for a different issue? It might be better to keep it separate, in case we need to back out one of the changes, etc.

[AlekseyTs]

This shows as an unnecessary empty line between an attribute and the target unit-test. #Resolved

[AlekseyTs]

LGTM (commit 19)

[jcouv]

@dotnet/roslyn-compiler for second review. Thanks

[jaredpar]

@333fred ptal

[jcouv]

@333fred for a second review. Thanks

[333fred]

Is there a version of this with a throw in the try? #Resolved

[jcouv]

No, the only test with a throw in the try is AddVariableCleanup_NestedStringLocal_InTryFinally_WithThrow_EarlyIterationExit.