Skip to content

Commit 6dab5a0

Browse files
GrahamTheCoderGrahamTheCoder
committed
Manually constructed test case
1 parent 386d402 commit 6dab5a0

2 files changed

Lines changed: 146 additions & 66 deletions

File tree

‎CodeConverter/CSharp/ExpressionNodeVisitor.cs‎

Lines changed: 69 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -414,19 +414,12 @@ public override async Task<CSharpSyntaxNode> VisitSimpleArgument(VBasic.Syntax.S
414414
var refType = GetRefType(node, argList, parameters, out argName, out refKind);
415415
var expression = CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node.Expression, (ExpressionSyntax)await node.Expression.AcceptAsync(TriviaConvertingExpressionVisitor), defaultToCast: refKind != RefKind.None);
416416

417-
if (refType == RefType.Variable) {
417+
if (refType != RefConversion.Inline) {
418418
var expressionTypeInfo = _semanticModel.GetTypeInfo(node.Expression);
419419
bool useVar = expressionTypeInfo.Type?.Equals(expressionTypeInfo.ConvertedType) == true && !CommonConversions.ShouldPreferExplicitType(node.Expression, expressionTypeInfo.ConvertedType, out var _);
420420
var typeSyntax = CommonConversions.GetTypeSyntax(expressionTypeInfo.ConvertedType, useVar);
421421
string prefix = $"arg{argName}";
422422
local = _additionalLocals.AddAdditionalLocal(new AdditionalLocal(prefix, expression, typeSyntax));
423-
} else if (refType == RefType.Property && invocation is VBSyntax.InvocationExpressionSyntax ies) {
424-
//TODO: Don't break for multiple byref args in one method
425-
var expressionTypeInfo = _semanticModel.GetTypeInfo(node.Expression);
426-
bool useVar = expressionTypeInfo.Type?.Equals(expressionTypeInfo.ConvertedType) == true && !CommonConversions.ShouldPreferExplicitType(node.Expression, expressionTypeInfo.ConvertedType, out var _);
427-
var typeSyntax = CommonConversions.GetTypeSyntax(expressionTypeInfo.ConvertedType, useVar);
428-
var (localFuncName, functionDecl) = CreateLocalByRefFunction(ies, argName, expression, methodSymbol);
429-
local = _additionalLocals.AddAdditionalLocal(new AdditionalLocal(localFuncName, expression, typeSyntax));
430423
}
431424
}
432425

@@ -458,7 +451,7 @@ private async Task<ExpressionSyntax> ConvertArgExpression(VBSyntax.SimpleArgumen
458451
return CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node.Expression, (ExpressionSyntax)await node.Expression.AcceptAsync(TriviaConvertingExpressionVisitor), defaultToCast: refKind != RefKind.None);
459452
}
460453

461-
private RefType GetRefType(VBSyntax.SimpleArgumentSyntax node, VBSyntax.ArgumentListSyntax argList, System.Collections.Immutable.ImmutableArray<IParameterSymbol> parameters, out string argName, out RefKind refKind)
454+
private RefConversion GetRefType(VBSyntax.SimpleArgumentSyntax node, VBSyntax.ArgumentListSyntax argList, System.Collections.Immutable.ImmutableArray<IParameterSymbol> parameters, out string argName, out RefKind refKind)
462455
{
463456
var parameter = !node.IsNamed ? parameters.ElementAtOrDefault(argList.Arguments.IndexOf(node)) : parameters.FirstOrDefault(p => p.Name.Equals(node.NameColonEquals.Name.Identifier.Text, StringComparison.OrdinalIgnoreCase));
464457
if (parameter != null) {
@@ -471,38 +464,6 @@ private RefType GetRefType(VBSyntax.SimpleArgumentSyntax node, VBSyntax.Argument
471464
return NeedsVariableForArgument(node, refKind);
472465
}
473466

474-
private async Task<(string Id, StatementSyntax FunctionDeclaration)> CreateLocalByRefFunction(VBSyntax.InvocationExpressionSyntax invocation, string argName, ExpressionSyntax expression, IMethodSymbol invocationSymbol)
475-
{
476-
var originalArgsWithRefTypes = invocation.ArgumentList.Arguments
477-
.Select(a => (Arg: (VBSyntax.SimpleArgumentSyntax)a, RefType: GetRefType((VBSyntax.SimpleArgumentSyntax) a, invocation.ArgumentList, invocationSymbol.Parameters, out var argName, out var refKind), Name: argName, RefKind: refKind))
478-
.ToArray();
479-
480-
var argsForCallInLocalFunction = (await originalArgsWithRefTypes.SelectAsync(async a => a.RefType switch {
481-
RefType.Inline => await ConvertArgExpression(a.Arg, a.RefKind),
482-
_ => SyntaxFactory.IdentifierName(a.Name)
483-
})).Select(SyntaxFactory.Argument).ToArray();
484-
var argList = SyntaxFactory.ArgumentList(GetAdditionalRequiredArgs(invocationSymbol, argsForCallInLocalFunction));
485-
var tmpArgName = $"arg{argName}";
486-
var tempArg = SyntaxFactory.IdentifierName(tmpArgName);
487-
var localFuncName = $"local{invocationSymbol.Name}";
488-
const string retVariableName = "ret";
489-
var localFuncId = SyntaxFactory.IdentifierName(localFuncName);
490-
var declareTemp = CommonConversions.CreateLocalVariableDeclarationAndAssignment(tmpArgName, expression);
491-
// Need essentially the original invocation with any byref args swapped out for temporaries
492-
var callAndStoreResult = CommonConversions.CreateLocalVariableDeclarationAndAssignment(retVariableName,
493-
SyntaxFactory.InvocationExpression(expression,
494-
);
495-
var block = SyntaxFactory.Block(
496-
declareTemp,
497-
callAndStoreResult,
498-
AssignStmt(expression, tempArg),
499-
SyntaxFactory.ReturnStatement(SyntaxFactory.IdentifierName(retVariableName))
500-
);
501-
var localfunction = SyntaxFactory.LocalFunctionStatement(CommonConversions.GetTypeSyntax(invocationSymbol.ReturnType),
502-
localFuncId.Identifier).WithBody(block);
503-
return (localFuncName, localfunction);
504-
}
505-
506467
private static StatementSyntax AssignStmt(ExpressionSyntax left, IdentifierNameSyntax right)
507468
{
508469
return SyntaxFactory.ExpressionStatement(Assign(left, right));
@@ -878,6 +839,9 @@ public override async Task<CSharpSyntaxNode> VisitInvocationExpression(
878839

879840
return SyntaxFactory.InvocationExpression((ExpressionSyntax)expr, args);
880841
}
842+
//TODO: Decide if the above override should be subject to the rest of this method's adjustments (probably)
843+
844+
CreateLocalByRefFunction(node, invocationSymbol);
881845

882846
// VB doesn't have a specialized node for element access because the syntax is ambiguous. Instead, it just uses an invocation expression or dictionary access expression, then figures out using the semantic model which one is most likely intended.
883847
// https://github.com/dotnet/roslyn/blob/master/src/Workspaces/VisualBasic/Portable/LanguageServices/VisualBasicSyntaxFactsService.vb#L768
@@ -927,6 +891,48 @@ async Task<CSharpSyntaxNode> CreateElementAccess()
927891
}
928892
}
929893

894+
895+
/// <summary>
896+
/// If we need a local function,
897+
/// </summary>
898+
/// <param name="invocation"></param>
899+
/// <param name="invocationSymbol"></param>
900+
/// <returns></returns>
901+
private async Task<(string Id, StatementSyntax FunctionDeclaration)> CreateLocalByRefFunction(VBSyntax.InvocationExpressionSyntax invocation, IMethodSymbol invocationSymbol)
902+
{
903+
var originalArgsWithRefTypes = invocation.ArgumentList.Arguments
904+
.Select(a => (Arg: (VBSyntax.SimpleArgumentSyntax)a, RefType: GetRefType((VBSyntax.SimpleArgumentSyntax)a, invocation.ArgumentList, invocationSymbol.Parameters, out var argName, out var refKind), Name: argName, RefKind: refKind))
905+
.ToArray();
906+
907+
var isDefinitelyExecuted = IsDefinitelyExecutedInStatement(invocation);
908+
909+
var localFuncName = $"local{invocationSymbol.Name}";
910+
const string retVariableName = "ret";
911+
var localFuncId = SyntaxFactory.IdentifierName(localFuncName);
912+
// Need essentially the original invocation with any byref args swapped out for temporaries
913+
var callAndStoreResult = CommonConversions.CreateLocalVariableDeclarationAndAssignment(retVariableName,
914+
SyntaxFactory.InvocationExpression(expression,
915+
);
916+
917+
var block = SyntaxFactory.Block(
918+
callAndStoreResult,
919+
AssignStmt(expression, tempArg),
920+
SyntaxFactory.ReturnStatement(SyntaxFactory.IdentifierName(retVariableName))
921+
);
922+
var localfunction = SyntaxFactory.LocalFunctionStatement(CommonConversions.GetTypeSyntax(invocationSymbol.ReturnType),
923+
localFuncId.Identifier).WithBody(block);
924+
return (localFuncName, localfunction);
925+
}
926+
927+
private static bool IsDefinitelyExecutedInStatement(VBSyntax.InvocationExpressionSyntax invocation)
928+
{
929+
VBSyntax.StatementSyntax parentStatement;
930+
do {
931+
parentStatement = invocation.GetAncestor<VBSyntax.StatementSyntax>();
932+
} while (parentStatement is VBSyntax.ElseIfStatementSyntax);
933+
return parentStatement.FollowProperty<SyntaxNode>(n => n.ChildNodes().FirstOrDefault()).Contains(invocation);
934+
}
935+
930936
public override async Task<CSharpSyntaxNode> VisitSingleLineLambdaExpression(VBasic.Syntax.SingleLineLambdaExpressionSyntax node)
931937
{
932938
IReadOnlyCollection<StatementSyntax> convertedStatements;
@@ -1358,7 +1364,7 @@ private ArgumentSyntax CreateOptionalRefArg(IParameterSymbol p)
13581364
return (ArgumentSyntax)CommonConversions.CsSyntaxGenerator.Argument(p.Name, p.RefKind, local.IdentifierName);
13591365
}
13601366

1361-
private RefType NeedsVariableForArgument(VBasic.Syntax.SimpleArgumentSyntax node, RefKind refKind)
1367+
private RefConversion NeedsVariableForArgument(VBasic.Syntax.SimpleArgumentSyntax node, RefKind refKind)
13621368
{
13631369
bool isIdentifier = node.Expression is VBasic.Syntax.IdentifierNameSyntax;
13641370
bool isMemberAccess = node.Expression is VBasic.Syntax.MemberAccessExpressionSyntax;
@@ -1370,16 +1376,31 @@ private RefType NeedsVariableForArgument(VBasic.Syntax.SimpleArgumentSyntax node
13701376
var typeInfo = _semanticModel.GetTypeInfo(node.Expression);
13711377
bool isTypeMismatch = typeInfo.Type == null || !typeInfo.Type.Equals(typeInfo.ConvertedType);
13721378

1373-
if (isProperty) return RefType.Property;
1374-
if ((!isIdentifier && !isMemberAccess) || isTypeMismatch || isUsing) return RefType.Variable;
1375-
return RefType.Inline;
1379+
if (isProperty) return RefConversion.PreAndPostAssignment;
1380+
if ((!isIdentifier && !isMemberAccess) || isTypeMismatch || isUsing) return RefConversion.PreAssigment;
1381+
return RefConversion.Inline;
13761382
}
13771383

1378-
private enum RefType
1384+
/// <summary>
1385+
/// https://github.com/icsharpcode/CodeConverter/issues/324
1386+
/// https://github.com/icsharpcode/CodeConverter/issues/310
1387+
/// </summary>
1388+
private enum RefConversion
13791389
{
1390+
/// <summary>
1391+
/// e.g. Normal field, parameter or local
1392+
/// </summary>
13801393
Inline,
1381-
Variable,
1382-
Property
1394+
/// <summary>
1395+
/// Needs assignment before and/or after
1396+
/// e.g. Method/Property result
1397+
/// </summary>
1398+
PreAssigment,
1399+
/// <summary>
1400+
/// Needs assignment before and/or after
1401+
/// i.e. Property
1402+
/// </summary>
1403+
PreAndPostAssignment
13831404
}
13841405

13851406
private ISymbol GetInvocationSymbol(SyntaxNode invocation)

‎Tests/CSharp/ExpressionTests/ByRefTests.cs‎

Lines changed: 77 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -331,38 +331,97 @@ public static Class1 Foo(ref Class1 c1)
331331
[Fact]
332332
public async Task AssignsBackToPropertyAsync()
333333
{
334-
await TestConversionVisualBasicToCSharpAsync(@"Public Class RefTestClass
334+
await TestConversionVisualBasicToCSharpAsync(@"Imports System
335335
336-
Private Property Prop1 As Integer
337-
Private Property Prop2 As Integer = 1
336+
Public Class MyTestClass
338337
339-
Private Sub TakesRef(ByRef vrbTst As Integer) As Boolean
340-
vrbTst = vrbTst + Prop2
341-
Return vrbTst > 1
338+
Private Property Prop As Integer
339+
Private Property Prop2 As Integer
340+
341+
Private Function TakesRef(ByRef vrbTst As Integer) As Boolean
342+
vrbTst = Prop + 1
343+
Return vrbTst > 3
344+
End Function
345+
346+
Private Sub TakesRefVoid(ByRef vrbTst As Integer)
347+
vrbTst = vrbTst + 1
342348
End Sub
343-
344-
Private Sub UsesRef(someInt As Integer)
345-
If TakesRef(Prop1) OrElse TakesRef(Prop2) AndAlso TakesRef(Prop1)
349+
350+
Public Sub UsesRef(someBool As Boolean, someInt As Integer)
351+
352+
TakesRefVoid(someInt) ' Convert directly
353+
TakesRefVoid(1) 'Requires variable before
354+
TakesRefVoid(Prop2) ' Requires variable before, and to assign back after
355+
356+
Dim a =TakesRef(someInt) ' Convert directly
357+
Dim b = TakesRef(2) 'Requires variable before
358+
Dim c = TakesRef(Prop) ' Requires variable before, and to assign back after
359+
360+
If 16 > someInt OrElse TakesRef(someInt) ' Convert directly
361+
Console.WriteLine(1)
362+
Else If someBool AndAlso TakesRef(3) 'Requires variable before (in local function)
346363
someInt += 1
364+
Else If TakesRef(Prop) ' Requires variable before, and to assign back after (in local function)
365+
someInt -=2
347366
End If
367+
Console.WriteLine(someInt)
348368
End Sub
349-
End Class", @"public class RefTestClass
369+
End Class", @"
370+
public partial class MyTestClass
350371
{
351-
private int Prop1 { get; set; }
372+
private int Prop { get; set; }
352373
private int Prop2 { get; set; }
353374
354375
private bool TakesRef(ref int vrbTst)
355376
{
356-
vrbTst = vrbTst + Prop2;
357-
return vrbTst > 1;
377+
vrbTst = Prop + 1;
378+
return vrbTst > 3;
358379
}
359380
360-
private void UsesRef(int someInt)
381+
private void TakesRefVoid(ref int vrbTst)
361382
{
362-
var argvrbTst = Prop;
363-
TakesRef(ref argvrbTst);
364-
Prop = argvrbTst; // Need to set value of property afterwards to maintain logic
365-
someInt += 1;
383+
vrbTst = vrbTst + 1;
384+
}
385+
386+
public void UsesRef(bool someBool, int someInt)
387+
{
388+
TakesRefVoid(ref someInt); // Convert directly
389+
int argvrbTst = 1;
390+
TakesRefVoid(ref argvrbTst); // Requires variable before
391+
int argvrbTst1 = Prop2;
392+
TakesRefVoid(ref argvrbTst1); // Requires variable before, and to assign back after
393+
bool a = TakesRef(ref someInt); // Convert directly
394+
int argvrbTst2 = 2;
395+
bool b = TakesRef(ref argvrbTst2); // Requires variable before
396+
int argvrbTst3 = Prop;
397+
bool c = TakesRef(ref argvrbTst3); // Requires variable before, and to assign back after
398+
399+
bool localTakesRef1() {
400+
int argvrbTst4 = 3;
401+
var ret = TakesRef(ref argvrbTst4);
402+
return ret;
403+
}
404+
bool localTakesRef2() {
405+
int argvrbTst5 = Prop;
406+
var ret = TakesRef(ref argvrbTst5);
407+
Prop = argvrbTst5;
408+
return ret;
409+
}
410+
411+
if (16 > someInt || TakesRef(ref someInt)) // Convert directly
412+
{
413+
Console.WriteLine(1);
414+
}
415+
else if (someBool && localTakesRef1()) // Requires variable before (in local function)
416+
{
417+
someInt += 1;
418+
}
419+
else if (localTakesRef2()) // Requires variable before, and to assign back after (in local function)
420+
{
421+
someInt -= 2;
422+
}
423+
424+
Console.WriteLine(someInt);
366425
}
367426
}");
368427
}

0 commit comments

Comments
 (0)