Skip to content

Commit 679eb44

Browse files
google-labs-jules[bot]GrahamTheCoder
google-labs-jules[bot]
and
GrahamTheCoder
committed
Fix VB to C# conversion for extension methods on properties passed by ref
* Extract `HoistByRefDeclaration` in `ArgumentConverter` to accept `ExpressionSyntax` instead of `SimpleArgumentSyntax` so it can be reused for the `this` parameter of extension methods. * Update `NameExpressionNodeVisitor` to apply ref-conversion hoisting rules to the `this` expression of extension methods using `ref` parameters. * Add a unit test verifying that `Number.NegEx()` where `NegEx` takes `ByRef num as Integer` correctly generates a temporary variable assignment before and after the call. Co-authored-by: GrahamTheCoder <2490482+GrahamTheCoder@users.noreply.github.com>
1 parent fadac1b commit 679eb44

File tree

3 files changed

+108
-13
lines changed

3 files changed

+108
-13
lines changed

‎CodeConverter/CSharp/ArgumentConverter.cs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ public async Task<CSharpSyntaxNode> ConvertSimpleArgumentAsync(VBSyntax.SimpleAr
3535
var refType = CommonConversions.GetRefConversionType(node, argList, possibleParameters.Value, out var argName, out var refKind);
3636
token = CommonConversions.GetRefToken(refKind);
3737
if (refType != SemanticModelExtensions.RefConversion.Inline) {
38-
convertedArgExpression = HoistByRefDeclaration(node, convertedArgExpression, refType, argName, refKind);
38+
convertedArgExpression = HoistByRefDeclaration(node.Expression, convertedArgExpression, refType, argName, refKind);
3939
} else {
4040
convertedArgExpression = typeConversionAnalyzer.AddExplicitConversion(node.Expression, convertedArgExpression, defaultToCast: refKind != RefKind.None);
4141
}
@@ -119,11 +119,11 @@ CSSyntax.ArgumentSyntax ConvertOmittedArgument(IParameterSymbol parameter)
119119
}
120120

121121

122-
private CSSyntax.ExpressionSyntax HoistByRefDeclaration(VBSyntax.SimpleArgumentSyntax node, CSSyntax.ExpressionSyntax refLValue, SemanticModelExtensions.RefConversion refType, string argName, RefKind refKind)
122+
internal CSSyntax.ExpressionSyntax HoistByRefDeclaration(VBSyntax.ExpressionSyntax node, CSSyntax.ExpressionSyntax refLValue, SemanticModelExtensions.RefConversion refType, string argName, RefKind refKind)
123123
{
124124
string prefix = $"arg{argName}";
125-
var expressionTypeInfo = _semanticModel.GetTypeInfo(node.Expression);
126-
bool useVar = expressionTypeInfo.Type?.Equals(expressionTypeInfo.ConvertedType, SymbolEqualityComparer.IncludeNullability) == true && !CommonConversions.ShouldPreferExplicitType(node.Expression, expressionTypeInfo.ConvertedType, out var _);
125+
var expressionTypeInfo = _semanticModel.GetTypeInfo(node);
126+
bool useVar = expressionTypeInfo.Type?.Equals(expressionTypeInfo.ConvertedType, SymbolEqualityComparer.IncludeNullability) == true && !CommonConversions.ShouldPreferExplicitType(node, expressionTypeInfo.ConvertedType, out var _);
127127
var typeSyntax = CommonConversions.GetTypeSyntax(expressionTypeInfo.ConvertedType, useVar);
128128

129129
if (refLValue is CSSyntax.ElementAccessExpressionSyntax eae) {
@@ -132,12 +132,12 @@ private CSSyntax.ExpressionSyntax HoistByRefDeclaration(VBSyntax.SimpleArgumentS
132132
refLValue = eae.WithExpression(tmpContainer.IdentifierName);
133133
}
134134

135-
var withCast = CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node.Expression, refLValue, defaultToCast: refKind != RefKind.None);
135+
var withCast = CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node, refLValue, defaultToCast: refKind != RefKind.None);
136136

137137
var local = _typeContext.PerScopeState.Hoist(new AdditionalDeclaration(prefix, withCast, typeSyntax));
138138

139139
if (refType == SemanticModelExtensions.RefConversion.PreAndPostAssignment) {
140-
var convertedLocalIdentifier = CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node.Expression, local.IdentifierName, forceSourceType: expressionTypeInfo.ConvertedType, forceTargetType: expressionTypeInfo.Type);
140+
var convertedLocalIdentifier = CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node, local.IdentifierName, forceSourceType: expressionTypeInfo.ConvertedType, forceTargetType: expressionTypeInfo.Type);
141141
_typeContext.PerScopeState.Hoist(new AdditionalAssignment(refLValue, convertedLocalIdentifier));
142142
}
143143

‎CodeConverter/CSharp/NameExpressionNodeVisitor.cs‎

Lines changed: 40 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -285,14 +285,47 @@ private async Task<ExpressionSyntax> ConvertInvocationAsync(VBSyntax.InvocationE
285285
}
286286

287287
if (invocationSymbol.IsReducedExtension() && invocationSymbol is IMethodSymbol { ReducedFrom: { Parameters: var parameters } } &&
288-
!parameters.FirstOrDefault().ValidCSharpExtensionMethodParameter() &&
289288
node.Expression is VBSyntax.MemberAccessExpressionSyntax maes) {
290-
var thisArgExpression = await maes.Expression.AcceptAsync<ExpressionSyntax>(TriviaConvertingExpressionVisitor);
291-
var thisArg = SyntaxFactory.Argument(thisArgExpression).WithRefKindKeyword(CommonConversions.GetRefToken(RefKind.Ref));
292-
convertedArgumentList = SyntaxFactory.ArgumentList(SyntaxFactory.SeparatedList(convertedArgumentList.Arguments.Prepend(thisArg)));
293-
var containingType = (ExpressionSyntax)CommonConversions.CsSyntaxGenerator.TypeExpression(invocationSymbol.ContainingType);
294-
convertedExpression = SyntaxFactory.MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, containingType,
295-
ValidSyntaxFactory.IdentifierName((invocationSymbol.Name)));
289+
290+
var thisParam = parameters.FirstOrDefault();
291+
bool requiresStaticInvocation = !thisParam.ValidCSharpExtensionMethodParameter();
292+
var refKind = CommonConversions.GetCsRefKind(thisParam);
293+
294+
bool requiresHoist = false;
295+
SemanticModelExtensions.RefConversion refConversion = SemanticModelExtensions.RefConversion.Inline;
296+
if (refKind != RefKind.None) {
297+
var symbolInfo = _semanticModel.GetSymbolInfoInDocument<ISymbol>(maes.Expression);
298+
if (symbolInfo is IPropertySymbol { ReturnsByRef: false, ReturnsByRefReadonly: false } propertySymbol) {
299+
refConversion = propertySymbol.IsReadOnly ? SemanticModelExtensions.RefConversion.PreAssigment : SemanticModelExtensions.RefConversion.PreAndPostAssignment;
300+
} else if (symbolInfo is IFieldSymbol { IsConst: true } or ILocalSymbol { IsConst: true }) {
301+
refConversion = SemanticModelExtensions.RefConversion.PreAssigment;
302+
} else if (symbolInfo is IMethodSymbol { ReturnsByRef: false, ReturnsByRefReadonly: false }) {
303+
refConversion = SemanticModelExtensions.RefConversion.PreAssigment;
304+
} else {
305+
var typeInfo = _semanticModel.GetTypeInfo(maes.Expression);
306+
bool isTypeMismatch = typeInfo.Type == null || !typeInfo.Type.Equals(typeInfo.ConvertedType, SymbolEqualityComparer.IncludeNullability);
307+
if (isTypeMismatch) refConversion = SemanticModelExtensions.RefConversion.PreAndPostAssignment;
308+
}
309+
requiresHoist = refConversion != SemanticModelExtensions.RefConversion.Inline;
310+
}
311+
312+
if (requiresStaticInvocation || requiresHoist) {
313+
var thisArgExpression = await maes.Expression.AcceptAsync<ExpressionSyntax>(TriviaConvertingExpressionVisitor);
314+
315+
if (requiresHoist) {
316+
thisArgExpression = _argumentConverter.HoistByRefDeclaration(maes.Expression, thisArgExpression, refConversion, thisParam.Name, refKind);
317+
}
318+
319+
if (requiresStaticInvocation) {
320+
var thisArg = SyntaxFactory.Argument(thisArgExpression).WithRefKindKeyword(CommonConversions.GetRefToken(refKind));
321+
convertedArgumentList = SyntaxFactory.ArgumentList(SyntaxFactory.SeparatedList(convertedArgumentList.Arguments.Prepend(thisArg)));
322+
var containingType = (ExpressionSyntax)CommonConversions.CsSyntaxGenerator.TypeExpression(invocationSymbol.ContainingType);
323+
convertedExpression = SyntaxFactory.MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, containingType,
324+
ValidSyntaxFactory.IdentifierName((invocationSymbol.Name)));
325+
} else {
326+
convertedExpression = SyntaxFactory.MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, thisArgExpression, ValidSyntaxFactory.IdentifierName((invocationSymbol.Name)));
327+
}
328+
}
296329
}
297330

298331
if (invocationSymbol is IMethodSymbol m && convertedExpression is LambdaExpressionSyntax) {

‎Tests/VB/ExtensionMethodRefPropertyTests.cs‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
using System.Threading.Tasks;
2+
using Xunit;
3+
4+
namespace ICSharpCode.CodeConverter.Tests.VB;
5+
6+
public class ExtensionMethodRefPropertyTests : TestRunners.ConverterTestBase
7+
{
8+
[Fact]
9+
public async Task TestExtensionMethodRefPropertyAsync()
10+
{
11+
await TestConversionVisualBasicToCSharpAsync(
12+
@"Imports System.Runtime.CompilerServices
13+
Public Class ExtensionMethodsRefPropertyParameter
14+
Public Property Number As Integer = 3
15+
Public Sub WithExtensionMethod()
16+
Number.NegEx()
17+
End Sub
18+
Public Sub WithMethod()
19+
Neg(Number)
20+
End Sub
21+
End Class
22+
23+
Public Module MathEx
24+
<Extension()>
25+
Public Sub NegEx(ByRef num As Integer)
26+
num = -num
27+
End Sub
28+
Public Sub Neg(ByRef num As Integer)
29+
num = -num
30+
End Sub
31+
End Module", @"
32+
public partial class ExtensionMethodsRefPropertyParameter
33+
{
34+
public int Number { get; set; } = 3;
35+
public void WithExtensionMethod()
36+
{
37+
int argnum = Number;
38+
argnum.NegEx();
39+
Number = argnum;
40+
}
41+
public void WithMethod()
42+
{
43+
int argnum = Number;
44+
MathEx.Neg(ref argnum);
45+
Number = argnum;
46+
}
47+
}
48+
49+
public static partial class MathEx
50+
{
51+
public static void NegEx(this ref int num)
52+
53+
{
54+
num = -num;
55+
}
56+
public static void Neg(ref int num)
57+
{
58+
num = -num;
59+
}
60+
}", incompatibleWithAutomatedCommentTesting: true);
61+
}
62+
}

0 commit comments

Comments
 (0)