VB -> C#: Incorrect conversion of If immediately preceded by Else

[fitdev]

Seems like a pretty serious bug, as it may screw up control flow significantly and lead to completely unexpected results.

VB.Net input code

If OUTER Then
  If FOO1 Then Bar1() : Exit Sub
Else 'NOT OUTER
  If FOO2 Then
    Bar2()
    Exit Sub
  End If
End If 'OUTER

Erroneous output

if (OUTER)
  if (FOO1) {
    Bar1();
    return;
  } else if (FOO2) {
    Bar2();
    return;
  }

Expected output

if (OUTER) {
  if (FOO1) {
    Bar1();
    return;
  }
} else { //NOT OUTER
  if (FOO2) {
    Bar2();
    return;
  }
}

Details

• Product in use: VS extension
• Version in use: latest - 8.4.5.0

[GrahamTheCoder]

Thanks for the report - changes to output logic like this are indeed high priority to fix.
I can't reproduce the erroneous output in the VS extension or the web converter.

I tried this:

Public Class VisualBasicClass
    Public Sub X(OUTER As Boolean, FOO1 As Boolean, Bar1 As Action, FOO2 As Boolean, Bar2 As Action)
        If OUTER Then
            If FOO1 Then Bar1() : Exit Sub
        Else 'NOT OUTER
            If FOO2 Then
                Bar2()
                Exit Sub
            End If
        End If 'OUTER
    End Sub
End Class

which converts (correctly) to this in the VS extension:

public class VisualBasicClass
{
    public void X(bool OUTER, bool FOO1, Action Bar1, bool FOO2, Action Bar2)
    {
        if (OUTER)
        {
            if (FOO1)
            {
                Bar1();
                return;
            }
        }
        else if (FOO2) // NOT OUTER
        {
            Bar2();
            return;
        } // OUTER
    }
}

I've also tried some similar runnable snippets to ensure the same output comes out before and after conversion such VB input -> CS code converter output

Can you provide a similar repro which provides unexpected behaviour? (It may be convenient to use https://codeconverter.icsharpcode.net/ to quickly try converting snippets) Thanks!

[fitdev]

Thank you for looking into this. Strangely I cannot reproduce it with a fuller example using online code converter either.

However I did notice that the online converter is very explicit about { ... } for each level of nesting, whereas the VS extension output I received used a braceless syntax where possible, so when I actually had:

If FIRST Then
  If OUTER Then
    If FOO1 Then Bar1() : Exit Sub
  Else 'NOT OUTER
    If FOO2 Then
      Bar2()
      Exit Sub
    End If
  End If 'OUTER
End If 'FIRST

The first 2 Ifs were converted into C# using the short braceless form, which is what led to improper conversion of the FOO2 branch:

if (FIRST) // <- No { for FIRST
  if (OUTER) // <- No { for OUTER
    if (FOO1) {
      Bar1();
      return;
    } else if (FOO2) {
      Bar2();
      return;
    }
// <- No } for OUTER
// <- No } for FIRST

The problem arises in the } else if (FOO2) { line. Because:

• While } unambiguously closes the inner-most if (FOO1) {
• The else that follows it could in principle be applied to any of the 3 ifs above: FOO1, OUTER, and FIRST
• However because both the OUTER, and FIRST ifs lack the { ... } it is correctly interpreted to be a continuation of the inner-most FOO1 if.
• Whereas it should have been a continuation of the 2nd OUTER if

So the question is why VS Extension applied a different short-hand braceless formatting during the conversion which led to the incorrect result, while the Online Converter always used explicit brace formatting and thud converted correctly.

[fitdev]

Also I could reproduce this erroneous behavior is VS 2022. See the attached sample project which includes both the vb source and cs output.
VBtoCSIfTest.zip

Here's the log:

Building project 'VBtoCSIfTest\VBtoCSIfTest.vbproj' prior to conversion for maximum accuracy...

--------------------------------------------------------------------------------


Converting VBtoCSIfTest...

Phase 1 of 2:
* VBtoCSIfTest\My Project\MyNamespace.Static.1.Designer.vb - conversion started
* VBtoCSIfTest\My Project\MyNamespace.Static.3.Designer.vb - conversion started
* VBtoCSIfTest\My Project\MyNamespace.Static.2.Designer.vb - conversion started
* VBtoCSIfTest\Program.vb - conversion started
* VBtoCSIfTest\My Project\MyNamespace.Static.3.Designer.vb - conversion succeeded
* VBtoCSIfTest\My Project\MyNamespace.Static.1.Designer.vb - conversion succeeded
* VBtoCSIfTest\Program.vb - conversion succeeded
* VBtoCSIfTest\My Project\MyNamespace.Static.2.Designer.vb - conversion succeeded

Phase 2 of 2:
* VBtoCSIfTest\My Project\MyNamespace.Static.3.Designer.vb - simplification started
* VBtoCSIfTest\Program.vb - simplification started
* VBtoCSIfTest\My Project\MyNamespace.Static.1.Designer.vb - simplification started
* VBtoCSIfTest\My Project\MyNamespace.Static.2.Designer.vb - simplification started
* VBtoCSIfTest\My Project\MyNamespace.Static.1.Designer.vb - simplification succeeded
* VBtoCSIfTest\My Project\MyNamespace.Static.3.Designer.vb - simplification succeeded
* VBtoCSIfTest\Program.vb - simplification succeeded
* VBtoCSIfTest\My Project\MyNamespace.Static.2.Designer.vb - simplification succeeded
Awaiting user confirmation for overwrite....declined

Code conversion completed
5 files have been written to disk.

So, maybe something happens in the 2nd - Simplification stage?

[Yozer]

Interesting. I run the conversion on your sample project (VS 2022 17.0.5, extension: 8.4.5.0) and I got the correct result:

public static void TestMe(bool FIRST, bool OUTER, bool FOO1, bool FOO2)
{
    if (FIRST)
    {
        if (OUTER)
        {
            if (FOO1)
            {
                Bar1();
                return;
            }
        }
        else if (FOO2) // NOT OUTER
        {
            Bar2();
            return;
        } // OUTER
    } // FIRST
}

Are you sure your extension is up to date?

[fitdev]

Hmmm. Interesting indeed!

Yes, I use VS 2022 17.1 latest preview and Code Converter 8.4.5.0. And I did include in the zipped repro the exact output I got after the conversion.

Might it have something to do with Net Framework vs Dot Net 6 which I now target?

Or it has to do with .editorconfig style preferences? I do prefer to avoid braces as much as possible in my projects and do have appropriate .editorconfig rules, but why should they affect the conversion?

[GrahamTheCoder]

Thanks both. The "simplification" stage is mostly just a call to the codeanalysis (AKA roslyn) library, so the Visual Studio version can have an impact. I'll try with the latest preview and see if I can repro when I get the chance. There is an open issue #649 for using the editorconfig which I've never figured out how to do, but it's possible it's now being automatically picked up by more recent versions.

One thing you could try, is to take the correct output, put it in a csharp document in the same solution (i.e. with your editorconfig), and see if it erroneously offers the auto-fix option of removing the braces, or whether running a format command on the document causes the problem.
If so, we should definitely file that bug upstream in the roslyn repo asap. If not, it still doesn't totally rule out an issue with the library, just makes it a bit less likely.

[fitdev]

I think I found the problem, which is related to the code style but is not necessarily due to .editorconfig (because for my repro I used a completely new clean solution that did not have any associated .editorconfig):

So, when the setting under Text Editor, C#, Code Style, General, Code block preference, Prefer braces is set to Yes, then everything works correctly - the converted C# code always has braces. But when it is set to No, it produces erroneous code as I reported. I confirmed this by first changing the setting to Yes and running the conversion which produced proper results. Then changed it back to No (the way I prefer) and it produced erroneous output once again.

Formatting the document does not affect this likely because the setting is set to be applied as a Refactoring-only.

So, as a workaround, I guess i should make sure the setting is at Yes. However this raises a few questions:

• Why does this setting affect the code conversion process at all and does it depend on the Severity value (i.e. presumably it would affect the code conversion output only in Refactoring-only mode, but maybe in other modes too)?
• Will .editorconfig file affect the output as well as VS's built-in settings?
• Are there any other code style preferences that can potentially lead to similarly erroneous code conversion output?

[GrahamTheCoder]

Thanks for that, I can confirm that changing the setting does allow me to reproduce the issue converting the example I gave here:
#823 (comment)
(using VisualStudio.17.Preview/17.1.0-pre.1.1+31911.260 in case it turns out to matter)

I'll see if I can find out why this is happening

[GrahamTheCoder]

Debugging through, I can see that the issue is indeed introduced here:

CodeConverter/CodeConverter/Shared/DocumentExtensions.cs

Lines 110 to 112 in 068c7dd

	var options = await doc.GetOptionsAsync(cancellationToken);
	var newOptions = doc.Project.Language == LanguageNames.VisualBasic ? GetVBOptions(options) : GetCSOptions(options);
	return await Simplifier.ReduceAsync(withSyntaxRoot, newOptions, cancellationToken: cancellationToken);

This looks like a bug in roslyn. I'll try to repro purely against that library and file/fix upstream if I can.

Even if the fix was made today, we couldn't rely on everyone using a version of VS containing it for years, so it's worth a fix/workaround given the severity. In the file I referenced you can see it does grab the options from the Document (which Visual Studio controls). Originally I'd hoped this would include editorconfig stuff, though I've seen no evidence of that ever working.
You can see in that file I've overridden one of the VB options, and I will see if I can do the same for this case.

[GrahamTheCoder]

I did make an attempt at reproing within roslyn, but got bogged down by trying to find an existing type of test to fit this in with and ran out of time. Will try again when I have more time.
For the workaround locally, I can't manage to construct an option to override the one mentioned since so much is internal. We could of course stop using the document options altogether but I feel like that's a degradation. Unless with fresh eyes anyone can figure out how to get at the option we need, we'll likely need to use some reflection hackery unfortunately.

[fitdev]

Thank you for your efforts in this area! For my part I have temporarily changed the Prefer Braces option to Yes to avoid this issue. It's just that until this is properly fixed, perhaps a warning message should be displayed to the users informing them that they may need to change this option. Also, I am still not sure if other code style options can have adverse impact on the conversion too, but so far at least I have not encountered anything.