Handle type mismatches, constants, instance expressions

GrahamTheCoder · GrahamTheCoder · commit f721d73f997f · 2020-04-27T10:29:55.000+01:00
diff --git a/CodeConverter/CSharp/AdditionalAssignment.cs b/CodeConverter/CSharp/AdditionalAssignment.cs
@@ -4,13 +4,13 @@ namespace ICSharpCode.CodeConverter.CSharp
 {
     internal class AdditionalAssignment : IHoistedNode
     {
-        public AdditionalAssignment(IdentifierNameSyntax identifierName, ExpressionSyntax expression)
+        public AdditionalAssignment(ExpressionSyntax lhs, ExpressionSyntax rhs)
         {
-            IdentifierName = identifierName ?? throw new System.ArgumentNullException(nameof(identifierName));
-            Expression = expression ?? throw new System.ArgumentNullException(nameof(expression));
+            RightHandSide = rhs ?? throw new System.ArgumentNullException(nameof(rhs));
+            LeftHandSide = lhs ?? throw new System.ArgumentNullException(nameof(lhs));
         }
 
-        public ExpressionSyntax Expression { get; set; }
-        public IdentifierNameSyntax IdentifierName { get; }
+        public ExpressionSyntax LeftHandSide { get; set; }
+        public ExpressionSyntax RightHandSide { get; }
     }
 }
diff --git a/CodeConverter/CSharp/ExpressionNodeVisitor.cs b/CodeConverter/CSharp/ExpressionNodeVisitor.cs
@@ -407,26 +407,38 @@ public override async Task<CSharpSyntaxNode> VisitSimpleArgument(VBasic.Syntax.S
                 return await node.Expression.AcceptAsync(TriviaConvertingExpressionVisitor);
             var symbol = GetInvocationSymbol(invocation);
             SyntaxToken token = default(SyntaxToken);
-            string argName = null;
-            RefKind refKind = RefKind.None;
-            AdditionalDeclaration local = null;
+            var convertedExpression = (ExpressionSyntax)await node.Expression.AcceptAsync(TriviaConvertingExpressionVisitor);
             if (symbol is IMethodSymbol methodSymbol) {
                 var parameters = (CommonConversions.GetCsOriginalSymbolOrNull(methodSymbol.OriginalDefinition) ?? methodSymbol).GetParameters();
-                var refType = GetRefType(node, argList, parameters, out argName, out refKind);
-                var expression = CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node.Expression, (ExpressionSyntax)await node.Expression.AcceptAsync(TriviaConvertingExpressionVisitor), defaultToCast: refKind != RefKind.None);
+                var refType = GetRefConversionType(node, argList, parameters, out var argName, out var refKind);
+                var convertedExpressionWithoutCast = convertedExpression;
+                convertedExpression = CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node.Expression, convertedExpression, defaultToCast: refKind != RefKind.None);
 
-                string prefix = $"arg{argName}";
                 if (refType != RefConversion.Inline) {
+                    string prefix = $"arg{argName}";
                     var expressionTypeInfo = _semanticModel.GetTypeInfo(node.Expression);
                     bool useVar = expressionTypeInfo.Type?.Equals(expressionTypeInfo.ConvertedType) == true && !CommonConversions.ShouldPreferExplicitType(node.Expression, expressionTypeInfo.ConvertedType, out var _);
                     var typeSyntax = CommonConversions.GetTypeSyntax(expressionTypeInfo.ConvertedType, useVar);
-                    local = _additionalLocals.Hoist(new AdditionalDeclaration(prefix, expression, typeSyntax));
-                }
-                if (refType == RefConversion.PreAndPostAssignment) {
-                    _additionalLocals.Hoist(new AdditionalAssignment(local.IdentifierName, expression));
+                    var local = _additionalLocals.Hoist(new AdditionalDeclaration(prefix, convertedExpression, typeSyntax));
+
+                    if (refType == RefConversion.PreAndPostAssignment) {
+                        var convertedLocalIdentifier = CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node.Expression, local.IdentifierName, forceSourceType: expressionTypeInfo.ConvertedType, forceTargetType: expressionTypeInfo.Type);
+                        _additionalLocals.Hoist(new AdditionalAssignment(convertedExpressionWithoutCast, convertedLocalIdentifier));
+                    }
+                    convertedExpression = local.IdentifierName;
                 }
+                token = GetRefToken(refKind);
+            } else {
+                convertedExpression = CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node.Expression, convertedExpression);
             }
 
+            var nameColon = node.IsNamed ? SyntaxFactory.NameColon((IdentifierNameSyntax)await node.NameColonEquals.Name.AcceptAsync(TriviaConvertingExpressionVisitor)) : null;
+            return SyntaxFactory.Argument(nameColon, token, convertedExpression);
+        }
+
+        private static SyntaxToken GetRefToken(RefKind refKind)
+        {
+            SyntaxToken token;
             switch (refKind) {
                 case RefKind.None:
                     token = default(SyntaxToken);
@@ -440,22 +452,11 @@ public override async Task<CSharpSyntaxNode> VisitSimpleArgument(VBasic.Syntax.S
                 default:
                     throw new ArgumentOutOfRangeException();
             }
-            var simpleExpression = await ConvertArgExpression(node, refKind);
 
-            var nameColon = node.IsNamed ? SyntaxFactory.NameColon((IdentifierNameSyntax)await node.NameColonEquals.Name.AcceptAsync(TriviaConvertingExpressionVisitor)) : null;
-            if (local == null) {
-                return SyntaxFactory.Argument(nameColon, token, simpleExpression);
-            } else {
-                return SyntaxFactory.Argument(nameColon, token, local.IdentifierName);
-            }
+            return token;
         }
 
-        private async Task<ExpressionSyntax> ConvertArgExpression(VBSyntax.SimpleArgumentSyntax node, RefKind refKind)
-        {
-            return CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(node.Expression, (ExpressionSyntax)await node.Expression.AcceptAsync(TriviaConvertingExpressionVisitor), defaultToCast: refKind != RefKind.None);
-        }
-
-        private RefConversion GetRefType(VBSyntax.ArgumentSyntax node, VBSyntax.ArgumentListSyntax argList, System.Collections.Immutable.ImmutableArray<IParameterSymbol> parameters, out string argName, out RefKind refKind)
+        private RefConversion GetRefConversionType(VBSyntax.ArgumentSyntax node, VBSyntax.ArgumentListSyntax argList, System.Collections.Immutable.ImmutableArray<IParameterSymbol> parameters, out string argName, out RefKind refKind)
         {
             var parameter = node.IsNamed && node is VBSyntax.SimpleArgumentSyntax sas 
                 ? parameters.FirstOrDefault(p => p.Name.Equals(sas.NameColonEquals.Name.Identifier.Text, StringComparison.OrdinalIgnoreCase))
@@ -918,11 +919,10 @@ async Task<CSharpSyntaxNode> CreateElementAccess()
 
 
         /// <summary>
-        /// If we need a local function,
+        /// The VB compiler actually just hoists the conditions within the same method, but that leads to the original logic looking very different.
+        /// This should be equivalent but keep closer to the look of the original source code.
+        /// See https://github.com/icsharpcode/CodeConverter/issues/310 and https://github.com/icsharpcode/CodeConverter/issues/324
         /// </summary>
-        /// <param name="invocation"></param>
-        /// <param name="invocationSymbol"></param>
-        /// <returns></returns>
         private async Task<InvocationExpressionSyntax> HoistAndCallLocalFunction(VBSyntax.InvocationExpressionSyntax invocation, IMethodSymbol invocationSymbol, ExpressionSyntax csExpression)
         {
             const string retVariableName = "ret";
@@ -945,8 +945,17 @@ private async Task<InvocationExpressionSyntax> HoistAndCallLocalFunction(VBSynta
         private bool RequiresLocalFunction(VBSyntax.InvocationExpressionSyntax invocation, IMethodSymbol invocationSymbol)
         {
             if (invocation.ArgumentList == null || IsDefinitelyExecutedInStatement(invocation)) return false;
-            return invocation.ArgumentList.Arguments
-                .Any(a => RefConversion.Inline != GetRefType(a, invocation.ArgumentList, invocationSymbol.Parameters, out var argName, out var refKind));
+            return invocation.ArgumentList.Arguments.Any(a => RequiresLocalFunction(invocation, invocationSymbol, a));
+
+            bool RequiresLocalFunction(VBSyntax.InvocationExpressionSyntax invocation, IMethodSymbol invocationSymbol, VBSyntax.ArgumentSyntax a)
+            {
+                var refConversion = GetRefConversionType(a, invocation.ArgumentList, invocationSymbol.Parameters, out var argName, out var refKind);
+                if (RefConversion.Inline == refConversion) return false;
+                if (!(a is VBSyntax.SimpleArgumentSyntax sas)) return false;
+                var argExpression = sas.Expression.SkipParens();
+                if (argExpression is VBSyntax.InstanceExpressionSyntax) return false;
+                return !_semanticModel.GetConstantValue(argExpression).HasValue;
+            }
         }
 
         private static bool IsDefinitelyExecutedInStatement(VBSyntax.InvocationExpressionSyntax invocation)
@@ -961,8 +970,6 @@ private static bool IsDefinitelyExecutedInStatement(VBSyntax.InvocationExpressio
         /// <summary>
         /// It'd be great to use _semanticModel.AnalyzeControlFlow(invocation).ExitPoints, but that doesn't account for the possibility of exceptions
         /// </summary>
-        /// <param name="n"></param>
-        /// <returns></returns>
         private static SyntaxNode GetLeftMostWithPossibleExitPoints(SyntaxNode n) => n switch
         {
             VBSyntax.VariableDeclaratorSyntax vds => vds.Initializer,
diff --git a/CodeConverter/CSharp/HoistedNodeState.cs b/CodeConverter/CSharp/HoistedNodeState.cs
@@ -91,7 +91,7 @@ public async Task<SyntaxList<StatementSyntax>> CreateLocals(VBasic.VisualBasicSy
             }
 
             foreach (var additionalAssignment in GetPostAssignments()) {
-                var assign = CS.SyntaxFactory.AssignmentExpression(CS.SyntaxKind.SimpleAssignmentExpression, additionalAssignment.Expression, additionalAssignment.IdentifierName);
+                var assign = CS.SyntaxFactory.AssignmentExpression(CS.SyntaxKind.SimpleAssignmentExpression, additionalAssignment.LeftHandSide, additionalAssignment.RightHandSide);
                 postAssignments.Add(CS.SyntaxFactory.ExpressionStatement(assign));
             }
 
diff --git a/CodeConverter/CSharp/TypeConversionAnalyzer.cs b/CodeConverter/CSharp/TypeConversionAnalyzer.cs
@@ -46,20 +46,20 @@ public TypeConversionAnalyzer(SemanticModel semanticModel, CSharpCompilation csC
             _expressionEvaluator = expressionEvaluator;
         }
 
-        public ExpressionSyntax AddExplicitConversion(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax vbNode, ExpressionSyntax csNode, bool addParenthesisIfNeeded = true, bool defaultToCast = false, bool isConst = false, ITypeSymbol forceTargetType = null)
+        public ExpressionSyntax AddExplicitConversion(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax vbNode, ExpressionSyntax csNode, bool addParenthesisIfNeeded = true, bool defaultToCast = false, bool isConst = false, ITypeSymbol forceSourceType = null, ITypeSymbol forceTargetType = null)
         {
             if (csNode == null) return null;
-            var conversionKind = AnalyzeConversion(vbNode, defaultToCast, isConst, forceTargetType);
+            var conversionKind = AnalyzeConversion(vbNode, defaultToCast, isConst, forceSourceType, forceTargetType);
             csNode = addParenthesisIfNeeded && (conversionKind == TypeConversionKind.DestructiveCast || conversionKind == TypeConversionKind.NonDestructiveCast)
                 ? VbSyntaxNodeExtensions.ParenthesizeIfPrecedenceCouldChange(vbNode, csNode)
                 : csNode;
-            return AddExplicitConversion(vbNode, csNode, conversionKind, addParenthesisIfNeeded, isConst, forceTargetType: forceTargetType);
+            return AddExplicitConversion(vbNode, csNode, conversionKind, addParenthesisIfNeeded, isConst, forceSourceType: forceSourceType, forceTargetType: forceTargetType);
         }
 
-        public ExpressionSyntax AddExplicitConversion(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax vbNode, ExpressionSyntax csNode, TypeConversionKind conversionKind, bool addParenthesisIfNeeded = false, bool isConst = false, ITypeSymbol forceTargetType = null)
+        public ExpressionSyntax AddExplicitConversion(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax vbNode, ExpressionSyntax csNode, TypeConversionKind conversionKind, bool addParenthesisIfNeeded = false, bool isConst = false, ITypeSymbol forceSourceType = null, ITypeSymbol forceTargetType = null)
         {
             var typeInfo = ModelExtensions.GetTypeInfo(_semanticModel, vbNode);
-            var vbType = typeInfo.Type;
+            var vbType = forceSourceType ?? typeInfo.Type;
             var vbConvertedType = forceTargetType ?? typeInfo.ConvertedType;
 
             if (isConst && _expressionEvaluator.GetConstantOrNull(vbNode, vbConvertedType, csNode) is ExpressionSyntax constLiteral) {
@@ -98,10 +98,10 @@ private ExpressionSyntax CreateCast(ExpressionSyntax csNode, ITypeSymbol vbConve
             return ValidSyntaxFactory.CastExpression(typeName, csNode);
         }
 
-        public TypeConversionKind AnalyzeConversion(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax vbNode, bool alwaysExplicit = false, bool isConst = false, ITypeSymbol forceTargetType = null)
+        public TypeConversionKind AnalyzeConversion(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax vbNode, bool alwaysExplicit = false, bool isConst = false, ITypeSymbol forceSourceType = null, ITypeSymbol forceTargetType = null)
         {
             var typeInfo = ModelExtensions.GetTypeInfo(_semanticModel, vbNode);
-            var vbType = typeInfo.Type;
+            var vbType = forceSourceType ?? typeInfo.Type;
             var vbConvertedType = forceTargetType ?? typeInfo.ConvertedType;
             if (vbType is null || vbConvertedType is null)
             {
diff --git a/Tests/CSharp/ExpressionTests/ByRefTests.cs b/Tests/CSharp/ExpressionTests/ByRefTests.cs
@@ -102,8 +102,10 @@ public void Foo()
         Bar(ref argclass1);
         object argclass11 = C1;
         Bar(ref argclass11);
+        C1 = (Class1)argclass11;
         object argclass12 = C1;
         Bar(ref argclass12);
+        C1 = (Class1)argclass12;
         object argclass13 = _c2;
         Bar(ref argclass13);
         object argclass14 = _c2;
@@ -359,14 +361,15 @@ Public Sub UsesRef(someBool As Boolean, someInt As Integer)
 
         If 16 > someInt OrElse TakesRef(someInt) ' Convert directly
             Console.WriteLine(1)    
-        Else If someBool AndAlso TakesRef(3) 'Requires variable before (in local function)
+        Else If someBool AndAlso TakesRef(3 * a) 'Requires variable before (in local function)
             someInt += 1
         Else If TakesRef(Prop) ' Requires variable before, and to assign back after (in local function)
             someInt -=2
         End If
         Console.WriteLine(someInt)
     End Sub
 End Class", @"using System;
+using Microsoft.VisualBasic.CompilerServices;
 
 public partial class MyTestClass
 {
@@ -398,7 +401,7 @@ public void UsesRef(bool someBool, int someInt)
         int argvrbTst3 = Prop;
         bool c = TakesRef(ref argvrbTst3);
         Prop = argvrbTst3; // Requires variable before, and to assign back after
-        bool localTakesRef() { int argvrbTst = 3; var ret = TakesRef(ref argvrbTst); return ret; }
+        bool localTakesRef() { int argvrbTst = 3 * Conversions.ToInteger(a); var ret = TakesRef(ref argvrbTst); return ret; }
 
         bool localTakesRef1() { int argvrbTst = Prop; var ret = TakesRef(ref argvrbTst); Prop = argvrbTst; return ret; }
 

Original file line number	Diff line number	Diff line change
`@@ -4,13 +4,13 @@ namespace ICSharpCode.CodeConverter.CSharp`
`4`	`4`	`{`
`5`	`5`	`internal class AdditionalAssignment : IHoistedNode`
`6`	`6`	`{`
`7`		`- public AdditionalAssignment(IdentifierNameSyntax identifierName, ExpressionSyntax expression)`
	`7`	`+ public AdditionalAssignment(ExpressionSyntax lhs, ExpressionSyntax rhs)`
`8`	`8`	`{`
`9`		`- IdentifierName = identifierName ?? throw new System.ArgumentNullException(nameof(identifierName));`
`10`		`- Expression = expression ?? throw new System.ArgumentNullException(nameof(expression));`
	`9`	`+ RightHandSide = rhs ?? throw new System.ArgumentNullException(nameof(rhs));`
	`10`	`+ LeftHandSide = lhs ?? throw new System.ArgumentNullException(nameof(lhs));`
`11`	`11`	`}`
`12`	`12`
`13`		`- public ExpressionSyntax Expression { get; set; }`
`14`		`- public IdentifierNameSyntax IdentifierName { get; }`
	`13`	`+ public ExpressionSyntax LeftHandSide { get; set; }`
	`14`	`+ public ExpressionSyntax RightHandSide { get; }`
`15`	`15`	`}`
`16`	`16`	`}`
Original file line number	Diff line number	Diff line change
`@@ -91,7 +91,7 @@ public async Task<SyntaxList<StatementSyntax>> CreateLocals(VBasic.VisualBasicSy`
`91`	`91`	`}`
`92`	`92`
`93`	`93`	`foreach (var additionalAssignment in GetPostAssignments()) {`
`94`		`- var assign = CS.SyntaxFactory.AssignmentExpression(CS.SyntaxKind.SimpleAssignmentExpression, additionalAssignment.Expression, additionalAssignment.IdentifierName);`
	`94`	`+ var assign = CS.SyntaxFactory.AssignmentExpression(CS.SyntaxKind.SimpleAssignmentExpression, additionalAssignment.LeftHandSide, additionalAssignment.RightHandSide);`
`95`	`95`	`postAssignments.Add(CS.SyntaxFactory.ExpressionStatement(assign));`
`96`	`96`	`}`
`97`	`97`
Original file line number	Diff line number	Diff line change
`@@ -46,20 +46,20 @@ public TypeConversionAnalyzer(SemanticModel semanticModel, CSharpCompilation csC`
`46`	`46`	`_expressionEvaluator = expressionEvaluator;`
`47`	`47`	`}`
`48`	`48`
`49`		`- public ExpressionSyntax AddExplicitConversion(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax vbNode, ExpressionSyntax csNode, bool addParenthesisIfNeeded = true, bool defaultToCast = false, bool isConst = false, ITypeSymbol forceTargetType = null)`
	`49`	`+ public ExpressionSyntax AddExplicitConversion(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax vbNode, ExpressionSyntax csNode, bool addParenthesisIfNeeded = true, bool defaultToCast = false, bool isConst = false, ITypeSymbol forceSourceType = null, ITypeSymbol forceTargetType = null)`
`50`	`50`	`{`
`51`	`51`	`if (csNode == null) return null;`
`52`		`- var conversionKind = AnalyzeConversion(vbNode, defaultToCast, isConst, forceTargetType);`
	`52`	`+ var conversionKind = AnalyzeConversion(vbNode, defaultToCast, isConst, forceSourceType, forceTargetType);`
`53`	`53`	`csNode = addParenthesisIfNeeded && (conversionKind == TypeConversionKind.DestructiveCast \|\| conversionKind == TypeConversionKind.NonDestructiveCast)`
`54`	`54`	`? VbSyntaxNodeExtensions.ParenthesizeIfPrecedenceCouldChange(vbNode, csNode)`
`55`	`55`	`: csNode;`
`56`		`- return AddExplicitConversion(vbNode, csNode, conversionKind, addParenthesisIfNeeded, isConst, forceTargetType: forceTargetType);`
	`56`	`+ return AddExplicitConversion(vbNode, csNode, conversionKind, addParenthesisIfNeeded, isConst, forceSourceType: forceSourceType, forceTargetType: forceTargetType);`
`57`	`57`	`}`
`58`	`58`
`59`		`- public ExpressionSyntax AddExplicitConversion(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax vbNode, ExpressionSyntax csNode, TypeConversionKind conversionKind, bool addParenthesisIfNeeded = false, bool isConst = false, ITypeSymbol forceTargetType = null)`
	`59`	`+ public ExpressionSyntax AddExplicitConversion(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax vbNode, ExpressionSyntax csNode, TypeConversionKind conversionKind, bool addParenthesisIfNeeded = false, bool isConst = false, ITypeSymbol forceSourceType = null, ITypeSymbol forceTargetType = null)`
`60`	`60`	`{`
`61`	`61`	`var typeInfo = ModelExtensions.GetTypeInfo(_semanticModel, vbNode);`
`62`		`- var vbType = typeInfo.Type;`
	`62`	`+ var vbType = forceSourceType ?? typeInfo.Type;`
`63`	`63`	`var vbConvertedType = forceTargetType ?? typeInfo.ConvertedType;`
`64`	`64`
`65`	`65`	`if (isConst && _expressionEvaluator.GetConstantOrNull(vbNode, vbConvertedType, csNode) is ExpressionSyntax constLiteral) {`
`@@ -98,10 +98,10 @@ private ExpressionSyntax CreateCast(ExpressionSyntax csNode, ITypeSymbol vbConve`
`98`	`98`	`return ValidSyntaxFactory.CastExpression(typeName, csNode);`
`99`	`99`	`}`
`100`	`100`
`101`		`- public TypeConversionKind AnalyzeConversion(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax vbNode, bool alwaysExplicit = false, bool isConst = false, ITypeSymbol forceTargetType = null)`
	`101`	`+ public TypeConversionKind AnalyzeConversion(Microsoft.CodeAnalysis.VisualBasic.Syntax.ExpressionSyntax vbNode, bool alwaysExplicit = false, bool isConst = false, ITypeSymbol forceSourceType = null, ITypeSymbol forceTargetType = null)`
`102`	`102`	`{`
`103`	`103`	`var typeInfo = ModelExtensions.GetTypeInfo(_semanticModel, vbNode);`
`104`		`- var vbType = typeInfo.Type;`
	`104`	`+ var vbType = forceSourceType ?? typeInfo.Type;`
`105`	`105`	`var vbConvertedType = forceTargetType ?? typeInfo.ConvertedType;`
`106`	`106`	`if (vbType is null \|\| vbConvertedType is null)`
`107`	`107`	`{`