diff --git a/.editorconfig b/.editorconfig index 75ce4f40d..c0fa0f884 100644 --- a/.editorconfig +++ b/.editorconfig @@ -131,3 +131,18 @@ dotnet_diagnostic.RCS1077.severity = none # IDE0040: Add accessibility modifiers dotnet_style_require_accessibility_modifiers = always:warning + +# IDE0048: Add parentheses for clarity +dotnet_style_parentheses_in_relational_binary_operators = always_for_clarity:none + +# IDE0048: Add parentheses for clarity +dotnet_style_parentheses_in_other_operators = never_if_unnecessary:none + +# IDE0048: Add parentheses for clarity +dotnet_style_parentheses_in_other_binary_operators = always_for_clarity:none + +# IDE0048: Add parentheses for clarity +dotnet_style_parentheses_in_arithmetic_binary_operators = always_for_clarity:none + +# RCS1123: Add parentheses according to operator precedence. +dotnet_diagnostic.RCS1123.severity = none diff --git a/CHANGELOG.md b/CHANGELOG.md index 01d42e715..a7f37b9ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) * Exit Property should become return [#497](https://github.com/icsharpcode/CodeConverter/issues/497) * First effort converting some Xml Member Access * Avoid adding new keyword when not allowed/required [#504](https://github.com/icsharpcode/CodeConverter/issues/504) +* Avoid evaluating Select Case expression multiple times in some cases where it may be non-deterministic or have side effects [#323](https://github.com/icsharpcode/CodeConverter/issues/323) ### C# -> VB diff --git a/ICSharpCode.CodeConverter/CSharp/LambdaConverter.cs b/ICSharpCode.CodeConverter/CSharp/LambdaConverter.cs index 27c8234fe..a269aabc4 100644 --- a/ICSharpCode.CodeConverter/CSharp/LambdaConverter.cs +++ b/ICSharpCode.CodeConverter/CSharp/LambdaConverter.cs @@ -75,7 +75,7 @@ private async Task ConvertToFunctionDeclarationOrNull(VBSyntax var paramListWithTypes = param.WithParameters(SyntaxFactory.SeparatedList(paramsWithTypes)); if (potentialAncestorDeclarationOperation is IFieldInitializerOperation fieldInit) { var fieldSymbol = fieldInit.InitializedFields.Single(); - if (fieldSymbol.GetResultantVisibility() != SymbolVisibility.Public && !fieldSymbol.Type.IsDelegateReferencableByName() && await _solution.HasWriteUsagesAsync(fieldSymbol) == false) { + if (fieldSymbol.GetResultantVisibility() != SymbolVisibility.Public && !fieldSymbol.Type.IsDelegateReferencableByName() && await _solution.IsNeverWritten(fieldSymbol)) { return CreateMethodDeclaration(anonFuncOp, fieldSymbol, block, arrow); } } @@ -90,7 +90,7 @@ private async Task ConvertToFunctionDeclarationOrNull(VBSyntax if (potentialAncestorDeclarationOperation is IVariableDeclarationOperation variableDeclaration) { var variableDeclaratorOperation = variableDeclaration.Declarators.Single(); if (!variableDeclaratorOperation.Symbol.Type.IsDelegateReferencableByName() && - await _solution.HasWriteUsagesAsync(variableDeclaratorOperation.Symbol) == false) { + await _solution.IsNeverWritten(variableDeclaratorOperation.Symbol)) { //Should do: Check no (other) write usages exist: SymbolFinder.FindReferencesAsync + checking if they're an assignment LHS or out parameter return CreateLocalFunction(anonFuncOp, variableDeclaratorOperation, paramListWithTypes, block, arrow); diff --git a/ICSharpCode.CodeConverter/CSharp/MethodBodyExecutableStatementVisitor.cs b/ICSharpCode.CodeConverter/CSharp/MethodBodyExecutableStatementVisitor.cs index 935044df1..227d10063 100644 --- a/ICSharpCode.CodeConverter/CSharp/MethodBodyExecutableStatementVisitor.cs +++ b/ICSharpCode.CodeConverter/CSharp/MethodBodyExecutableStatementVisitor.cs @@ -15,6 +15,7 @@ using VBasic = Microsoft.CodeAnalysis.VisualBasic; using VBSyntax = Microsoft.CodeAnalysis.VisualBasic.Syntax; using static ICSharpCode.CodeConverter.CSharp.SyntaxKindExtensions; +using Microsoft.CodeAnalysis.Text; namespace ICSharpCode.CodeConverter.CSharp { @@ -209,13 +210,10 @@ private async Task> ConvertRedimClause(VBSyntax.Redi if (!preserve) return SingleStatement(newArrayAssignment); var lastIdentifierText = node.Expression.DescendantNodesAndSelf().OfType().Last().Identifier.Text; - var oldTargetName = GetUniqueVariableNameInScope(node, "old" + lastIdentifierText.ToPascalCase()); - var oldArrayAssignment = CreateLocalVariableDeclarationAndAssignment(oldTargetName, csTargetArrayExpression); + var (oldTargetExpression, stmts) = await GetReusableExpression(node.Expression, "old" + lastIdentifierText.ToPascalCase(), true); + var arrayCopyIfNotNull = CreateConditionalArrayCopy(node, (IdentifierNameSyntax) oldTargetExpression, csTargetArrayExpression, convertedBounds); - var oldTargetExpression = SyntaxFactory.IdentifierName(oldTargetName); - var arrayCopyIfNotNull = CreateConditionalArrayCopy(node, oldTargetExpression, csTargetArrayExpression, convertedBounds); - - return SyntaxFactory.List(new StatementSyntax[] {oldArrayAssignment, newArrayAssignment, arrayCopyIfNotNull}); + return stmts.AddRange(new StatementSyntax[] {newArrayAssignment, arrayCopyIfNotNull}); } /// @@ -557,15 +555,16 @@ public override async Task> VisitGoToStatement(VBSyn public override async Task> VisitSelectBlock(VBSyntax.SelectBlockSyntax node) { - var expr = (ExpressionSyntax) await node.SelectStatement.Expression.AcceptAsync(_expressionVisitor); - var exprWithoutTrivia = expr.WithoutTrivia().WithoutAnnotations(); + var vbExpr = node.SelectStatement.Expression; + var (reusableExpr, stmts) = await GetReusableExpression(vbExpr, "switchExpr"); + var usedConstantValues = new HashSet(); var sections = new List(); foreach (var block in node.CaseBlocks) { var labels = new List(); foreach (var c in block.CaseStatement.Cases) { if (c is VBSyntax.SimpleCaseClauseSyntax s) { - var originalExpressionSyntax = (ExpressionSyntax) await s.Value.AcceptAsync(_expressionVisitor); + var originalExpressionSyntax = (ExpressionSyntax)await s.Value.AcceptAsync(_expressionVisitor); // CSharp requires an explicit cast from the base type (e.g. int) in most cases switching on an enum var typeConversionKind = CommonConversions.TypeConversionAnalyzer.AnalyzeConversion(s.Value); var expressionSyntax = CommonConversions.TypeConversionAnalyzer.AddExplicitConversion(s.Value, originalExpressionSyntax, typeConversionKind, true); @@ -583,11 +582,11 @@ public override async Task> VisitSelectBlock(VBSynta labels.Add(SyntaxFactory.DefaultSwitchLabel()); } else if (c is VBSyntax.RelationalCaseClauseSyntax relational) { var operatorKind = VBasic.VisualBasicExtensions.Kind(relational); - var cSharpSyntaxNode = SyntaxFactory.BinaryExpression(operatorKind.ConvertToken(TokenContext.Local), exprWithoutTrivia, (ExpressionSyntax) await relational.Value.AcceptAsync(_expressionVisitor)); + var cSharpSyntaxNode = SyntaxFactory.BinaryExpression(operatorKind.ConvertToken(TokenContext.Local), reusableExpr, (ExpressionSyntax)await relational.Value.AcceptAsync(_expressionVisitor)); labels.Add(WrapInCasePatternSwitchLabelSyntax(node, cSharpSyntaxNode, treatAsBoolean: true)); } else if (c is VBSyntax.RangeCaseClauseSyntax range) { - var lowerBoundCheck = SyntaxFactory.BinaryExpression(SyntaxKind.LessThanOrEqualExpression, (ExpressionSyntax) await range.LowerBound.AcceptAsync(_expressionVisitor), exprWithoutTrivia); - var upperBoundCheck = SyntaxFactory.BinaryExpression(SyntaxKind.LessThanOrEqualExpression, exprWithoutTrivia, (ExpressionSyntax) await range.UpperBound.AcceptAsync(_expressionVisitor)); + var lowerBoundCheck = SyntaxFactory.BinaryExpression(SyntaxKind.LessThanOrEqualExpression, (ExpressionSyntax)await range.LowerBound.AcceptAsync(_expressionVisitor), reusableExpr); + var upperBoundCheck = SyntaxFactory.BinaryExpression(SyntaxKind.LessThanOrEqualExpression, reusableExpr, (ExpressionSyntax)await range.UpperBound.AcceptAsync(_expressionVisitor)); var withinBounds = SyntaxFactory.BinaryExpression(SyntaxKind.LogicalAndExpression, lowerBoundCheck, upperBoundCheck); labels.Add(WrapInCasePatternSwitchLabelSyntax(node, withinBounds, treatAsBoolean: true)); } else throw new NotSupportedException(c.Kind().ToString()); @@ -602,8 +601,37 @@ public override async Task> VisitSelectBlock(VBSynta sections.Add(SyntaxFactory.SwitchSection(SyntaxFactory.List(labels), list)); } - var switchStatementSyntax = ValidSyntaxFactory.SwitchStatement(expr, sections); - return SingleStatement(switchStatementSyntax); + var switchStatementSyntax = ValidSyntaxFactory.SwitchStatement(reusableExpr, sections); + return stmts.Add(switchStatementSyntax); + } + + private async Task<(ExpressionSyntax, SyntaxList)> GetReusableExpression(VBSyntax.ExpressionSyntax vbExpr, string variableNameBase, bool forceVariable = false) + { + var expr = (ExpressionSyntax)await vbExpr.AcceptAsync(_expressionVisitor); + SyntaxList stmts = SyntaxFactory.List(); + ExpressionSyntax reusableExpr; + if (forceVariable || !await CanEvaluateMultipleTimesAsync(vbExpr)) { + var contextNode = vbExpr.GetAncestor() ?? (VBasic.VisualBasicSyntaxNode) vbExpr.Parent; + var varName = GetUniqueVariableNameInScope(contextNode, variableNameBase); + var stmt = CreateLocalVariableDeclarationAndAssignment(varName, expr); + stmts = stmts.Add(stmt); + reusableExpr = SyntaxFactory.IdentifierName(varName); + } else { + reusableExpr = expr.WithoutTrivia().WithoutAnnotations(); + } + + return (reusableExpr, stmts); + } + + private async Task CanEvaluateMultipleTimesAsync(VBSyntax.ExpressionSyntax vbExpr) + { + return _semanticModel.GetConstantValue(vbExpr).HasValue || vbExpr.SkipParens() is VBSyntax.NameSyntax ns && await IsNeverMutatedAsync(ns); + } + + private async Task IsNeverMutatedAsync(VBSyntax.NameSyntax ns) + { + var allowedLocation = Location.Create(ns.SyntaxTree, TextSpan.FromBounds(ns.GetAncestor().SpanStart, ns.Span.End)); + return await CommonConversions.Document.Project.Solution.IsNeverWritten(_semanticModel.GetSymbolInfo(ns).Symbol, allowedLocation); } private CasePatternSwitchLabelSyntax WrapInCasePatternSwitchLabelSyntax(VBSyntax.SelectBlockSyntax node, ExpressionSyntax cSharpSyntaxNode, bool treatAsBoolean = false) @@ -629,29 +657,16 @@ private CasePatternSwitchLabelSyntax WrapInCasePatternSwitchLabelSyntax(VBSyntax public override async Task> VisitWithBlock(VBSyntax.WithBlockSyntax node) { - var withExpression = (ExpressionSyntax) await node.WithStatement.Expression.AcceptAsync(_expressionVisitor); - var generateVariableName = _semanticModel.GetTypeInfo(node.WithStatement.Expression).Type?.IsValueType != true; - - ExpressionSyntax lhsExpression; - List prefixDeclarations = new List(); - if (generateVariableName) { - string uniqueVariableNameInScope = GetUniqueVariableNameInScope(node, "withBlock"); - lhsExpression = - SyntaxFactory.IdentifierName(uniqueVariableNameInScope); - prefixDeclarations.Add( - CreateLocalVariableDeclarationAndAssignment(uniqueVariableNameInScope, withExpression)); - } else { - lhsExpression = withExpression; - } + var (lhsExpression, prefixDeclarations) = await GetReusableExpression(node.WithStatement.Expression, "withBlock"); _withBlockLhs.Push(lhsExpression); try { var statements = await ConvertStatements(node.Statements); - IEnumerable statementSyntaxs = prefixDeclarations.Concat(statements).ToArray(); - return generateVariableName + var statementSyntaxs = SyntaxFactory.List(prefixDeclarations.Concat(statements)); + return prefixDeclarations.Any() ? SingleStatement(SyntaxFactory.Block(statementSyntaxs)) - : SyntaxFactory.List(statementSyntaxs); + : statementSyntaxs; } finally { _withBlockLhs.Pop(); } diff --git a/ICSharpCode.CodeConverter/CSharp/UsageTypeAnalyzer.cs b/ICSharpCode.CodeConverter/CSharp/UsageTypeAnalyzer.cs index 3eaed796f..66e60df54 100644 --- a/ICSharpCode.CodeConverter/CSharp/UsageTypeAnalyzer.cs +++ b/ICSharpCode.CodeConverter/CSharp/UsageTypeAnalyzer.cs @@ -11,24 +11,29 @@ namespace ICSharpCode.CodeConverter.CSharp { internal static class UsageTypeAnalyzer { - public static async Task HasWriteUsagesAsync(this Solution solution, ISymbol symbol) + public static async Task IsNeverWritten(this Solution solution, ISymbol symbol, Location outsideLocation = null) + { + return symbol.AllWriteUsagesKnowable() && !await ContainsWriteUsagesFor(solution, symbol, outsideLocation); + } + + public static async Task ContainsWriteUsagesFor(Solution solution, ISymbol symbol, Location outsideLocation = null) { var references = await SymbolFinder.FindReferencesAsync(symbol, solution); var operationsReferencing = await references.SelectMany(r => r.Locations).SelectAsync(async l => { + if (l.Location.SourceTree == outsideLocation?.SourceTree && l.Location.SourceSpan.OverlapsWith(outsideLocation.SourceSpan)) return null; var semanticModel = await l.Document.GetSemanticModelAsync(); var syntaxRoot = await l.Document.GetSyntaxRootAsync(); var syntaxNode = syntaxRoot.FindNode(l.Location.SourceSpan); return semanticModel.GetOperation(syntaxNode); }); if (operationsReferencing.Any(IsWriteUsage)) return true; - if (symbol.GetResultantVisibility() == SymbolVisibility.Public) return null; return false; } private static bool IsWriteUsage(IOperation operation) { - return operation.Parent is IAssignmentOperation a && a.Target == operation - || operation is IParameterReferenceOperation pro && pro.Parameter.RefKind == RefKind.Ref; + return operation?.Parent is IAssignmentOperation a && a.Target == operation + || operation is IArgumentOperation ao && ao.Parameter.RefKind == RefKind.Ref; } } } diff --git a/ICSharpCode.CodeConverter/Util/SymbolExtensions.cs b/ICSharpCode.CodeConverter/Util/SymbolExtensions.cs index 8789ed3ca..ff80ae1b4 100644 --- a/ICSharpCode.CodeConverter/Util/SymbolExtensions.cs +++ b/ICSharpCode.CodeConverter/Util/SymbolExtensions.cs @@ -230,6 +230,12 @@ public static SymbolVisibility GetResultantVisibility(this ISymbol symbol) return visibility; } + public static bool AllWriteUsagesKnowable(this ISymbol symbol) + { + if (symbol == null) return false; + return symbol.MatchesKind(SymbolKind.Local, SymbolKind.Parameter) || symbol.GetResultantVisibility() > SymbolVisibility.Public; + } + public static bool IsAnonymousType(this ISymbol symbol) { return symbol is INamedTypeSymbol && ((INamedTypeSymbol)symbol).IsAnonymousType; diff --git a/Tests/CSharp/MissingSemanticModelInfo/ExpressionTests.cs b/Tests/CSharp/MissingSemanticModelInfo/ExpressionTests.cs index e4dcf8e46..6c2ae724c 100644 --- a/Tests/CSharp/MissingSemanticModelInfo/ExpressionTests.cs +++ b/Tests/CSharp/MissingSemanticModelInfo/ExpressionTests.cs @@ -179,7 +179,8 @@ public enum PositionEnum : int public PositionEnum PositionEnumFromString(string pS, MissingType missing) { var tPos = default(PositionEnum); - switch (pS.ToUpper()) + var switchExpr = pS.ToUpper(); + switch (switchExpr) { case ""NONE"": case ""0"": diff --git a/Tests/CSharp/StatementTests.cs b/Tests/CSharp/StatementTests.cs index b2a6bb4b1..23293a95e 100644 --- a/Tests/CSharp/StatementTests.cs +++ b/Tests/CSharp/StatementTests.cs @@ -670,13 +670,11 @@ private void Save() { using (var cmd = new SqlCommand()) { - { - var withBlock = cmd; - withBlock.ExecuteNonQuery(); - withBlock?.ExecuteNonQuery(); - withBlock.ExecuteNonQuery(); - withBlock?.ExecuteNonQuery(); - } + cmd +.ExecuteNonQuery(); + cmd?.ExecuteNonQuery(); + cmd.ExecuteNonQuery(); + cmd?.ExecuteNonQuery(); } } }"); @@ -1865,7 +1863,8 @@ public partial class TestClass { public static string TimeAgo(string x) { - switch (Strings.UCase(x)) + var switchExpr = Strings.UCase(x); + switch (switchExpr) { case var @case when @case == Strings.UCase(""a""): case var case1 when case1 == Strings.UCase(""b""): @@ -1960,6 +1959,55 @@ private bool IsSqlAlive() }"); } + [Fact] + public async Task SelectCaseWithNonDeterministicExpression() + { + await TestConversionVisualBasicToCSharpWithoutComments(@"Public Class TestClass2 + Sub DoesNotThrow() + Dim rand As New Random + Select Case rand.Next(8) + Case Is < 4 + Case 4 + Case Is > 4 + Case Else + Throw New Exception + End Select + End Sub +End Class", @"using System; + +public partial class TestClass2 +{ + public void DoesNotThrow() + { + var rand = new Random(); + var switchExpr = rand.Next(8); + switch (switchExpr) + { + case object _ when switchExpr < 4: + { + break; + } + + case 4: + { + break; + } + + case object _ when switchExpr > 4: + { + break; + } + + default: + { + throw new Exception(); + break; + } + } + } +}"); + } + [Fact] public async Task TryCatch() { diff --git a/Tests/TestData/MultiFileCharacterization/VBToCSResults/ConvertWholeSolution/VbNetStandardLib/My Project/MyNamespace.Static.Designer.cs b/Tests/TestData/MultiFileCharacterization/VBToCSResults/ConvertWholeSolution/VbNetStandardLib/My Project/MyNamespace.Static.Designer.cs index 7540a42e0..c61bfe580 100644 --- a/Tests/TestData/MultiFileCharacterization/VBToCSResults/ConvertWholeSolution/VbNetStandardLib/My Project/MyNamespace.Static.Designer.cs +++ b/Tests/TestData/MultiFileCharacterization/VBToCSResults/ConvertWholeSolution/VbNetStandardLib/My Project/MyNamespace.Static.Designer.cs @@ -645,42 +645,34 @@ private static decimal ParseDecimal(string Value, System.Globalization.NumberFor private static System.Globalization.NumberFormatInfo GetNormalizedNumberFormat(System.Globalization.NumberFormatInfo InNumberFormat) { System.Globalization.NumberFormatInfo OutNumberFormat; + if (!(InNumberFormat.CurrencyDecimalSeparator == null) && !(InNumberFormat.NumberDecimalSeparator == null) && !(InNumberFormat.CurrencyGroupSeparator == null) && !(InNumberFormat.NumberGroupSeparator == null) && InNumberFormat.CurrencyDecimalSeparator.Length == 1 && InNumberFormat.NumberDecimalSeparator.Length == 1 && InNumberFormat.CurrencyGroupSeparator.Length == 1 && InNumberFormat.NumberGroupSeparator.Length == 1 && InNumberFormat.CurrencyDecimalSeparator[0] == InNumberFormat.NumberDecimalSeparator[0] && InNumberFormat.CurrencyGroupSeparator[0] == InNumberFormat.NumberGroupSeparator[0] && InNumberFormat.CurrencyDecimalDigits == InNumberFormat.NumberDecimalDigits) + return InNumberFormat; + if (!(InNumberFormat.CurrencyDecimalSeparator == null) && !(InNumberFormat.NumberDecimalSeparator == null) && InNumberFormat.CurrencyDecimalSeparator.Length == InNumberFormat.NumberDecimalSeparator.Length && !(InNumberFormat.CurrencyGroupSeparator == null) && !(InNumberFormat.NumberGroupSeparator == null) && InNumberFormat.CurrencyGroupSeparator.Length == InNumberFormat.NumberGroupSeparator.Length) { - var withBlock = InNumberFormat; - if (!(withBlock.CurrencyDecimalSeparator == null) && !(withBlock.NumberDecimalSeparator == null) && !(withBlock.CurrencyGroupSeparator == null) && !(withBlock.NumberGroupSeparator == null) && withBlock.CurrencyDecimalSeparator.Length == 1 && withBlock.NumberDecimalSeparator.Length == 1 && withBlock.CurrencyGroupSeparator.Length == 1 && withBlock.NumberGroupSeparator.Length == 1 && withBlock.CurrencyDecimalSeparator[0] == withBlock.NumberDecimalSeparator[0] && withBlock.CurrencyGroupSeparator[0] == withBlock.NumberGroupSeparator[0] && withBlock.CurrencyDecimalDigits == withBlock.NumberDecimalDigits) - return InNumberFormat; - } - { - var withBlock1 = InNumberFormat; - if (!(withBlock1.CurrencyDecimalSeparator == null) && !(withBlock1.NumberDecimalSeparator == null) && withBlock1.CurrencyDecimalSeparator.Length == withBlock1.NumberDecimalSeparator.Length && !(withBlock1.CurrencyGroupSeparator == null) && !(withBlock1.NumberGroupSeparator == null) && withBlock1.CurrencyGroupSeparator.Length == withBlock1.NumberGroupSeparator.Length) + int i; + var loopTo = InNumberFormat.CurrencyDecimalSeparator.Length - 1; + for (i = 0; i <= loopTo; i++) { - int i; - var loopTo = withBlock1.CurrencyDecimalSeparator.Length - 1; - for (i = 0; i <= loopTo; i++) - { - if (withBlock1.CurrencyDecimalSeparator[i] != withBlock1.NumberDecimalSeparator[i]) - goto MisMatch; - } + if (InNumberFormat.CurrencyDecimalSeparator[i] != InNumberFormat.NumberDecimalSeparator[i]) + goto MisMatch; + } - var loopTo1 = withBlock1.CurrencyGroupSeparator.Length - 1; - for (i = 0; i <= loopTo1; i++) - { - if (withBlock1.CurrencyGroupSeparator[i] != withBlock1.NumberGroupSeparator[i]) - goto MisMatch; - } - return InNumberFormat; + var loopTo1 = InNumberFormat.CurrencyGroupSeparator.Length - 1; + for (i = 0; i <= loopTo1; i++) + { + if (InNumberFormat.CurrencyGroupSeparator[i] != InNumberFormat.NumberGroupSeparator[i]) + goto MisMatch; } + return InNumberFormat; } MisMatch: ; OutNumberFormat = (System.Globalization.NumberFormatInfo)InNumberFormat.Clone(); - { - var withBlock2 = OutNumberFormat; - withBlock2.CurrencyDecimalSeparator = withBlock2.NumberDecimalSeparator; - withBlock2.CurrencyGroupSeparator = withBlock2.NumberGroupSeparator; - withBlock2.CurrencyDecimalDigits = withBlock2.NumberDecimalDigits; - } + OutNumberFormat +.CurrencyDecimalSeparator = OutNumberFormat.NumberDecimalSeparator; + OutNumberFormat.CurrencyGroupSeparator = OutNumberFormat.NumberGroupSeparator; + OutNumberFormat.CurrencyDecimalDigits = OutNumberFormat.NumberDecimalDigits; return OutNumberFormat; } public static float ToSingle(string Value)