Place ByRef write-backs where VB runs them (return/throw/yield, block headers, field initializers)

CodeConverter/CSharp/CommonConversions.cs

@@ -61,6 +61,22 @@ public record VariablesDeclaration(CSSyntax.VariableDeclarationSyntax Decl, ITyp
 
     public async Task<(IReadOnlyCollection<VariablesDeclaration> Variables, IReadOnlyCollection<CSharpSyntaxNode> Methods)> SplitVariableDeclarationsAsync(
         VariableDeclaratorSyntax declarator, HashSet<ILocalSymbol> symbolsToSkip = null, bool preferExplicitType = false)
+    {
+        if (!IsField(declarator)) return await SplitVariableDeclarationsInCurrentScopeAsync(declarator, symbolsToSkip, preferExplicitType);
+
+        // A field initializer has no statement to hoist into, so it gets its own scope, which ConvertEqualsValueClauseSyntaxAsync moves into a method
+        _typeContext.PerScopeState.PushScope();
+        try {
+            return await SplitVariableDeclarationsInCurrentScopeAsync(declarator, symbolsToSkip, preferExplicitType);
+        } finally {
+            _typeContext.PerScopeState.PopScope();
+        }
+    }
+
+    private static bool IsField(VariableDeclaratorSyntax declarator) => declarator.Parent.IsKind(SyntaxKind.FieldDeclaration);
+
+    private async Task<(IReadOnlyCollection<VariablesDeclaration> Variables, IReadOnlyCollection<CSharpSyntaxNode> Methods)> SplitVariableDeclarationsInCurrentScopeAsync(
+        VariableDeclaratorSyntax declarator, HashSet<ILocalSymbol> symbolsToSkip, bool preferExplicitType)
     {
         var vbInitValue = GetInitializerToConvert(declarator);
         var initializerOrMethodDecl = await vbInitValue.AcceptAsync<CSharpSyntaxNode>(TriviaConvertingExpressionVisitor);
@@ -129,7 +145,7 @@ public bool ShouldPreferExplicitType(VBSyntax.ExpressionSyntax exp,
     {
         var csTypeSyntax = GetTypeSyntax(declaredSymbolType);
 
-        bool isField = vbDeclarator.Parent.IsKind(SyntaxKind.FieldDeclaration);
+        bool isField = IsField(vbDeclarator);
         bool declaredConst = declaredSymbol is IFieldSymbol fieldSymbol && fieldSymbol.IsConst ||
                              declaredSymbol is ILocalSymbol localSymbol && localSymbol.IsConst;
 
@@ -144,15 +160,17 @@ public bool ShouldPreferExplicitType(VBSyntax.ExpressionSyntax exp,
             if (isField && !declaredSymbol.IsStatic && !SemanticModel.IsDefinitelyStatic(vbName, vbInitValue)) {
                 if (!_typeContext.Initializers.HasInstanceConstructorsOutsideThisPart) {
                     var lhs = SyntaxFactory.IdentifierName(ConvertIdentifier(vbName.Identifier, sourceTriviaMapKind: SourceTriviaMapKind.None));
-                    _typeContext.Initializers.AdditionalInstanceInitializers.Add(new Assignment(lhs, CSSyntaxKind.SimpleAssignmentExpression, adjustedInitializerExpr.WithoutSourceMapping()));
+                    var initializer = await WithHoistedNodesInMethodAsync(vbName, adjustedInitializerExpr, csTypeSyntax, isStatic: false);
+                    _typeContext.Initializers.AdditionalInstanceInitializers.Add(new Assignment(lhs, CSSyntaxKind.SimpleAssignmentExpression, initializer.WithoutSourceMapping()));
                     equalsValueClauseSyntax = null;
                 } else {
-                    var returnBlock = SyntaxFactory.Block(SyntaxFactory.ReturnStatement(adjustedInitializerExpr));
-                    _typeContext.PerScopeState.Hoist(new HoistedFunction(GetInitialValueFunctionName(vbName), csTypeSyntax, returnBlock, SyntaxFactory.ParameterList()));
+                    var returnBlock = await _typeContext.PerScopeState.CreateReturnBlockAsync(vbName, adjustedInitializerExpr, csTypeSyntax, SemanticModel);
+                    _typeContext.PerScopeState.HoistToParent(new HoistedFunction(GetInitialValueFunctionName(vbName), csTypeSyntax, returnBlock, SyntaxFactory.ParameterList(), isStatic: false));
                     equalsValueClauseSyntax = null;
                 }
             } else {
-                equalsValueClauseSyntax = SyntaxFactory.EqualsValueClause(convertedInitializer);
+                var initializer = isField ? await WithHoistedNodesInMethodAsync(vbName, convertedInitializer, csTypeSyntax, isStatic: true) : convertedInitializer;
+                equalsValueClauseSyntax = SyntaxFactory.EqualsValueClause(initializer);
             }
         }
         else if (isField || declaredSymbol != null && SemanticModel.IsDefinitelyAssignedBeforeRead(declaredSymbol, vbName))
@@ -168,6 +186,30 @@ public bool ShouldPreferExplicitType(VBSyntax.ExpressionSyntax exp,
         return equalsValueClauseSyntax;
     }
 
+    /// <summary>
+    /// Temporaries for ByRef arguments (and their write-backs) need statements, which a field initializer can't contain.
+    /// A method keeps them together with the initializer whether it stays on the field or moves to the constructor.
+    /// It must be static to be called from a field initializer, and an instance method to read instance members from the constructor.
+    /// </summary>
+    private async Task<ExpressionSyntax> WithHoistedNodesInMethodAsync(VBSyntax.ModifiedIdentifierSyntax vbName, ExpressionSyntax initializer, TypeSyntax type, bool isStatic)
+    {
+        if (!_typeContext.PerScopeState.HasHoistedNodes) return initializer;
+
+        var body = await _typeContext.PerScopeState.CreateReturnBlockAsync(vbName, initializer, type, SemanticModel);
+        var baseMethodName = GetInitializerMethodName(initializer, ConvertIdentifier(vbName.Identifier).ValueText);
+        var methodName = NameGenerator.CS.GetUniqueVariableNameInScope(SemanticModel, new HashSet<string>(), vbName, baseMethodName);
+        _typeContext.PerScopeState.HoistToParent(new HoistedFunction(methodName, type, body, SyntaxFactory.ParameterList(), isStatic));
+        return SyntaxFactory.InvocationExpression(ValidSyntaxFactory.IdentifierName(methodName));
+    }
+
+    private static string GetInitializerMethodName(ExpressionSyntax initializer, string fieldName)
+    {
+        // Names are qualified by the time they're converted, e.g. Me.Foo(...)
+        return initializer is CSSyntax.InvocationExpressionSyntax { Expression: CSSyntax.MemberAccessExpressionSyntax { Name: var invokedName } }
+            ? $"{invokedName.Identifier.ValueText}_{fieldName}"
+            : "init" + fieldName.UppercaseFirstLetter();
+    }
+
     /// <remarks>
     /// In CS we need to lift non-static initializers to the constructor. But for partial classes these can be in different files.
     /// Rather than re-architect to allow communication between files, we create an initializer function, and call it from the other part, and just hope the name doesn't clash.

CodeConverter/CSharp/DeclarationNodeVisitor.cs

@@ -548,20 +548,12 @@ private IEnumerable<MemberDeclarationSyntax> CreateMemberDeclarations(IReadOnlyC
                 }
             }
 
+            foreach (var method in CreateExtraMethodMembers()) yield return method;
             if (isWithEvents) {
                 var fieldDecls = CreateWithEventsMembers(thisFieldModifiers, attributes, variablesDecl.Decl);
                 foreach (var f in fieldDecls) yield return f;
             } else {
-                foreach (var method in CreateExtraMethodMembers()) yield return method;
-
-                if (AdditionalLocals.GetDeclarations().Any()) {
-                    foreach (var additionalDecl in CreateAdditionalLocalMembers(thisFieldModifiers, attributes, variablesDecl.Decl)) {
-                        yield return additionalDecl;
-                    }
-                } else {
-                    yield return CS.SyntaxFactory.FieldDeclaration(CS.SyntaxFactory.List(attributes), thisFieldModifiers, variablesDecl.Decl);
-                }
-
+                yield return CS.SyntaxFactory.FieldDeclaration(CS.SyntaxFactory.List(attributes), thisFieldModifiers, variablesDecl.Decl);
             }
         }
     }
@@ -586,67 +578,11 @@ private IEnumerable<MemberDeclarationSyntax> CreateWithEventsMembers(SyntaxToken
         return fieldDecls;
     }
 
-    private IEnumerable<MemberDeclarationSyntax> CreateAdditionalLocalMembers(SyntaxTokenList convertedModifiers, List<AttributeListSyntax> attributes, VariableDeclarationSyntax decl)
-    {
-        if (decl.Variables.Count > 1) {
-            // Currently no way to tell which _additionalLocals would apply to which initializer
-            throw new NotImplementedException(
-                "Fields with multiple declarations and initializers with ByRef parameters not currently supported");
-        }
-
-        var v = decl.Variables.First();
-        var invocations = v.Initializer.Value.DescendantNodesAndSelf().OfType<InvocationExpressionSyntax>().ToArray();
-        if (invocations.Length > 1) {
-            throw new NotImplementedException(
-                "Field initializers with nested method calls not currently supported");
-        }
-
-        string newMethodName;
-        if (invocations.OnlyOrDefault() is { Expression: {} e} && e.ChildNodes().OfType<SimpleNameSyntax>().LastOrDefault() is {} sns) {
-            newMethodName = $"{sns?.Identifier.ValueText}_{v.Identifier.ValueText}";
-        } else {
-            newMethodName = "init" + v.Identifier.ValueText.UppercaseFirstLetter();
-        }
-
-        var declarationInfo = AdditionalLocals.GetDeclarations();
-
-        var localVars = declarationInfo
-            .Select(al => CommonConversions.CreateLocalVariableDeclarationAndAssignment(al.Prefix, al.Initializer))
-            .ToArray<StatementSyntax>();
-
-        // This should probably use a unique name like in MethodBodyVisitor - a collision is far less likely here
-        var newNames = declarationInfo.ToDictionary(l => l.Id, l => l.Prefix);
-        var newInitializer = PerScopeState.ReplaceNames(v.Initializer.Value, newNames);
-
-        var body = CS.SyntaxFactory.Block(localVars.Concat(CS.SyntaxFactory.ReturnStatement(newInitializer).Yield()));
-        // Method calls in initializers must be static in C# - Supporting this is #281
-        var methodDecl = ValidSyntaxFactory.CreateParameterlessMethod(newMethodName, decl.Type, body);
-        yield return methodDecl;
-
-        var newVar =
-            v.WithInitializer(CS.SyntaxFactory.EqualsValueClause(
-                CS.SyntaxFactory.InvocationExpression(ValidSyntaxFactory.IdentifierName(newMethodName))));
-        var newVarDecl =
-            CS.SyntaxFactory.VariableDeclaration(decl.Type, CS.SyntaxFactory.SingletonSeparatedList(newVar));
-
-        yield return CS.SyntaxFactory.FieldDeclaration(CS.SyntaxFactory.List(attributes), convertedModifiers, newVarDecl);
-    }
-
     private IReadOnlyCollection<MemberDeclarationSyntax> CreateExtraMethodMembers()
     {
-        var methodsInfos = AdditionalLocals.GetParameterlessFunctions();
-
-        // In theory this should use a unique name like in MethodBodyVisitor
-        // Unfortunately we can't change the name for these since the file containing the constructor of a partial class tries to call the method with exactly this name.
-        // See GetInitialValueFunctionName
-        var newMethodNames = methodsInfos.ToDictionary(l => l.Id, l => l.Prefix);
-        for (int i = 0; i < _typeContext.Initializers.AdditionalInstanceInitializers.Count; i++) {
-            var (a, b, initializer, _) = _typeContext.Initializers.AdditionalInstanceInitializers[i];
-            _typeContext.Initializers.AdditionalInstanceInitializers[i] = new Assignment(a, b, PerScopeState.ReplaceNames(initializer, newMethodNames));
-        }
-
-        return methodsInfos
-            .Select(al => al.AsInstanceMethod(newMethodNames[al.Id]))
+        // Callers reference these by their final name, so it's the prefix rather than a placeholder
+        return AdditionalLocals.TakeParameterlessFunctions()
+            .Select(al => al.AsInstanceMethod(al.Prefix))
             .ToArray();
     }
 

CodeConverter/CSharp/HoistedFunction.cs

@@ -8,20 +8,23 @@ internal class HoistedFunction : IHoistedNode
     private readonly TypeSyntax _returnType;
     private readonly BlockSyntax _block;
     private readonly ParameterListSyntax _parameters;
+    private readonly bool _isStatic;
 
     public string Id { get; }
     public string Prefix { get; }
 
-    public HoistedFunction(string localFuncName, TypeSyntax returnType, BlockSyntax block, ParameterListSyntax parameters)
+    /// <param name="isStatic">Only used when hoisted to a member: it must be an instance method to be called from an instance context such as a constructor</param>
+    public HoistedFunction(string localFuncName, TypeSyntax returnType, BlockSyntax block, ParameterListSyntax parameters, bool isStatic = true)
     {
         Id = $"hs{Guid.NewGuid().ToString("N")}";
         Prefix = localFuncName;
         _returnType = returnType;
         _block = block;
         _parameters = parameters;
+        _isStatic = isStatic;
     }
 
     public IdentifierNameSyntax TempIdentifier => ValidSyntaxFactory.IdentifierName(Id).WithAdditionalAnnotations(PerScopeState.AdditionalLocalAnnotation);
     public LocalFunctionStatementSyntax AsLocalFunction(string functionName) => SyntaxFactory.LocalFunctionStatement(_returnType, SyntaxFactory.Identifier(functionName)).WithParameterList(_parameters).WithBody(_block);
-    public MethodDeclarationSyntax AsInstanceMethod(string functionName) => ValidSyntaxFactory.CreateMethod(functionName, _returnType, _parameters, _block);
+    public MethodDeclarationSyntax AsInstanceMethod(string functionName) => ValidSyntaxFactory.CreateMethod(functionName, _returnType, _parameters, _block, _isStatic);
 }

CodeConverter/CSharp/MethodBodyExecutableStatementVisitor.cs

@@ -492,7 +492,8 @@ private ExpressionStatementSyntax CreateNewArrayAssignment(VBSyntax.ExpressionSy
 
     public override async Task<SyntaxList<StatementSyntax>> VisitThrowStatement(VBSyntax.ThrowStatementSyntax node)
     {
-        return SingleStatement(SyntaxFactory.ThrowStatement(await node.Expression.AcceptAsync<ExpressionSyntax>(_expressionVisitor)));
+        var csExpression = await node.Expression.AcceptAsync<ExpressionSyntax>(_expressionVisitor);
+        return WithWriteBacksBeforeJump(node.Expression, csExpression, "ex", SyntaxFactory.ThrowStatement);
     }
 
     public override async Task<SyntaxList<StatementSyntax>> VisitReturnStatement(VBSyntax.ReturnStatementSyntax node)
@@ -502,7 +503,27 @@ public override async Task<SyntaxList<StatementSyntax>> VisitReturnStatement(VBS
 
         var csExpression = await node.Expression.AcceptAsync<ExpressionSyntax>(_expressionVisitor);
         csExpression = CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node.Expression, csExpression);
-        return SingleStatement(SyntaxFactory.ReturnStatement(csExpression));
+        return WithWriteBacksBeforeJump(node.Expression, csExpression, "ret", SyntaxFactory.ReturnStatement);
+    }
+
+    /// <summary>
+    /// Write-backs are normally emitted after the converted statement, which after a jump would be unreachable (or, for yield, delayed until the consumer resumes).
+    /// </summary>
+    private SyntaxList<StatementSyntax> WithWriteBacksBeforeJump(VBSyntax.ExpressionSyntax vbExpression, ExpressionSyntax csExpression, string resultPrefix,
+        Func<ExpressionSyntax, StatementSyntax> createJump)
+    {
+        if (csExpression is null) return SingleStatement(createJump(null));
+        return SyntaxFactory.List(_perScopeState.WithWriteBacksBefore(csExpression, resultPrefix, () => GetResultTypeSyntax(vbExpression), createJump));
+    }
+
+    /// <summary>
+    /// <c>var</c> is only safe when the expression's own type is the one the jump needs: e.g. Nothing or a lambda has no natural type in C#.
+    /// </summary>
+    private TypeSyntax GetResultTypeSyntax(VBSyntax.ExpressionSyntax vbExpression)
+    {
+        var typeInfo = _semanticModel.GetTypeInfo(vbExpression);
+        var isNaturalType = typeInfo.Type is { } type && SymbolEqualityComparer.Default.Equals(type, typeInfo.ConvertedType);
+        return CommonConversions.GetTypeSyntax(typeInfo.ConvertedType, useImplicitType: isNaturalType);
     }
 
     public override async Task<SyntaxList<StatementSyntax>> VisitContinueStatement(VBSyntax.ContinueStatementSyntax node)
@@ -513,7 +534,8 @@ public override async Task<SyntaxList<StatementSyntax>> VisitContinueStatement(V
 
     public override async Task<SyntaxList<StatementSyntax>> VisitYieldStatement(VBSyntax.YieldStatementSyntax node)
     {
-        return SingleStatement(SyntaxFactory.YieldStatement(SyntaxKind.YieldReturnStatement, await node.Expression.AcceptAsync<ExpressionSyntax>(_expressionVisitor)));
+        var csExpression = await node.Expression.AcceptAsync<ExpressionSyntax>(_expressionVisitor);
+        return WithWriteBacksBeforeJump(node.Expression, csExpression, "ret", e => SyntaxFactory.YieldStatement(SyntaxKind.YieldReturnStatement, e));
     }
 
     public override async Task<SyntaxList<StatementSyntax>> VisitExitStatement(VBSyntax.ExitStatementSyntax node)

CodeConverter/CSharp/NameExpressionNodeVisitor.cs

@@ -610,7 +610,9 @@ private bool MayThrow(VBSyntax.ExpressionSyntax expression)
     }
 
     /// <summary>
-    /// Conservative version of _semanticModel.AnalyzeControlFlow(invocation).ExitPoints to account for exceptions
+    /// Conservative version of _semanticModel.AnalyzeControlFlow(invocation).ExitPoints to account for exceptions.
+    /// A block's header statement is followed by its body, so a write-back placed after the converted block would run too late
+    /// (or, for a later Case clause, run when the clause was never evaluated).
     /// </summary>
     private static bool NextStatementDefinitelyExecutedAfter(VBSyntax.InvocationExpressionSyntax invocation)
     {
@@ -626,6 +628,14 @@ private static bool NextStatementDefinitelyExecutedAfter(VBSyntax.InvocationExpr
                 case VBSyntax.IfStatementSyntax _:
                 case VBSyntax.ElseIfStatementSyntax _:
                 case VBSyntax.SingleLineIfStatementSyntax _:
+                case VBSyntax.SelectStatementSyntax _:
+                case VBSyntax.CaseStatementSyntax _:
+                case VBSyntax.ForStatementSyntax _:
+                case VBSyntax.ForEachStatementSyntax _:
+                case VBSyntax.UsingStatementSyntax _:
+                case VBSyntax.SyncLockStatementSyntax _:
+                case VBSyntax.WithStatementSyntax _:
+                case VBSyntax.CatchStatementSyntax _:
                     return false;
                 case VBSyntax.ExpressionSyntax _:
                 case VBSyntax.StatementSyntax _: