Preserve newobj when rewritten targets remain instance constructors #528

Summary

Preserve newobj when type rewriting changes a constructor reference but its resolved target remains a real instance constructor. Emit call only when the replacement is a static factory.

This is a general rewriter correction proposed as a prerequisite to #527, not a request to merge that PR unchanged. The unsafe assumption predates #527; its new configured-type mappings expose a caller-visible regression. Identical probes pass the tested rewritten main baseline and fail the PR head.

Exact reviewed versions and environment

• PR: Fix rewriting of configured async enumeration and disposal #527, "Fix rewriting of configured async enumeration and disposal".
• Baseline: aabd3b477500977d43c28d89b993476438ec7ea9.
• Failing head: 39e5761c1c6a0136a9bb4825f60c2c631cdda251.
• Reproduced during adversarial review on Windows x64, .NET 8.0.32, SDK 8.0.426.
• These results concern those exact commits, not an assertion about later main.

Root cause and code pointers

The new mappings in TypeRewritingPass.cs:76-81 cause generic constructor declaring types to be rewritten when their type arguments include ConfiguredCancelableAsyncEnumerable<T> or ConfiguredAsyncDisposable.

MethodBodyTypeRewritingPass.VisitNewobjInstruction:115-125 then unconditionally emits OpCodes.Call for a rewritten reference, assuming the replacement is a factory. It can actually remain an instance .ctor.

Baseline IL:

newobj System.Void Box`1<System.Runtime.CompilerServices.ConfiguredCancelableAsyncEnumerable`1<System.Int32>>::.ctor(T)

PR-head IL:

call System.Void Box`1<Microsoft.Coyote.Rewriting.Types.Runtime.CompilerServices.ConfiguredCancelableAsyncEnumerable`1<System.Int32>>::.ctor(T)
stloc.2

The call has no allocated receiver and produces no object result for the store. The CLR rejects the containing method with InvalidProgramException, before any configured await occurs.

Reproducer

In an isolated checkout of each exact version, add this xUnit class to Tests\Tests.BugFinding. Build through the normal project targets so the test assembly is rewritten before running it. Do not switch a shared active worktree.

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using Xunit;

public class AdversarialConfiguredAsyncEnumerableTests
{
    [Fact]
    public void TestConfiguredEnumerableInGenericConstructor()
    {
        var configured = new EmptyEnumerable().WithCancellation(default);
        var box = new Box<ConfiguredCancelableAsyncEnumerable<int>>(configured);
        Assert.NotNull(box);
        box.Value.GetAsyncEnumerator();
    }

    [Fact]
    public void TestConfiguredDisposableInGenericConstructor()
    {
        var configured = ((IAsyncDisposable)new EmptyEnumerable()).ConfigureAwait(false);
        var box = new Box<ConfiguredAsyncDisposable>(configured);
        Assert.NotNull(box);
    }

    [Fact]
    public void TestConfigurationMethodGroup()
    {
        Func<IAsyncEnumerable<int>, CancellationToken, ConfiguredCancelableAsyncEnumerable<int>> configure =
            TaskAsyncEnumerableExtensions.WithCancellation<int>;
        configure(new EmptyEnumerable(), default).GetAsyncEnumerator();
    }

    [Fact]
    public void TestCustomConfigurationMethodGroup()
    {
        using var cancellation = new CancellationTokenSource();
        var source = new EmptyEnumerable();
        ConfigureCallback configure = TaskAsyncEnumerableExtensions.WithCancellation<int>;
        configure(source, cancellation.Token).GetAsyncEnumerator();
        Assert.Equal(cancellation.Token, source.ObservedToken);
    }

    private delegate ConfiguredCancelableAsyncEnumerable<int> ConfigureCallback(
        IAsyncEnumerable<int> source, CancellationToken cancellationToken);

    private sealed class Box<T>
    {
        internal T Value { get; }
        internal Box(T value) { this.Value = value; }
    }

    private sealed class EmptyEnumerable : IAsyncEnumerable<int>, IAsyncEnumerator<int>
    {
        public int Current => 0;
        internal CancellationToken ObservedToken { get; private set; }
        public IAsyncEnumerator<int> GetAsyncEnumerator(CancellationToken cancellationToken = default)
        {
            this.ObservedToken = cancellationToken;
            return this;
        }
        public ValueTask<bool> MoveNextAsync() => new ValueTask<bool>(false);
        public ValueTask DisposeAsync() => default;
    }
}

PowerShell commands used for the focused reproduction:

dotnet build Tests\Tests.BugFinding\Tests.BugFinding.csproj -c Release -f net8.0 -p:TargetFrameworks=net8.0
dotnet test Tests\Tests.BugFinding\Tests.BugFinding.csproj -c Release -f net8.0 --no-build --no-restore --filter FullyQualifiedName~AdversarialConfiguredAsyncEnumerableTests

Observed differential results

• Exact PR head: the two Box<T> cases and the Func constructor case fail with System.InvalidProgramException: Common Language Runtime detected an invalid program.
• Exact baseline: identical three cases pass.
• The non-generic custom-delegate control passes at unmodified head; it does not establish a separate function-pointer defect.
• All 13 original ConfiguredAsyncEnumerableTests cases pass at head, so existing tests miss this regression.
• A diagnostic constructor-opcode correction alone passed all 17 focused cases: the four probes above and the 13 original cases. The baseline passed all four probes.
• Builds for the diagnostic reproduction reported zero warnings/errors.

The Func method-group failure is another trigger of the same constructor bug, not a second finding. The helper is synchronous, empty, and local; this is not an unsupported external async source or a configured-ValueTask await failure.

Suggested implementation

Resolve the rewritten target and preserve constructor allocation semantics. The diagnostic correction changed the discarded resolved definition to resolvedMethod, then used:

Instruction newInstruction = Instruction.Create(resolvedMethod.IsConstructor ?
    OpCodes.Newobj : OpCodes.Call, newMethod);

Treat this as a verified starting point, not a substitute for reviewing the complete dispatch contract. Preserve existing resolution-failure handling and static-factory replacement behavior.

Acceptance criteria

• Real instance constructor replacements retain newobj and allocate/return a valid object.
• Existing constructor-to-static-factory rewrites still use call and continue working.
• Add permanent rewriter regression tests for generic constructors whose type arguments are rewritten, following existing test organization and naming.
• Preserve configured-type integration coverage from the reproducer when Fix rewriting of configured async enumeration and disposal #527 is rebased onto this correction; include configured enumerable, nested enumerator, disposable, and generic delegate construction as appropriate. The nested-enumerator case is requested coverage, not an already-reproduced failure.
• Demonstrate failing-before/passing-after behavior on the relevant exact source and run focused tests plus repository rewriter validation.
• Inspect generated IL and explain any expected IL-baseline changes; do not blindly update hashes.

If delivered as a separate prerequisite PR, keep the general rewriter fix and tests there, and the new configured-type mappings/support and their integration tests in #527. Do not add those mappings to main merely to make the prerequisite test reproduce.

Scope and delivery state

No production fix or permanent regression tests were delivered by the review. Diagnostic changes and scratch probes were removed. The source and reproduction above are self-contained; private review artifacts are not required to implement the fix.

The separately observed Windows CI Tests.BugFinding IL-hash mismatch is not proof of this runtime defect and should not be conflated with it: CI job.