Skip to content

Commit

Permalink
Don't try to ref foreach, Me or Using identifiers - fixes #1052
Browse files Browse the repository at this point in the history
  • Loading branch information
@GrahamTheCoder
GrahamTheCoder committed Feb 13, 2024
1 parent de43c73 commit c0745d8
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 5 deletions.
6 changes: 6 additions & 0 deletions CodeConverter/CSharp/CachedReflectedDelegates.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ public static ISymbol GetAssociatedField(this IPropertySymbol declaredSymbol) =>
public static SyntaxTree GetEmbeddedSyntaxTree(this Location loc) =>
GetCachedReflectedPropertyDelegate(loc, "PossiblyEmbeddedOrMySourceTree", ref _possiblyEmbeddedOrMySourceTree);
private static Func<Location, SyntaxTree> _possiblyEmbeddedOrMySourceTree;

public static bool GetIsUsing(this ILocalSymbol l) =>
GetCachedReflectedPropertyDelegate(l, "IsUsing", ref _isUsing);
private static Func<ILocalSymbol, bool> _isUsing;



/// <remarks>Unfortunately the roslyn UnassignedVariablesWalker and all useful collections created from it are internal only
/// Other attempts using DataFlowsIn on each reference showed that "DataFlowsIn" even from an uninitialized variable (at least in the case of ints)
Expand Down
12 changes: 7 additions & 5 deletions CodeConverter/CSharp/MethodBodyExecutableStatementVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -870,7 +870,10 @@ private static CasePatternSwitchLabelSyntax VarWhen(SyntaxToken varName, Express
SyntaxList<StatementSyntax> stmts = SyntaxFactory.List<StatementSyntax>();
ExpressionSyntax exprWithoutSideEffects;
ExpressionSyntax reusableExprWithoutSideEffects;
if (!await CanEvaluateMultipleTimesAsync(vbExpr)) {
if (IsReusableReadOnlyLocalKind(_semanticModel.GetSymbolInfo(vbExpr).Symbol) || await CanEvaluateMultipleTimesAsync(vbExpr)) {
exprWithoutSideEffects = expr;
reusableExprWithoutSideEffects = expr.WithoutSourceMapping();
} else {
TypeSyntax forceType = null;
if (_semanticModel.GetOperation(vbExpr.SkipIntoParens()).IsAssignableExpression()) {
forceType = SyntaxFactory.RefType(ValidSyntaxFactory.VarType);
Expand All @@ -880,14 +883,13 @@ private static CasePatternSwitchLabelSyntax VarWhen(SyntaxToken varName, Express
var (stmt, id) = CreateLocalVariableWithUniqueName(vbExpr, variableNameBase, expr, forceType);
stmts = stmts.Add(stmt);
reusableExprWithoutSideEffects = exprWithoutSideEffects = id;
} else {
exprWithoutSideEffects = expr;
reusableExprWithoutSideEffects = expr.WithoutSourceMapping();
}

return (reusableExprWithoutSideEffects, stmts, exprWithoutSideEffects);
}

private static bool IsReusableReadOnlyLocalKind(ISymbol symbol) => symbol is ILocalSymbol ls && (VBasic.VisualBasicExtensions.IsForEach(ls) || ls.GetIsUsing());

private (StatementSyntax Declaration, IdentifierNameSyntax Reference) CreateLocalVariableWithUniqueName(VBSyntax.ExpressionSyntax vbExpr, string variableNameBase, ExpressionSyntax expr, TypeSyntax forceType = null)
{
var contextNode = vbExpr.GetAncestor<VBSyntax.MethodBlockBaseSyntax>() ?? (VBasic.VisualBasicSyntaxNode) vbExpr.Parent;
Expand All @@ -903,7 +905,7 @@ private static CasePatternSwitchLabelSyntax VarWhen(SyntaxToken varName, Express

private async Task<bool> CanEvaluateMultipleTimesAsync(VBSyntax.ExpressionSyntax vbExpr)
{
return _semanticModel.GetConstantValue(vbExpr).HasValue || vbExpr.SkipIntoParens() is VBSyntax.NameSyntax ns && await IsNeverMutatedAsync(ns);
return _semanticModel.GetConstantValue(vbExpr).HasValue || vbExpr.IsKind(VBasic.SyntaxKind.MeExpression) || vbExpr.SkipIntoParens() is VBSyntax.NameSyntax ns && await IsNeverMutatedAsync(ns);
}

private async Task<bool> IsNeverMutatedAsync(VBSyntax.NameSyntax ns)
Expand Down
86 changes: 86 additions & 0 deletions Tests/CSharp/StatementTests/MethodStatementTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,92 @@ public partial struct SomeStruct
}");
}

[Fact]
public async Task WithBlockMeClassAsync()
{
await TestConversionVisualBasicToCSharpAsync(@"Public Class TestWithMe
Private _x As Integer
Sub S()
With Me
._x = 1
._x = 2
End With
End Sub
End Class", @"
public partial class TestWithMe
{
private int _x;
public void S()
{
_x = 1;
_x = 2;
}
}");
}

[Fact]
public async Task WithBlockMeStructAsync()
{
await TestConversionVisualBasicToCSharpAsync(@"Public Structure TestWithMe
Private _x As Integer
Sub S()
With Me
._x = 1
._x = 2
End With
End Sub
End Structure", @"
public partial struct TestWithMe
{
private int _x;
public void S()
{
_x = 1;
_x = 2;
}
}");
}

[Fact]
public async Task WithBlockForEachAsync()
{
await TestConversionVisualBasicToCSharpAsync(@"Imports System.Collections.Generic
Public Class TestWithForEachClass
Private _x As Integer
Public Shared Sub Main()
Dim x = New List(Of TestWithForEachClass)()
For Each y In x
With y
._x = 1
System.Console.Write(._x)
End With
y = Nothing
Next
End Sub
End Class", @"using System;
using System.Collections.Generic;
public partial class TestWithForEachClass
{
private int _x;
public static void Main()
{
var x = new List<TestWithForEachClass>();
foreach (var y in x)
{
y._x = 1;
Console.Write(y._x);
y = (TestWithForEachClass)null;
}
}
}
1 target compilation errors:
CS1656: Cannot assign to 'y' because it is a 'foreach iteration variable'");
}

[Fact]
public async Task NestedWithBlockAsync()
{
Expand Down

8 comments on commit c0745d8

@TymurGubayev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

But also, I kind of liked how I could see the With-blocks in translated code. Would it be much of a hassle to output something like the following?

    public void S()
    {
       { //With Me
            _x = 1;
            _x = 2;
        }
    }

Or maybe even the "canonical" form:

    public void S()
    {
       {
            var withBlock = this;
            withBlock._x = 1;
            withBlock._x = 2;
        }
    }

@GrahamTheCoder
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this isn't something I'd put in the converter since it's the wrong direction of trade off between point 3 and point 5 in the priorities here: https://github.com/icsharpcode/CodeConverter/blob/master/.github/CONTRIBUTING.md#deciding-what-the-output-should-be
I'd be more likely to try to remove the nested braces in some cases where posible

Though perhaps I've missed the reasoning behind wanting to know the old vb code in this case?

@TymurGubayev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason is exactly point 5: unfortunately, I need to look at the original code from time to time (mostly when checking if conversion introduced a bug), and in these cases, it's extremely convenient to have a similar code structure and recognizable code conversion patterns (which is why I'd prefer var withBlock = this;).

The presence of the same blocks as in the original code also helps in case they are used to "highlight" code blocks (or group logic together), something like:

With x
   'do things for 100 lines
End With

This block is collapsable in VB.NET, which would be very nice to have in C# as well.

@GrahamTheCoder
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting in the grouping aspect yeah, maybe more like a local method in some cases.
For history I actually do a commit where I rename the vb files to .cs extension right before the commit where I convert so that it's easily visible in git history if needed.
I.e. Do a recursive rename, commit, reset hard to previous commit, reset soft to rename commit, convert, commit.

@TymurGubayev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's genius!

@GrahamTheCoder
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added it as a tip in the wiki too in case it's useful to others.
I have wondered whether to ask people in a dialog box (with a "don't show this again") during project/solution conversion whether to auto-create such a commit, but it's kind of hard to explain succinctly and will probably cause more confusion than good.

@TymurGubayev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Do you want to create an intermediate commit to have uninterrupted file histories?"
(maybe along with some simple illustration like what Git Extensions does for merges).

Btw, does it need to involve resets? I think you can do the "rename commit", and then revert it (but don't commit reverted changes), thus restoring the state before renaming, then convert and commit - which is, if I'm counting correctly, one less step.

@GrahamTheCoder
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, same effect ☺

Please sign in to comment.