Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C# -> VB: Support "is pattern expressions" #63

Closed
GrahamTheCoder opened this issue Mar 20, 2018 · 0 comments
Closed

C# -> VB: Support "is pattern expressions" #63

GrahamTheCoder opened this issue Mar 20, 2018 · 0 comments
Assignees
Milestone

Comments

@GrahamTheCoder
Copy link
Member

GrahamTheCoder commented Mar 20, 2018

Error processing ICSharpCode.CodeConverter\CSharp\CompilationErrorFixer.cs
In 'C:\Users\Graham\Documents\GitHub\CodeConverter\ICSharpCode.CodeConverter\CSharp\CompilationErrorFixer.cs':
System.NotImplementedException: Cannot convert IsPatternExpressionSyntax from 'potentiallyRewrittenNode is...' at character 962
 in '('
   at ICSharpCode.CodeConverter.VB.CSharpConverter.MethodBodyVisitor.DefaultVisit(SyntaxNode node)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor`1.VisitIsPatternExpression(IsPatternExpressionSyntax node)
   at Microsoft.CodeAnalysis.CSharp.Syntax.IsPatternExpressionSyntax.Accept[TResult](CSharpSyntaxVisitor`1 visitor)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor`1.Visit(SyntaxNode node)
   at ICSharpCode.CodeConverter.VB.CommentConvertingMethodBodyVisitor.DefaultVisit(SyntaxNode node)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor`1.VisitIsPatternExpression(IsPatternExpressionSyntax node)
   at Microsoft.CodeAnalysis.CSharp.Syntax.IsPatternExpressionSyntax.Accept[TResult](CSharpSyntaxVisitor`1 visitor)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor`1.Visit(SyntaxNode node)
   at ICSharpCode.CodeConverter.VB.CSharpConverter.NodesVisitor.DefaultVisit(SyntaxNode node)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor`1.VisitIsPatternExpression(IsPatternExpressionSyntax node)
   at Microsoft.CodeAnalysis.CSharp.Syntax.IsPatternExpressionSyntax.Accept[TResult](CSharpSyntaxVisitor`1 visitor)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor`1.Visit(SyntaxNode node)
   at ICSharpCode.CodeConverter.VB.CommentConvertingNodesVisitor.DefaultVisit(SyntaxNode node)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor`1.VisitIsPatternExpression(IsPatternExpressionSyntax node)
   at Microsoft.CodeAnalysis.CSharp.Syntax.IsPatternExpressionSyntax.Accept[TResult](CSharpSyntaxVisitor`1 visitor)
   at ICSharpCode.CodeConverter.VB.CSharpConverter.NodesVisitor.VisitParenthesizedExpression(ParenthesizedExpressionSyntax node)
   at Microsoft.CodeAnalysis.CSharp.Syntax.ParenthesizedExpressionSyntax.Accept[TResult](CSharpSyntaxVisitor`1 visitor)
   at Microsoft.CodeAnalysis.CSharp.CSharpSyntaxVisitor`1.Visit(SyntaxNode node)
   at ICSharpCode.CodeConverter.VB.CommentConvertingNodesVisitor.DefaultVisit(SyntaxNode node)
   ...

Option 1 - duplicate expression

The easiest way to convert this is probably to replace usages of the newly declared name with a cast of the original expression. Though this means the same (potentially expensive or even state-changing) expression may be evaluated multiple times and the repeated cast will look ugly and perform slightly worse.

Option 2 - hoist initialization

The alternative is to hoist the declaration and assignment out the front with a TryCast. With this solution there's potential for evaluating an expression that would not otherwise be evaluated, so if it's used in an else if, that would need to ideally turn into something like

    Else
    Dim casted = TryCast(uncasted)
    If casted Not Nothing Then
       ...

Option 3 - hoist declaration, inline initialization

Hoist just the variable declaration out of the block, and assign to it in the expression:

        Dim x = New TextReader
        Dim casted As StringReader
        If (casted = TryCast(x, StringReader)) IsNot Nothing Then
        End If

Can't do exactly this because VB doesn't allow inline assignment, but we already have a mechanism for doing inline assignment with a helper method.

Option 4 - use local lazy/func

Hoist the declaration and initialization out the front as a Lazy. This ensures it's only evaluated once at the point it's required, though looks a little clunky:

        Dim x = New TextReader
        Dim casted = New Lazy(Of StringReader)(Function() TryCast(x, StringReader))
        If casted.Value IsNot Nothing Then
        End If

Similarly VB has the ability to declare inline functions which pass by reference which means it can actually assign the casted variable

        Dim x = New TextReader
        Dim casted As StringReader
        Dim attemptCast = Function(ByRef output As Object) output = TryCast(x, StringReader)
        If attemptCast(casted) IsNot Nothing Then
        End If

Proposal

Make use of inline assignment helper method with option 3, and the soution to #62
This should give the neatest result at the call site that's closest to matching the C#, and the easiest to refactor from (vs hoisting more of the cast into another helper method).

It'd also be good to ensure the variable declaration is assigned Nothing to avoid compiler warnings.
__Assign's formal parameter should probably also be marked with <Out>, even though the above directly goes against that - this is necessary due to the VB compiler generating a warning irrespective of <Out> dotnet/vblang#67

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant