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

Add initial support for VB MyClass expressions [#260] #273

Merged
merged 5 commits into from Apr 19, 2019

Conversation

Projects
None yet
2 participants
@mrmonday
Copy link
Contributor

mrmonday commented Apr 8, 2019

Closes #260

This should implement the majority of MyClass support for VB -> C#. There is one case (that I'm aware of) that is missing - that should throw an exception.

@GrahamTheCoder GrahamTheCoder force-pushed the mrmonday:260-vb-to-cs-myclass branch from ffffbff to ca40d5e Apr 8, 2019

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Apr 8, 2019

I've rebased to fix the line endings

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Apr 9, 2019

For naming, I'd go for MyClassF1.
In the place of a to-do it'd be fine to just throw. It should already get caught at the statement level and turned into a conversion error comment
(I've just had a thought, maybe rather than comments they should be #error or #warning directives but that's a separate issue)

When converting a virtual method, look for usages of its symbol within the class (there's a Roslyn API for finding usages) and see if any are a myclass reference (not quite sure how to do the second bit, but it has to be symbolic to deal with partial classes).
If found, add the normal result for the visit method to the "additional declarations" field and just return a method call to the new method.

Let me know if that doesn't make enough sense and I'll go and poke around the code a bit.

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Apr 16, 2019

I have a more complete version of this now, I'll hopefully update the PR later today.

@mrmonday mrmonday force-pushed the mrmonday:260-vb-to-cs-myclass branch from ca40d5e to c98e030 Apr 16, 2019

@mrmonday mrmonday changed the title WIP: Add initial support for VB MyClass expressions [#260] Add initial support for VB MyClass expressions [#260] Apr 16, 2019

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Apr 16, 2019

I'm pretty happy with where this is now - there's one case missing, which I've documented, and will throw an exception.

Finding if we need to introduce an extra method is a little messy - I traverse up the AST to find the class we're in, then back down again. I looked into using SymbolFinder, but that requires a Solution object, which we don't seem to have available. We also don't need to find all references, only those in the same class. We could potentially skip this, and generate the extra method for every virtual call, that's quite ugly though.

There are a few things which could probably be cleaned up, happy do do so if there's anything specific that you see.

Hopefully I've not messed up line endings again here.

@mrmonday mrmonday force-pushed the mrmonday:260-vb-to-cs-myclass branch from c98e030 to 299517b Apr 18, 2019

@mrmonday mrmonday force-pushed the mrmonday:260-vb-to-cs-myclass branch from 299517b to 6e86408 Apr 18, 2019

@GrahamTheCoder GrahamTheCoder force-pushed the mrmonday:260-vb-to-cs-myclass branch from 6e86408 to d065b49 Apr 19, 2019

@GrahamTheCoder
Copy link
Member

GrahamTheCoder left a comment

I'll make a few tweaks and merge this, good stuff!

var identifierName = "MyClass" + node.Identifier.ValueText;
var getReturn = SyntaxFactory.Block(SyntaxFactory.ParseStatement($"return this.{identifierName};"));
var getAccessor = SyntaxFactory.AccessorDeclaration(SyntaxKind.GetAccessorDeclaration, getReturn);
var setValue = SyntaxFactory.Block(SyntaxFactory.ParseStatement($"this.{identifierName} = value;"));

This comment has been minimized.

Copy link
@GrahamTheCoder

GrahamTheCoder Apr 19, 2019

Member

I'll add a call to ConvertIdentifier first and change the test to include some inconsistent VB casing.

Especially when working with strings, its really easy to lose track of what language they're in. You'll see I've started prefixing some variable names with cs or vb to help with this in complicated areas. I feel like the SyntaxFactory.Parse* methods make it easier to get lost due to encouraging more strings. I think you've used them in moderation at a sensible level in this case, so good job!

This comment has been minimized.

Copy link
@mrmonday

mrmonday Apr 19, 2019

Author Contributor

I skipped ConvertIdentifier, since it could prefix the variable with @, which would break this. Perhaps it should concat then convert?

I'll make sure to prefix with cs and vb in future.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Apr 19, 2019

Ah I'm just going to check what happens when there are multiple references to the same property via MyClass too.
EDIT: Looks like it should be fine

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Apr 19, 2019

Since all the tests pass, I've enabled updateCase (from your other PR) in all cases. If there was a situation you were trying to avoid by having it optional let me know.

@GrahamTheCoder GrahamTheCoder merged commit 850f489 into icsharpcode:master Apr 19, 2019

@mrmonday mrmonday deleted the mrmonday:260-vb-to-cs-myclass branch Apr 19, 2019

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Apr 19, 2019

updateCase was mostly to not break things - I hadn't tested everywhere it was called. If it works everywhere, that's great!

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.