icsharpcode / CodeConverter

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

Options for migration of string comparisons (helper method?) #1034

Open plqplq opened 1 year ago

plqplq commented 1 year ago

Perhaps this should not be an issue or a feature request but I think it's worthy of discussion, and we can always delete later if needed.

I'm about to Migrate perhaps 50k to 100k lines of code from VB to C#. Almost all of it has Option Compare Text.

Now when we migrate, code like this

If kv.Key = sVariableName Then

It gets converted to this:

if (CultureInfo.CurrentCulture.CompareInfo.Compare(kv.Key ?? "", sVariableName ?? "", CompareOptions.IgnoreCase | CompareOptions.IgnoreKanaType | CompareOptions.IgnoreWidth) == 0)

I'm sure I'm not the only one who doesn't want to live with that kind of code going forward, especially in a large project.

Ideally I would like improve the syntactic elegance using something like an Extension method

    [Extension]
    public static bool EqualsIgnoreCase(this string str1, string str2)
    {
        return string.Equals(str1 ?? "", str2 ?? "", StringComparison.OrdinalIgnoreCase);
    }

Then the migrated c# would be

                if (kv.Key.EqualsIgnoreCase(sVariableName)) {

I would have liked to overload the string == operator but that doesn't sound possible.

So, before I start digging in the CodeConverter source code for how to implement this, are there any other options or ways to influence the conversion as above, and does anyone else feel that there would be value in changing the code if appropriate for something like the above ?

GrahamTheCoder commented 1 year ago

Hi, thanks for getting in touch. There aren't currently any options for this, but I understand and agree with the sentiment - the output looks dreadful when sprinkled all over the place especially. In general, I've tried to avoid helper methods where possible. With the null coalesce, my hope was that people would just run a mass fixup to remove that where it's redundant, and that's only possible when it's inline. In terms of just tweaking the source locally to exactly what you would like, putting the code in a test case, and a breakpoint in VisitBinaryExpression should be a good start to see where edits are needed, or just directly here: https://github.com/icsharpcode/CodeConverter/blob/3c98bfebd380c6669c0d3f0ccdffc121767aa5ed/CodeConverter/CSharp/VisualBasicEqualityComparison.cs#L237-L248 Also see: https://github.com/icsharpcode/CodeConverter/blob/3c98bfebd380c6669c0d3f0ccdffc121767aa5ed/.github/CONTRIBUTING.md#how-to-get-started-changing-code

I'm open to a PR that adds an option to the options dialog to customise this case to what you think is best too. Let me know if you want more pointers to get started.

Here are a couple of examples that add a helper method: https://github.com/icsharpcode/CodeConverter/blob/3c98bfebd380c6669c0d3f0ccdffc121767aa5ed/CodeConverter/VB/CSharpHelperMethodDefinition.CSharpHelperMethodDefinition.cs#L53 https://github.com/icsharpcode/CodeConverter/blob/3c98bfebd380c6669c0d3f0ccdffc121767aa5ed/CodeConverter/CSharp/XmlImportContext.cs#L33

I should mention that if you just want the easiest no-code fix for your case, you can probably just regex find/replace the output to call a method, then add that method manually somewhere

plqplq commented 1 year ago

Thank you Graham, I have a few other things to close off so will take some time to get onto this in a serious way, but it looks perfectly doable, and when done I will communicate back here with what I came up with. Thanks for the pointers also. I'm sure I will have some other contributions or suggestions after converting this monster project !

plqplq commented 1 year ago

I've been trying to do some fiddling in advance of taking this seriously, by putting a break point in the source code. But when running from "codeconv" it comes up with this error:

Msbuild failed when processing the file '...myproj.vbproj' with message: C:\Program Files\Microsoft Visual Studio\2022\Professional\MSBuild\Microsoft\NuGet\17.0\Microsoft.NuGet.targets: (198, 5): Your project file doesn't list 'win' as a "RuntimeIdentifier". You should add 'win' to the "RuntimeIdentifiers" property in your project file and then re-run NuGet restore.

It compiles fine from the vsix and of course from vb itself. I've tried cleaning etc, but can't delete the obj/bin folders because they contain reference dlls.

GrahamTheCoder commented 1 year ago

Seems like a known build issue separate from the converter: https://stackoverflow.com/questions/52182158/vs-15-8-2-broke-build-tools-missing-runtimeidentifier In general though to prevent this kind of nightmare, move the crucial dlls you need into a folder called "lib" or "dlls", and reference them from there instead.

If you want to sidestep the whole mess, you could try running the Vsix startup project instead, which should be able to start another instance of visual studio with the converter running in debug mode.

Or, even more likely to "just work", copy an existing test, put in a snippet of the code you want to look at, and run that in debug mode.

plqplq commented 1 year ago

I don't know if this helps, but I just accidentally came across a decompilation of my code into C# done by visual studio. Basically I forgot to build one of the components so it gave me a c# decompilation of the previous build of the VB on the fly, which I've never seen before.

Anyway, this the code it generated is:

        If msTransformDesc = "Some Value" Then
            Throw New Security.SecurityException("Permission denied")
        End If
    if (Operators.CompareString(msTransformDesc, "Some Value", TextCompare: true) == 0)
    {
        throw new SecurityException("Permission denied");
    }

So perhaps Operators.CompareString, if it is available as implied above, would be a better translation method than the current one used. When I tried to use Operators.CompareString it wasn't recognised in either VB or C#, I also couldn't go to the definition of it, and I couldn't get much sense out of chatgpt either !

I still haven't got back to this, but thought the above might help.

Edit: See here: https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualbasic.compilerservices.operators.comparestring?view=net-7.0

So I think Operators.CompareString is a "special" thing recognised by the compiler, and is not available for coding.

iMising commented 11 months ago

Thanks for this issue When I remove "Dim s As String", ?? "" disappear, maybe it's one of the reason?