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

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

Open
plqplq opened this issue Sep 23, 2023 · 6 comments
Open

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

plqplq opened this issue Sep 23, 2023 · 6 comments

Comments

@plqplq
Copy link

plqplq commented Sep 23, 2023

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
Copy link
Member

GrahamTheCoder commented Sep 23, 2023

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:

if (OptionCompareTextCaseInsensitive) {
ExtraUsingDirectives.Add("System.Globalization");
var compareOptions = SyntaxFactory.Argument(GetCompareTextCaseInsensitiveCompareOptions());
var compareString = SyntaxFactory.InvocationExpression(ValidSyntaxFactory.MemberAccess(nameof(CultureInfo), nameof(CultureInfo.CurrentCulture),
nameof(CultureInfo.CompareInfo), nameof(CompareInfo.Compare)),
SyntaxFactory.ArgumentList(SyntaxFactory.SeparatedList(new[]
{SyntaxFactory.Argument(csLeft), SyntaxFactory.Argument(csRight), compareOptions})));
csLeft = compareString;
csRight = SyntaxFactory.LiteralExpression(SyntaxKind.NumericLiteralExpression,
SyntaxFactory.Literal(0));
}

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:

private IEnumerable<StatementSyntax> GetInlineHelperMethods(INamedTypeSymbol symbol)

public ClassDeclarationSyntax GenerateHelper()

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
Copy link
Author

plqplq commented Sep 23, 2023

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
Copy link
Author

plqplq commented Sep 25, 2023

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
Copy link
Member

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
Copy link
Author

plqplq commented Oct 9, 2023

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
Copy link

iMising commented Oct 17, 2023

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

@GrahamTheCoder GrahamTheCoder changed the title Migration of string comparisons Options for migration of string comparisons (helper method?) Dec 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants