Skip to content

Commit

Permalink
Merge branch 'select-case-expression' - fixes #323
Browse files Browse the repository at this point in the history
  • Loading branch information
@GrahamTheCoder
GrahamTheCoder committed Jan 19, 2020
2 parents d48f172 + f0385e8 commit 395e31e
Show file tree
Hide file tree
Showing 9 changed files with 156 additions and 73 deletions.
15 changes: 15 additions & 0 deletions .editorconfig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions ICSharpCode.CodeConverter/CSharp/LambdaConverter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ private async Task<CSharpSyntaxNode> 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);
}
}
Expand All @@ -90,7 +90,7 @@ private async Task<CSharpSyntaxNode> 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);
Expand Down
77 changes: 46 additions & 31 deletions ICSharpCode.CodeConverter/CSharp/MethodBodyExecutableStatementVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -209,13 +210,10 @@ private async Task<SyntaxList<StatementSyntax>> ConvertRedimClause(VBSyntax.Redi
if (!preserve) return SingleStatement(newArrayAssignment);

var lastIdentifierText = node.Expression.DescendantNodesAndSelf().OfType<VBSyntax.IdentifierNameSyntax>().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});
}

/// <summary>
Expand Down Expand Up @@ -557,15 +555,16 @@ public override async Task<SyntaxList<StatementSyntax>> VisitGoToStatement(VBSyn

public override async Task<SyntaxList<StatementSyntax>> 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<object>();
var sections = new List<SwitchSectionSyntax>();
foreach (var block in node.CaseBlocks) {
var labels = new List<SwitchLabelSyntax>();
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);
Expand All @@ -583,11 +582,11 @@ public override async Task<SyntaxList<StatementSyntax>> 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());
Expand All @@ -602,8 +601,37 @@ public override async Task<SyntaxList<StatementSyntax>> 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<StatementSyntax>)> GetReusableExpression(VBSyntax.ExpressionSyntax vbExpr, string variableNameBase, bool forceVariable = false)
{
var expr = (ExpressionSyntax)await vbExpr.AcceptAsync(_expressionVisitor);
SyntaxList<StatementSyntax> stmts = SyntaxFactory.List<StatementSyntax>();
ExpressionSyntax reusableExpr;
if (forceVariable || !await CanEvaluateMultipleTimesAsync(vbExpr)) {
var contextNode = vbExpr.GetAncestor<VBSyntax.MethodBlockBaseSyntax>() ?? (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<bool> CanEvaluateMultipleTimesAsync(VBSyntax.ExpressionSyntax vbExpr)
{
return _semanticModel.GetConstantValue(vbExpr).HasValue || vbExpr.SkipParens() is VBSyntax.NameSyntax ns && await IsNeverMutatedAsync(ns);
}

private async Task<bool> IsNeverMutatedAsync(VBSyntax.NameSyntax ns)
{
var allowedLocation = Location.Create(ns.SyntaxTree, TextSpan.FromBounds(ns.GetAncestor<VBSyntax.MethodBlockBaseSyntax>().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)
Expand All @@ -629,29 +657,16 @@ private CasePatternSwitchLabelSyntax WrapInCasePatternSwitchLabelSyntax(VBSyntax

public override async Task<SyntaxList<StatementSyntax>> 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<StatementSyntax> prefixDeclarations = new List<StatementSyntax>();
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<StatementSyntax> 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();
}
Expand Down
13 changes: 9 additions & 4 deletions ICSharpCode.CodeConverter/CSharp/UsageTypeAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,24 +11,29 @@ namespace ICSharpCode.CodeConverter.CSharp
{
internal static class UsageTypeAnalyzer
{
public static async Task<bool?> HasWriteUsagesAsync(this Solution solution, ISymbol symbol)
public static async Task<bool> IsNeverWritten(this Solution solution, ISymbol symbol, Location outsideLocation = null)
{
return symbol.AllWriteUsagesKnowable() && !await ContainsWriteUsagesFor(solution, symbol, outsideLocation);
}

public static async Task<bool> 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;
}
}
}
6 changes: 6 additions & 0 deletions ICSharpCode.CodeConverter/Util/SymbolExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion Tests/CSharp/MissingSemanticModelInfo/ExpressionTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"":
Expand Down
64 changes: 56 additions & 8 deletions Tests/CSharp/StatementTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}");
Expand Down Expand Up @@ -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""):
Expand Down Expand Up @@ -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()
{
Expand Down
Loading

0 comments on commit 395e31e

Please sign in to comment.