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

Automatically fix the casing of identifiers where possible #276

Merged

Conversation

Projects
None yet
2 participants
@mrmonday
Copy link
Contributor

mrmonday commented Apr 8, 2019

Closes #258

Problem

VB allows calling methods using any casing/capitalization, where C# does not. Attempt to fix the casing where possible.

Solution

  • When converting identifiers, fix-up the casing

  • This might be needed in other places too, but this fixes the original bug

  • At least one test covering the code changed

  • All tests pass

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Apr 8, 2019

Two DeclareStatement tests fail after this change - I am not sure why.

It seems the using System.Diagnostics; now get removed... I have no idea why that is though, or how to fix it - I'd appreciate some insight there.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Apr 8, 2019

Wow, that certainly is an unexpected effect based on what you changed.
Since it's in the VB source, either something went wrong with the conversion and it got silently swallowed, or this code decided to treat it differently:

compilationUnitSyntax.RemoveNodes(unusedUsings, SyntaxRemoveOptions.KeepNoTrivia);

The code to clean up usings happens in a second pass and only removes them if it finds no compilation errors missing types (in case it's actually a missing reference that the using statement would have correctly pointed to). I don't know how your changes could affect any of that since the rest of the code is identical though. Let me know if you're still struggling after sticking a breakpoing on the "using" conversion and on the "using" tidyup.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Apr 8, 2019

EDITED: Ah, I see it now, your change means it changes the case to process.GetProcesses(), then doesn't use the import anymore. But that's wrong. I'm still not sure why that happens though...

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Apr 8, 2019

Is the input code just ambiguous in VB (non-compilable) since it uses process with two different casings? If so, feel free to just change the variable name.

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Apr 8, 2019

OK, this is super weird - it seems like a Roslyn bug...

The GetSymbolInfo() for the Process in Process.GetProcesses() comes back with the local symbol process - but that shouldn't be in scope there. If it is, the whole loop shouldn't compile. Unless VB does some magic to try and guess which to use, but then we should get back either the correct symbol, or at the very least a list of candidates in the SymbolInfo (we don't).

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Apr 8, 2019

I think you might be right - it might not compile. Testing now.

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Apr 8, 2019

Gives one of these: https://docs.microsoft.com/en-us/dotnet/visual-basic/misc/bc30980

I'll fix up the test case.

@mrmonday mrmonday force-pushed the mrmonday:258-vb-to-cs-fix-casing branch from b921f91 to 43db822 Apr 8, 2019

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Apr 8, 2019

Fixed and rebased. Unfortunately my mergetool messed up the line endings, so the diff is messy... I'll see if I can fix it.

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Apr 8, 2019

I'm not sure what happened there, will leave it for now. Feel free to fix it if it bothers you.

@GrahamTheCoder GrahamTheCoder force-pushed the mrmonday:258-vb-to-cs-fix-casing branch 4 times, most recently from 54a1eeb to 66a80de Apr 8, 2019

@GrahamTheCoder GrahamTheCoder force-pushed the mrmonday:258-vb-to-cs-fix-casing branch from 66a80de to e171af5 Apr 8, 2019

@GrahamTheCoder GrahamTheCoder merged commit 32b43ae into icsharpcode:master Apr 8, 2019

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Apr 8, 2019

Ah yes, I hadn't realized the line endings were changed by your previous PRs. I've changed them back to CRLF in master. If you set git config core.autocrlf true for this repo, it should do the right thing from now on while everything's consistent.
I usually use notepad++'s edit->EOL conversion or a grep regex replace to sort things like this out depending how many files need changing.

Thanks for the PR!

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Apr 8, 2019

Sorry about that :(

It might be a good idea to add a .gitattributes file to force the line endings to be correct once they get as far as git.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Apr 8, 2019

No worries. I'll see if I can figure out the .gitattributes thing at some point. I've had issues with it in the past, but I think that was in the case where the repo was already inconsistent

@mrmonday mrmonday deleted the mrmonday:258-vb-to-cs-fix-casing branch Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.