[Demetrios Chiuratto Agourakis (agourakis82)]

Summary

Addresses the original reproducer and the loop-local array variant in #3940.

Partial evaluation was carrying qubit releases from one dynamic branch into the alternate path. The patch snapshots qubit resource state before the branch, restores it for the alternate path, and merges the live resources afterward. Liveness checks consult the active resource map rather than relying on references retained by snapshots.

The follow-up variant also exposed an unreachable cleanup suffix after an unconditional return in a loop. Return unification now stops rewriting a unit-valued block after a statement that unconditionally returns. Detection follows only straight-line nested blocks; a return inside a conditional arm or nested loop does not establish that the enclosing statement returns.

The regression tests cover the original reproducer, the exact variant, a range-loop variant, and controls without an early return, without a loop, and with a single qubit. Six existing snapshots are updated only to remove unreachable statements.

Validation of 2c666a4

Native Linux cluster checks with Rust 1.95.0:

• cargo test --locked -p qsc --lib issue_3940_ -- --test-threads=4: 6 passed.
• cargo test --locked -p qsc_fir_transforms --lib return_unify -- --test-threads=4: 463 passed.
• cargo test --locked -p qsc_partial_eval --lib tests::qubits -- --test-threads=4: 13 passed, including the dynamic-branch use-after-release regression.
• Formatting checks on the changed files and git diff --check passed.

With the same regression tests on preceding head 876554c, the maintainer variant and range-loop variant panic; the other four pass. With this follow-up, all six pass. These are focused compiler checks, not a claim that the full workspace suite or GitHub CI has passed on this head.

Prepared with Codex assistance. The return-lowering candidate also received a separate coder-next review; executable validation and final review remained part of the contribution workflow.

Additional return/cleanup regression coverage

Commit 6c50b68 adds six Q#-to-QIR compilation tests and one structural FIR test for return-operand effects, nested scalar/array cleanup, conditional fallthrough and possibly zero-iteration loops. Production code and the original early-return regressions remain unchanged by this follow-up.

All seven new tests pass. Scoped Clippy with warnings denied, formatting and whitespace checks pass. The existing early-return test filters also pass (16 qsc and 19 FIR-transform tests); these local checks are separate from hosted CI. The structural test checks operand evaluation before inner/array/outer cleanup and the return flag, without claiming a general semantic proof.

The maintainer's return-handling and snapshot-lifetime questions remain open for review. The investigation and additional compiler tests were prepared with OpenAI Codex assistance.

[Demetrios Chiuratto Agourakis (agourakis82)]

@microsoft-github-policy-service agree

[Demetrios Chiuratto Agourakis (agourakis82)]

The workflows for this PR are awaiting maintainer approval. Could you approve the pending runs so the change can receive build and test validation?

[Andrew Casey (amcasey)]

Demetrios Chiuratto Agourakis (@agourakis82) Thanks for this! We're pausing a lot of compiler changes until this monster PR stack goes in: #3961. Thanks for your patience!

[Demetrios Chiuratto Agourakis (agourakis82)]

Andrew Casey (@amcasey) Thanks for the update. #3961 is now merged, and this branch includes the subsequent main update at 3feb87cd3fbd.

The current CI Build and Test run is awaiting approval (action_required). Could you approve it and resume review when convenient?

Prepared with Codex assistance.

[Andrew Casey (amcasey)]

Does this address the variant repro of #3940? Maybe that's a related but distinct issue?

[Demetrios Chiuratto Agourakis (agourakis82)]

No — I checked your exact variant on 3feb87cd3fbd, and it still panics with qubit should still be alive at qsc_eval/src/val.rs:151.

With the same native Q#-to-QIR test harness, the original indexed-array reproducer passes on the PR while your variant fails. Against base 3977495f819c, both fail.

The variant's backtrace reaches ResourceManager::release_qubit through array release and a generated dynamic branch inside the lowered loop. So I cannot yet conclude that it is an unrelated issue. I have removed the automatic closing reference from the description: this PR addresses the original reproducer, but does not cover all of #3940.

Investigation and reply prepared with Codex assistance.

[Andrew Casey (amcasey)]

Is this going to keep Qubits alive longer than they used to be? I think there's behavior associated with their deallocation.

[Andrew Casey (amcasey)]

Via copilot

#[test]
fn released_qubit_inside_dynamic_branch_reports_error() {
    let error = get_partial_evaluation_error(indoc! {
        r#"
        namespace Test {
            operation Op(q : Qubit) : Unit { body intrinsic; }

            @EntryPoint()
            operation Main() : Unit {
                use control = Qubit();
                let q = QIR.Runtime.__quantum__rt__qubit_allocate();

                if MResetZ(control) == One {
                    QIR.Runtime.__quantum__rt__qubit_release(q);
                    Op(q);
                }
            }
        }
        "#,
    });

    assert!(matches!(
        error,
        crate::Error::EvaluationFailed(message, _)
            if message == "qubit used after release"
    ));
}

[Demetrios Chiuratto Agourakis (agourakis82)]

The snapshot does extend the Rc payload lifetime while the alternate path is evaluated. Qubit has no custom Drop implementation; logical resource release is explicit in free_qubit_id. A Weak reference can therefore still upgrade after logical release.

The map-only guard needed one more condition when an ID is recycled. In 015eb55, the guard requires both an active ID and Rc::ptr_eq with the allocation in qubit_tracker. On 2c666a4, regressions that release q, allocate fresh with the recycled ID, and then use q are accepted in both the true and else branches. With this change they report qubit used after release, while the fresh-allocation control remains valid.

Native Q#-to-IR checks passed: 17 qubit tests, 31 branching tests, and six #3940 cases including the original reproducer, your loop-local variant and four controls. The source hashes match the isolated compiler build. This validates these liveness cases; the snapshot still retains strong Rc ownership.

Does this address the lifetime concern, or is there another deallocation-dependent path you would like covered?

Investigation, patch and reply prepared with Codex assistance.

[Demetrios Chiuratto Agourakis (agourakis82)]

The variant is now covered in 2c666a4.

It exposed an unreachable cleanup suffix left after an unconditional return in the loop body. Partial evaluation visited that suffix and released the same array twice. The follow-up removes such suffixes from unit-valued blocks, following only straight-line nested blocks. It preserves qubit lifetime checks and the changes in 876554c.

On 876554c, your exact variant and a range-loop variant both panic. With the follow-up, all six focused cases pass. The return-unification suite passes all 463 tests, and all 13 qubit-management tests pass. Six snapshots changed only to remove unreachable statements. GitHub CI still needs to run on the new head.

Prepared with Codex assistance.

[Demetrios Chiuratto Agourakis (agourakis82)]

Andrew Casey (@amcasey) Thanks for the careful review. I’ve pushed two focused follow-ups to address the comments and the loop-local variant of #3940.

In 876554ca9:

• Renamed QubitState to QubitStateSnapshot.
• Kept the qubit snapshot save/restore adjacent to the static var-map save/restore. The snapshot is restored immediately after the cached var map, before evaluating the otherwise branch.
• Kept the resource-state merge after the if/else: the implicit false path still participates in the resource-state join when there is no otherwise branch, so moving the merge inside otherwise would discard state from the true path.
• Kept Rc<Qubit> ownership only in the short-lived snapshot, since restoring the alternate path requires restoring the same qubit identity. Logical liveness is now checked against the current qubit_id_map, so a released qubit cannot be made usable merely because the snapshot still owns its Rc. The merge consumes the snapshot to avoid an additional tracker clone.

The regression test released_qubit_inside_dynamic_branch_reports_error now reports qubit used after release instead of panicking. The follow-up 2c666a480 adds six native QIR regressions for the loop-local qubit-array variant and removes unreachable cleanup suffixes after unconditional returns.

Validation on the current branch head (2c666a480):

• cargo +1.96.0 test -p qsc --lib issue_3940_: 6 passed
• cargo +1.96.0 test -p qsc_partial_eval --lib released_qubit_inside_dynamic_branch_reports_error: passed
• cargo +1.96.0 fmt --all -- --check: passed

Please take another look when convenient.

[Andrew Casey (amcasey)]

Demetrios Chiuratto Agourakis (@agourakis82) Thanks for the updates! Just a heads up - I'll be out for a few days. Hopefully, someone will be able to pick it up while I'm gone, but I can't promise anything.

[Andrew Casey (amcasey)]

Demetrios Chiuratto Agourakis (@agourakis82) Both CI failures are qubit used after release, so possibly related to your change. Please have a look.

[Demetrios Chiuratto Agourakis (agourakis82)]

Thanks for flagging this. I've checked the logs: both failing tests exercise early returns in dynamic branches and report qubit used after release. I'll trace where the qubit state diverges in those cases before changing the implementation. There is also a separate Clippy failure in return_unify/lower.rs requesting let … else.

[Demetrios Chiuratto Agourakis (agourakis82)]

I've pushed the fix and integrated current main. Both failures came from live qubits supplied through closure captures: the new partial evaluator had not registered those inputs before checking their liveness.

Input registration now happens before evaluating the body. The stricter checks remain in place afterward, and a new regression confirms that an external strong reference cannot hide a release followed by numeric-ID reuse.

Both reported tests pass locally, along with 541 qsc tests, 439 qsc_partial_eval tests, 471 return_unify tests, and Clippy for the three affected packages. The new CI run is awaiting approval.

[Andrew Casey (amcasey)]

I've pushed the fix and integrated current main. Both failures came from live qubits supplied through closure captures: the new partial evaluator had not registered those inputs before checking their liveness.

Input registration now happens before evaluating the body. The stricter checks remain in place afterward, and a new regression confirms that an external strong reference cannot hide a release followed by numeric-ID reuse.

Both reported tests pass locally, along with 541 qsc tests, 439 qsc_partial_eval tests, 471 return_unify tests, and Clippy for the three affected packages. The new CI run is awaiting approval.

I'm probably missing something obvious, but I only see tests and comments in your last commit.

[Demetrios Chiuratto Agourakis (agourakis82)]

You're right — bbb6cb9 adds the regression test and explanatory comments. The implementation change is in the preceding merge commit, ed566fd, where I also integrated main.

Specifically, PartialEvaluator::invoke registers qubits in the supplied arguments before evaluating the callable, and ResourceManager::register_input_qubit registers their identities and slots. The existing liveness checks still apply after that. Sorry for not making the split between the two commits clear.

[Andrew Casey (amcasey)]

I'm a bit nervous about the new return handling. I'm pretty sure there's a reason we weren't already doing that. I'll try to confirm

[Andrew Casey (amcasey)]

Nit: we don't usually include issue numbers in test names. The test should make sense outside the context of a particular bug.

[Andrew Casey (amcasey)]

Incidentally, what does this test? I guess it used to panic?

[Demetrios Chiuratto Agourakis (agourakis82)]

Renamed all six tests in abe8816 to describe the behavior without an issue number.

The array-without-loop test you commented on is a compilation control, not the former panic reproducer. The regression is now named adaptive_loop_with_indexed_qubit_array_and_early_return_compiles: it exercises indexed qubit-array access in an Adaptive loop with an early return, which previously panicked with qubit should still be alive. I added that explanation next to the test. The controls distinguish array indexing, loop shape, and early-return handling.

Validation: all 73 codegen::tests::adaptive_ tests pass, including these six; formatting passes.

Prepared with Codex assistance.

[Andrew Casey (amcasey)]

This isn't obviously testing numeric ID reuse

[Demetrios Chiuratto Agourakis (agourakis82)]

Agreed: the integration test did not directly establish that the numeric ID was reused. In abe8816, imported_qubit_preserves_identity_across_release_and_reuse now explicitly asserts that the replacement has the original numeric ID and that Rc::ptr_eq is false for the two allocations. It then checks that the replacement is live and the released original is not, followed by the snapshot-restoration checks.

As a discriminating check, I temporarily reduced is_qubit_live to the active-ID-map check alone. The test passed the new ID/identity assertions and then failed at !manager.is_qubit_live(&original) after reallocation. Restoring the allocation-identity check makes all three input-qubit tests pass.

The callable integration test still passes and now describes its narrower role: checking the use-after-release diagnostic through QIR generation. Its comment points to the direct reuse assertions in the resource-manager test.

Prepared with Codex assistance.

[Demetrios Chiuratto Agourakis (agourakis82)]

I traced the suffix rule through allocation lowering and published seven additional compiler tests in 6c50b68 for operand effects, nested scalar/array cleanup, conditional fallthrough, and a possibly zero-iteration loop. Production code and the original early-return regressions are unchanged by this follow-up.

Allocation lowering captures the operand and releases active scopes before the Return. The suffix rule removes the later duplicate suffix only in Unit blocks, following direct returns and straight-line blocks. The structural FIR test checks Value(inner) before inner/array/outer cleanup and the return flag.

Disabling only that trimming rule reproduces qubit should still be alive in the existing loop-local array regression; restoring it passes. These checks do not prove general correctness or settle the return-handling and lifetime concerns. All seven new tests, scoped Clippy and formatting pass locally; hosted CI remains a separate approval gate.

Investigation and additional tests prepared with OpenAI Codex assistance.

[Demetrios Chiuratto Agourakis (agourakis82)]

Andrew Casey (@amcasey) Could you re-review head 6c50b68 and approve the pending workflows when convenient? The current-head runs remain at action_required, including Build and Test.

The latest follow-up adds targeted return/cleanup tests; the implementation and the earlier release/reuse regressions remain in the preceding commits. The source analysis and validation limits are recorded above. Your concern about the return transformation and qubit lifetime remains open for review; the additional tests are not a claim of general correctness.

Prepared with OpenAI Codex assistance.

[Andrew Casey (amcasey)]

Demetrios Chiuratto Agourakis (@agourakis82) We haven't forgotten about you! We're just trying to get somethings straightened out before our next release (some of which overlap with this change).

So that it doesn't come as a surprise later, I'll mention that while the first part of your change (state snapshots) seemed pretty straightforward, the early return change makes me much more nervous since it's a change to our historical approach (possibly an improvement, but nevertheless a change), so we're going to have to think about it in the context of a larger change (which almost certainly won't meet the bar for the current release).

Thanks for your patience!