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

Fix conversion with ambiguous names [#332] #416

Closed
wants to merge 5 commits into from

Conversation

@mrmonday
Copy link
Contributor

mrmonday commented Nov 12, 2019

Fixes #332

Problem

When an unqualified name is used in VB, the converted C# did not qualify it if it would become ambiguous.

Solution

When converting a generic name, qualify it if possible to ensure we select the right reference.

  • At least one test covering the code changed

Adding the test case took a really long time - since I needed multiple files for this, I couldn't just paste in strings, and there's no documentation for how the characterisation tests work (at least that I could see). I spent a huge amount of time dealing with copy/pasting random bits of solution files, as well as trying to remove/add trailing whitespace so things matched.

Fixes #332
if (!node.Parent.IsKind(VBasic.SyntaxKind.SimpleMemberAccessExpression, VBasic.SyntaxKind.QualifiedName)) {
var symbol = GetSymbolInfoInDocument(node);
if (symbol?.ContainingSymbol != null) {
var lhs = SyntaxFactory.ParseName(symbol.ContainingSymbol.ToString());

This comment has been minimized.

Copy link
@GrahamTheCoder

GrahamTheCoder Nov 12, 2019

Member

I need to check what happens here if the containing symbol is generic

This comment has been minimized.

Copy link
@mrmonday

mrmonday Nov 12, 2019

Author Contributor

Good point.

I expect this will do the wrong thing - .ToString() includes type parameters.

I did have some code which removed them in an earlier iteration, that would obviously do the wrong thing in this iteration though.

This comment has been minimized.

Copy link
@GrahamTheCoder

GrahamTheCoder Nov 12, 2019

Member

An approach I've either used or thought about using elsewhere for types (which should cover the generic case) is SyntaxFactory.ParseName(GetTypeSyntax(declaredSymbolType).ToString()) It's a bit ugly but might help here and I sadly don't know a better way.

This comment has been minimized.

Copy link
@GrahamTheCoder

GrahamTheCoder Nov 12, 2019

Member

If you need to construct the generic type name itself you can make use of CsSyntaxGenerator.GenericName which takes symbols for the parameters.

This comment has been minimized.

Copy link
@mrmonday

mrmonday Nov 13, 2019

Author Contributor

I've added some code for this in #414, I haven't added a test case though, so I'm not 100% if it works.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Nov 12, 2019

Sorry it was a hassle to add a characterization test. Is MsBuildFixture one of the places you looked for hints? I've pushed a commit with some information. Please edit it with any crucial points that were painful to figure out, or move/reference it wherever you would have seen it.

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 13, 2019

Sorry it was a hassle to add a characterization test. Is MsBuildFixture one of the places you looked for hints? I've pushed a commit with some information. Please edit it with any crucial points that were painful to figure out, or move/reference it wherever you would have seen it.

The first place I looked was CONTRIBUTING.md, to see if there was a "here's where to add tests for multiple files".

I then looked at SolutionAndProjectTests.cs, since this was the only place I'd seen tests which touched more than one file (I wasn't sure if it was the right place, since it primarily seemed to test project/solution conversion rather than specific code). I looked briefly at MsBuildFixture, but it seemed like mostly implementation details which would be uninteresting for adding a test.

Then I took a stab in the dark to try and figure out what was going on. It took me to near completion to realise:

  • CharacterizationTestSolution/CharacterizationTestSolution.sln is the source solution
  • VBToCSCharacterization/ConvertSolution/CharacterizationTestSolution.sln is the result of converting the solution to C#
  • VBToCSCharacterization/ConvertSingleProject/CharacterizationTestSolution.sln is the result of converting only the EmptyVb project in the solution to C#
  • CSToVBCharacterization/ConvertSolution/CharacterizationTestSolution.sln is the result of converting the solution to VB
  • CSToVBCharacterization/ConvertSingleProject/CharacterizationTestSolution.sln is the result of converting only the CSharpConsoleApp project in the solution to VB

I manually copy/pasted/tweaked things in until it worked - I didn't find the _writeNewCharacterization option.

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 13, 2019

Merged into #414

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Nov 13, 2019

Thanks for your thoughts on the characterization tests. I've opened #419 so it doesn't distract from and clutter another PR, but I will try and get on it soon. Sorry again for the pain, please do highlight other specific things that could be made easier. It is tricky enough to get contributors with just the high level of domain knowledge on VB/CS/compilers needed, without the codebase adding further difficulties.

I like to keep as much of the documentation either as naming in the code or xmldoc as possible, but there's probably some overview stuff we should add around the general shape of things, e.g. The snippet detection, the automatically guessed references, the different visitors, comment conversion, the simplifier, how errors are handled.

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