icsharpcode / CodeConverter

Convert code from C# to VB.NET and vice versa using Roslyn
https://icsharpcode.github.io/CodeConverter/
MIT License
830 stars 217 forks source link

VB -> C#: Error converting a For loop #602

Closed FeaXR closed 4 years ago

FeaXR commented 4 years ago

Input code

For i As Integer = 0 To i < OldWords.Length
            HTMLCode = HTMLCode.Replace(OldWords(i), NewWords(i))
        Next i

Erroneous output

 for (var i = 0, loopTo = i < OldWords.Length; i <= Conversions.ToInteger(loopTo); i++)
                HTMLCode = HTMLCode.Replace(OldWords[i], NewWords[i]);

Expected output

 for (var i = 0; i < OldWords.Length; i++)
            {
                HTMLCode = HTMLCode.Replace(OldWords[i], NewWords[i]);
            }

Details

GrahamTheCoder commented 4 years ago

Hi thanks for getting in touch. Can you tell me about what problem the erroneous output is causing you? Is it that it doesn't compile (e.g. Missing reference to Conversions), does the wrong thing, or you just don't like the look of it?

I believe the loopTo variable more accurately reflects what VB does. I.e. If a method in the loop, or another thread changes the length of Old Words, I think your suggested output would behave differently to the VB.

The Conversions method looks entirely redundant, and shouldn't be generated if there's type information. Were the variables or properties defined with types?

FeaXR commented 4 years ago

The loopTo variable is non existent and never gets changed in the loop, thus rendering it useless. The OldWords array never changes, especially in the for loop. Even if it did change, I believe

for (var i = 0; i < OldWords.Length; i++) { HTMLCode = HTMLCode.Replace(OldWords[i], NewWords[i]); } would still do the same thing, since it is checking if the iterator is greater than any possible index of the array.

GrahamTheCoder commented 4 years ago

Ah, now I'm reading on a proper-sized screen I see you've used a boolean as the limit on your for loop which is very unusual and will have surprising results (I think never entering the loop). Based on your expected output (converting it back to VB) your input should probably have been this:

For i As Integer = 0 To OldWords.Length - 1
    HTMLCode = HTMLCode.Replace(OldWords(i), NewWords(i))
Next

For that input, it does then convert to C# correctly - actual converted output:

for (int i = 0, loopTo = OldWords.Length - 1; i <= loopTo; i++)
    HTMLCode = HTMLCode.Replace(OldWords[i], NewWords[i]);

But even though I think it was a bug that lead here the bug report is still relevant. The VB you gave does compile (when the variables are defined), and is a feature people sometimes intentionally use in VB, so I'll try and improve the converter support around it at some point.

Hope that makes sense. Let me know if I've misunderstood, thanks for reporting!

GrahamTheCoder commented 4 years ago

I've handled the boolean To expression case. The fix will be in the next release. I'm not planning to handle the case where OldWords is not declared, since I only usually support very common non-compiling input and this is an edge case.

FeaXR commented 4 years ago

Yes, you understood correctly :) Sadly I didn't really have control over the input, as I was just given the code and I just had to transfer it to C#. Thanks a lot for your quick reply and fix, what you are doing is amazing.

Saibamen commented 4 years ago

@FeaXR: Please retest. New version is already available