[GrahamTheCoder]

Fixes icsharpcode#896
Fixes icsharpcode#1081

Both issues come from the same gap. When a ByRef argument needs a temp (a property, a method result, an omitted Optional ByRef), VB copies the temp back straight after the call. Roslyn's LocalRewriter_Call.vb lowers the call to a sequence: the call's value, then the copy-backs as side effects. The converter instead emits the write-back after the whole converted statement. For field initializers there is no statement at all.

Root causes

• VB -> C#: out param is set after return icsharpcode/CodeConverter#896, return/throw/yield: PerScopeState.CreateLocalsAsync adds write-backs (AdditionalAssignment) after the statement. After return or throw they can never run. After yield return they only run when the consumer resumes.
• Block headers (Select Case, Case, For start/bound, For Each, Using, SyncLock, With, Catch … When): the "statement" is the whole block, so the body saw the old value. For a later Case clause, the write-back ran even when the clause was never evaluated. NextStatementDefinitelyExecutedAfter treated these headers like plain statements. If/ElseIf and loop conditions were already handled.
• VB -> C#: Byref optional parameters to New field initializer can throw nullref icsharpcode/CodeConverter#1081: DeclarationNodeVisitor.CreateAdditionalLocalMembers read v.Initializer, which is null once an instance initializer has moved to the constructor. That threw a NullReferenceException. The same path dropped write-backs, left ph<guid> placeholders in constructors and initialX functions, and refused declarations with several variables or nested calls.
• Found along the way: a partial class's initialX() function (used when the constructors are in another part) was always static. So any initializer that read an instance member failed with CS0120. The existing multi-file characterization initialAnArrayWithNonStaticInitializerReferencingOtherPart is one example.

Output by case (progressive fallback; the existing simple output is unchanged)

VB	C#
F(P) as a statement, x = F(P), Dim x = F(P)	unchanged: temp, call, write-back after
any shape without a write-back (e.g. Return F(New X))	unchanged single statement
Return F(P)	int argv = P; long ret = F(ref argv); P = argv; return ret;
Throw CreateEx(P) / Yield F(P)	same shape, with throw ex; / yield return ret;
Select Case F(P), Case F(P), For i = 0 To F(P), For Each x In F(P), Using d = F(P), SyncLock F(P), With F(P), Catch When F(P)	the existing local-function fallback (as for If): int localF() { int argv = P; var ret = F(ref argv); P = argv; return ret; }, then switch (localF()) etc.
Private Second As New Outer(First) with an omitted Optional ByRef (icsharpcode#1081), instance initializer	constructor Second = initSecond();, plus an instance method Outer initSecond() { string argx = ""; return new Outer(First, x: ref argx); }
Private _a As Integer = Inc(P), instance	_a = Inc__a(); in the constructor; Inc__a() declares, calls, writes back and returns
Private Shared _c As Integer = Inc(SP) / any initializer that stays on the field	static int Inc__c() { … SP = argv; return ret; } and _c = Inc__c();, the existing Foo__p1 style, now with the write-back
Private _d, _e As New Outer(First), or A As Integer = Inc(C), B As Integer = Inc(C)	one method per variable, each emitted once
WithEvents W As New Src(P)	the method is emitted, and W = initW(); goes in the constructor
Partial class with constructors in another part	initialX() contains the temps, and is now an instance method

The jump result uses var only when the VB expression's own type is the type the jump needs. Otherwise the converted type is written out, e.g. long ret for a widening return.

Design

• PerScopeState.WithWriteBacksBefore (extracted in a pure refactor commit): if the current scope has write-backs, evaluate into a uniquely named local, write back, then use the local. Otherwise keep the single statement.
• Block headers reuse the existing local-function fallback through NextStatementDefinitelyExecutedAfter, so it's a single change. Capturing each header into a variable before the block would be a little prettier, but every visitor would need its own change. For Case clauses and Catch When it would also be wrong, because they're evaluated conditionally.
• Field initializers:
  • Each field initializer is converted in its own scope (CommonConversions.SplitVariableDeclarationsAsync).
  • If anything was hoisted, PerScopeState.CreateReturnBlockAsync builds a method body with the same machinery as method bodies: unique names, temps, write-backs before the return, and local functions. It doesn't consume the scope, so each variable sharing an initializer gets its own body.
  • DeclarationNodeVisitor takes the hoisted methods from the field scope as it emits them, so each one is emitted exactly once, for WithEvents fields too.
• CodeConverter/CSharp/README.md maps each case to the VB/C# rules, its code, its sources and the known gaps.

Decisions (accepted in review)

1. VB -> C#: Byref optional parameters to New field initializer can throw nullref icsharpcode/CodeConverter#1081: a wrapper method, rather than temps inlined in the constructor.
   • Inlined temps would be the most idiomatic output. They aren't cheap to do safely, though:
     • The initializers are copied into every constructor, so temps from different fields, and constructor parameters or locals, would need unique names in each constructor (CS0128/CS0136).
     • A partial class whose constructors are in another part needs a function anyway.
   • An omitted Optional ByRef argument already has no write-back. C# still needs a variable for ref (out _ only works for out), so a temp is the simplest correct form.
2. The method is static only when the initializer stays on the field. C# field initializers can't call instance methods. A constructor call needs an instance method to read instance members.
3. WithObjectInitializerCanReadFromPropertiesOfObjectAsync expectation changed: var init = new SomeClass(); became SomeClass @init = new SomeClass();. This is the same naming a method body uses for the same initializer.
4. Multi-file characterization PartialClassPart.cs (3 copies): static int[] initialAnArray…() lost static. It read an instance field, so it didn't compile before.

Review round 1

• Fixed, test-first:
  • A hoisted method was emitted twice when a field declaration had several declarators or array bounds (CS0111/CS0121).
  • A WithEvents field's method was never emitted (CS0103).
• Declined, and added to the README's Known gaps (both existed before this PR, and neither has a small clean fix):
  • VB copies back a call's arguments in reverse order. It only shows when two arguments write back to the same target. A fix needs per-call grouping in ArgumentConverter, because nested calls write back before their outer call, so a scope-wide reversal would be wrong.
  • A ReadOnly auto-property passed ByRef in a constructor or initializer is written back by VB but not by the converter. A fix needs the "assignable here" rule, and a C# write-back target that's assignable from the initializer method.

Reviewer attention

Known gaps, not changed here:

• ByRef arguments of New (object creation) in a block header still write back after the block.
• A non-constant For … Step is re-evaluated on every iteration. This is a separate For-conversion bug, not specific to ByRef.
• The two items declined in review round 1.

Tests

• Tests/TestData/SelfVerifyingTests/VBToCS/ByRefWriteBackPlacementTests.vb: 27 self-verifying tests. They cover:

  • return, return with a widening or Object conversion, return when the name ret is taken, property getter, multi-line lambda, throw, yield
  • Select Case, Case clause, a later Case that's never reached, For start and bound, For Each, nested headers, Using, SyncLock, With, Catch When
  • the VB -> C#: Byref optional parameters to New field initializer can throw nullref icsharpcode/CodeConverter#1081 repro, instance initializers in declaration order (including Inc(Counter) + Counter * 10), several variables in one declarator, several declarators with array bounds, a WithEvents field, a Shared initializer, a partial-class initializer
  • a control test for the unchanged shapes.

  Each fix was seen red first, for the expected reason.

• ByRefTests.cs: WritesBackBeforeJumpingIssue896Async, WritesBackBeforeBlockBodyAsync, OmittedOptionalRefInInstanceFieldInitializerIssue1081Async (the exact repro) and FieldInitializersWriteBackAsync pin the output.

• Every commit builds.

Suite vs baseline

• Full suite (net10.0): 1456 passed, 5 skipped, 0 failed.
• Master baseline: 1425 passed, 5 skipped, 0 failed.
• The difference is the 31 new tests. No other test changed result.

Coverage of changed lines

I ran the full suite with XPlat Code Coverage before review round 1. Every changed line is covered except:

• MethodBodyExecutableStatementVisitor.GetResultTypeSyntax: half the branches of typeInfo.Type is { } && Equals(...). The null-type branch is a guard, because a ByRef call's result always has a type.
• CommonConversions.GetInitializerMethodName: its unqualified-name branch was unreachable and has been removed.

The round 1 fix has no uncovered paths: both of its branches are exercised by the new tests.

🤖 Generated with Claude Code

https://claude.ai/code/session_019FEr8FVAmHV9bscZ82CChg