VB -> C#: Static variables in VB are not converted to C#

[rlktradewright]

Input code

    Private Sub IStrategyHostController_NotifyPosition(pPosition As Integer) Implements IStrategyHostController.NotifyPosition
        Static sPrevPosition As Integer

        mView.NotifyPosition(pPosition)

        If (pPosition <> 0 And sPrevPosition = 0) Or (pPosition > 0 And sPrevPosition < 0) Or (pPosition < 0 And sPrevPosition > 0) Then
            If mModel.IsTickReplay Then
                mView.EnableTradeDrawing()
                mView.DisableTradeDrawing()
            End If
            mTradeBarNumber = mTradeBarNumber + 1
            If mModel.ShowChart Then
                Logger.Log(LogLevels.LogLevelNormal, $"New trade bar: {mTradeBarNumber} at {mModel.Ticker.Timestamp}")
                mView.NotifyNewTradeBar(mTradeBarNumber, mModel.Ticker.Timestamp)
            End If
        End If
        sPrevPosition = pPosition
    End Sub

Erroneous output

        void IStrategyHostController.NotifyPosition(int pPosition)
        {
            ;
#error Cannot convert LocalDeclarationStatementSyntax - see comment for details
            /* Cannot convert LocalDeclarationStatementSyntax, System.NotSupportedException: StaticKeyword not supported!
               at ICSharpCode.CodeConverter.CSharp.SyntaxKindExtensions.ConvertToken(SyntaxKind t, TokenContext context)
               at ICSharpCode.CodeConverter.CSharp.CommonConversions.ConvertModifier(SyntaxToken m, TokenContext context)
               at ICSharpCode.CodeConverter.CSharp.CommonConversions.<ConvertModifiersCore>d__43.MoveNext()
               at System.Linq.Enumerable.<ConcatIterator>d__59`1.MoveNext()
               at System.Linq.Enumerable.WhereEnumerableIterator`1.MoveNext()
               at System.Linq.Buffer`1..ctor(IEnumerable`1 source)
               at System.Linq.OrderedEnumerable`1.<GetEnumerator>d__1.MoveNext()
               at Microsoft.CodeAnalysis.SyntaxTokenList.CreateNode(IEnumerable`1 tokens)
               at ICSharpCode.CodeConverter.CSharp.CommonConversions.ConvertModifiers(SyntaxNode node, IReadOnlyCollection`1 modifiers, TokenContext context, Boolean isVariableOrConst, SyntaxKind[] extraCsModifierKinds)
               at ICSharpCode.CodeConverter.CSharp.MethodBodyExecutableStatementVisitor.<VisitLocalDeclarationStatement>d__31.MoveNext()
            --- End of stack trace from previous location where exception was thrown ---
               at ICSharpCode.CodeConverter.CSharp.HoistedNodeStateVisitor.<AddLocalVariablesAsync>d__6.MoveNext()
            --- End of stack trace from previous location where exception was thrown ---
               at ICSharpCode.CodeConverter.CSharp.CommentConvertingMethodBodyVisitor.<DefaultVisitInnerAsync>d__3.MoveNext()

            Input:
                    Static sPrevPosition As Integer

             */
            mView.NotifyPosition(pPosition);
            if (pPosition != 0 & sPrevPosition == 0 | pPosition > 0 & sPrevPosition < 0 | pPosition < 0 & sPrevPosition > 0)
            {
                if (mModel.IsTickReplay)
                {
                    mView.EnableTradeDrawing();
                    mView.DisableTradeDrawing();
                }

                mTradeBarNumber = mTradeBarNumber + 1;
                if (mModel.ShowChart)
                {
                    StrategyUtils.Logger.Log(LogLevels.LogLevelNormal, $"New trade bar: {mTradeBarNumber} at {mModel.Ticker.Timestamp}");
                    mView.NotifyNewTradeBar(mTradeBarNumber, mModel.Ticker.Timestamp);
                }
            }

            sPrevPosition = pPosition;
        }

Expected output

What's required here is to create a class member variable, so that the value stored in it is preserved between calls to the relevant method.

The only difficulty is to decide a name for it. The obvious way to do it is to prefix the VB variable's name with the name of the method in which it's declared, but this is rather clumsy: for example in this case, the variable would then end up named something like the following, including some additional tweaks to avoid the name being misinterpreted as an interface member:

private int _IStrategyHostController_NotifyPosition_sPrevPosition

This is rather clumsy, and makes the code difficult to read. A better option might simply be to suffix the VB variable's name with an integer that is incremented for each successive Static variable encountered in the class. So for example in this case we could have:

private int sPrevPosition__1

This does little to impair the readability, but it would mean the conversion would need to track how many Static variables it encounters during conversion of a class.

Similar considerations apply to where a VB function, sub or property is declared Static which means that all local variables declared in the procedure are implicitly static.

        `private int sPrevPosition__1;
        void IStrategyHostController.NotifyPosition(int pPosition)
        {
            mView.NotifyPosition(pPosition);
            if (pPosition != 0 & sPrevPosition__1 == 0 | pPosition > 0 & sPrevPosition__1 < 0 | pPosition < 0 & sPrevPosition__1 > 0)
            {
                if (mModel.IsTickReplay)
                {
                    mView.EnableTradeDrawing();
                    mView.DisableTradeDrawing();
                }

                mTradeBarNumber = mTradeBarNumber + 1;
                if (mModel.ShowChart)
                {
                    StrategyUtils.Logger.Log(LogLevels.LogLevelNormal, $"New trade bar: {mTradeBarNumber} at {mModel.Ticker.Timestamp}");
                    mView.NotifyNewTradeBar(mTradeBarNumber, mModel.Ticker.Timestamp);
                }
            }

            sPrevPosition__1 = pPosition;
        }

Details

• Product in use: Code Converter VS extension

• Version in use: 8.1.8

• Did you see it working in a previous version: N/A

• Any other relevant information to the issue, or your interest in contributing a fix:
  I'll consider contributing a fix, but I have very little time (ie basically none!) to do it. It may well be that it's just not worth the effort, since use of Static variables in my code base is not very common, but it would be a good learning exercise as I'm new to this tool.

[GrahamTheCoder]

Thanks for the report. I'm amazed the converter has gone this long without someone mentioning it!

[rlktradewright]

Yes, I was a bit surprised myself, as Static is quite useful at times particularly for caching results of a function or property. Its use can also be a 'code smell' indicating that a new class could be factored out.

[GrahamTheCoder]

Great observations on the naming issues by the way - the converter already has some code to find (and remember) unique names generated. I think the best approach here might be a special case in HoistedNodeStateVisitor.VisitLocalDeclarationStatement. It could detect the static keyword in the list of modifiers, then add an extra field declaration to the hoisted state. A few other changes might be needed to finish making this area of code (which was originally used only for locals), work sensibly for enforcing that that something always gets hoisted up to the field level, but I don't think it'll be too complex. Any interest in having a go at it?

Here's what I'd start with in MemberTests.cs that I think should ensure reasonable initial coverage:

        [Fact]
        public async Task TestConstructorStaticLocalConvertedToFieldAsync()
        {
            await TestConversionVisualBasicToCSharpAsync(
@"Class StaticLocalConvertedToField
    Sub New(x As Boolean)
        Static sPrevPosition As Integer = 7
        Console.WriteLine(sPrevPosition)
    End Sub
    Sub New(x As Integer)
        Static sPrevPosition As Integer
        Console.WriteLine(sPrevPosition)
    End Sub
End Class", @"TODO1");
        }

        [Fact]
        public async Task TestStaticLocalConvertedToFieldAsync()
        {
            await TestConversionVisualBasicToCSharpAsync(
@"Class StaticLocalConvertedToField
    Sub OtherName(x As Boolean)
        Static sPrevPosition As Integer
        Console.WriteLine(sPrevPosition)
    End Sub
    Function OtherName(x As Integer) as Integer
        Static sPrevPosition As Integer
        Return sPrevPosition
    End Function
End Class", @"TODO2");
        }

        [Fact]
        public async Task TestStaticLocalConvertedToStaticFieldAsync()
        {
            await TestConversionVisualBasicToCSharpAsync(
@"Class StaticLocalConvertedToField
    Shared Sub OtherName(x As Boolean)
        Static sPrevPosition As Integer
        Console.WriteLine(sPrevPosition)
    End Sub
    Sub OtherName(x As Integer)
        Static sPrevPosition As Integer
        Console.WriteLine(sPrevPosition)
    End Sub
End Class", @"TODO3");
        }

It'd then be worth checking it still works when nested deep within a lambda + if statement / for loop and in a module

[rlktradewright]

Thanks for the hints. Yes, I'll have a go at it. It would be good to get some familiarity with the code base.

[GrahamTheCoder]

Great! I should mention, this is definitely at the more challenging end for a first issue. Aside from contributing.md's general advice, I'm happy to provide more hands-on help as needed.

Here's where I started the refactor to widen the abilities of HoistedNodeState:
1372eec#r42102000
(part of #566)

[rlktradewright]

It pains me to have to admit that, one month later, I haven't been able to even get started on this - I simply already have too much on my plate. I can't realistically see this situation improving any time in the near future (say 2-3 months). Longer term, perhaps.

So if anyone else wants to pick this up, feel free but please drop a note here so I know.

[GrahamTheCoder]

Thanks @rlktradewright . I appreciate the update. Totally understand since plate-wise I'm in the same situation at the moment. Do get in touch (or just grab a bug and start coding) when things are quieter, I've always got time for assisting new contributors 😄

[Noah1989]

I am starting on this issue now. Are there any important changes to consider since this was last discussed here?

[GrahamTheCoder]

Nope nothing's changed in this area really (except perhaps needing later versions of Visual Studio and the dot net sdk installed).

I did just get a few minutes to start spiking this the other day. I've pushed commit f1a6c01 just in case it helps you see one possible direction.
In that commit I tried just hoisting it as a local to remind myself how that worked. I think ReplaceNames could help rename the variable in scope.
The locals hoisting is a bit cleverer than needed but not quite targeted right for this since it can pull things up through multiple structures within a method, whereas I think static variables are always just top-level in methods/operators/properties and pulled out to the containing class/module.
Up to you whether to use that class or just take inspiration. Either way you'll want to stash something in a field accessible to both the MethodBodyVisitor and the DeclarationNodeVisitor that creates it, and probably some kind of stack assuming nested classes need to be supported.
Feel free to push whatever and open a draft PR where we can discuss approach if you need help.

[GrahamTheCoder]

Fixed in 4b4ac0a mostly by #805

[Noah1989]

There is still a minor issue in a special case like this:

        Private Shared Function StaticTestFunction() As Integer
            Static variable As Integer
            variable = 1
            Debug.Print(variable.ToString)
            variable += 1
            Return variable
        End Function

Here, the static local gets away without an initializer, because the value is always assigned below, which apparently skips the step where a default initializer is generated. This results in a NRE when generating the field, because that currently always assumes there to be an initializer. I'll try to fix that and leave the initializer out on the field as well. However, wouldn't it be better to just ignore the static keyword in this case, since it actually does nothing. Or am I missing something?

[GrahamTheCoder]

Good find. The initializer should be moved to be a field initializer - it only gets run the first time according to the docs https://docs.microsoft.com/en-us/dotnet/visual-basic/language-reference/modifiers/static#example

I haven't checked if the initializer can reference stuff from local scope. If it does then we'd need to initialize it locally and keep track of whether it had been initialized yet in another field I think, or using a construct such as Lazy