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

Ensure casing matches for namespaces [#421] #424

Merged
merged 11 commits into from Nov 19, 2019

Conversation

@mrmonday
Copy link
Contributor

mrmonday commented Nov 14, 2019

Fixes #421

Problem

When converting multiple namespaces with the same name but different cases, the symbols from the semantic info get merged, despite being distinct symbols. Roslyn recognises this internally (using a MergedNamespaceDeclaration), but doesn't seem to expose it.

When this happens, CsSyntaxGenerator will always use the casing of the first namespace declaration it encountered, even if this is incorrect.

Solution

  • When converting identifiers, only update the case if this is not a declaration

  • When getting a TypeSyntax for conversion, manually replace the identifiers when the strings are identical, but cases don't match.

  • At least one test covering the code changed

The tests for this will fail until #414 is merged. I didn't want to include it in there, since it's already doing quite a lot, and my solution to this seems rather hacky.

@mrmonday mrmonday force-pushed the mrmonday:421-namespace-casing branch from 09a6ef2 to b18fce7 Nov 15, 2019
@@ -165,7 +165,17 @@ private TypeSyntax GetFuncTypeSyntax(IMethodSymbol method)
public TypeSyntax GetTypeSyntax(ITypeSymbol typeSymbol, bool useImplicitType = false)
{
if (useImplicitType || typeSymbol == null) return CreateVarTypeName();
return (TypeSyntax) CsSyntaxGenerator.TypeExpression(typeSymbol);
var vbType = SyntaxFactory.ParseTypeName(typeSymbol.ToDisplayString());

This comment has been minimized.

Copy link
@GrahamTheCoder

GrahamTheCoder Nov 15, 2019

Member

I expect a VbSyntaxGenerator would do this correctly. Might be worth pulling one through and doing the logic on a generated vb tree rather than parsing typeSymbol.ToDisplayString() which might have a few unexpected quirks.

This comment has been minimized.

Copy link
@mrmonday

mrmonday Nov 15, 2019

Author Contributor

Tried this, and it has the same problem as CsSyntaxGenerator - it uses the wrong casing.

@@ -216,7 +226,8 @@ public SyntaxToken ConvertIdentifier(SyntaxToken id, bool isAttribute = false)
if (id.SyntaxTree == _semanticModel.SyntaxTree) {
var symbol = _semanticModel.GetSymbolInfo(id.Parent).Symbol;
if (symbol != null && !String.IsNullOrWhiteSpace(symbol.Name)) {
if (text.Equals(symbol.Name, StringComparison.OrdinalIgnoreCase)) {
bool isDeclaration = symbol.Locations.Any(l => l.SourceSpan == id.Span);
if (!isDeclaration && text.Equals(symbol.Name, StringComparison.OrdinalIgnoreCase)) {

This comment has been minimized.

Copy link
@GrahamTheCoder

GrahamTheCoder Nov 15, 2019

Member

In the problem/issue descriptions it only talks about namespaces, but this bit seems like it'll have the same effect on other identifiers. Do those have the same behaviour? Examples:

  • N parts of a partial class declared with different casings
  • A method overriding a base method using a different casing
var sym = _semanticModel.GetDeclaredSymbol(node);
var sourceName = (await node.NamespaceStatement.Name.AcceptAsync(_triviaConvertingExpressionVisitor)).ToString();
var namespaceToDeclare = sym?.ToDisplayString() ?? sourceName;
int lastIndex = namespaceToDeclare.LastIndexOf(sourceName, StringComparison.OrdinalIgnoreCase);

This comment has been minimized.

Copy link
@GrahamTheCoder

GrahamTheCoder Nov 15, 2019

Member

Probably better off either fully parsing, or splitting on dots at least, otherwise you'll always get issues trying to replace "Blah" in things like Blah.SomethingContainingBlah I think
In fact it might even be legit to have a namespace that is Blah.SomethingContainingBlah.Blah I think which will wreck my dot-splitting idea too

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Nov 15, 2019

Good work on a very gnarly issue. I've tried to pick out some edge cases in the inline comments. Since this is something complicated, it'd be good to pull the logic out into a method and give it a name where possible, so someone can connect these 3 bits of code together. e.g. "WithNamespaceDeclaredCasing" (and then put xmldoc on that method saying "See WithIdentifierDeclaredCasing" along with some of the description from this PR).

Since they're separate namespaces, is it legitimate in vb to define a class Aaa.C and aaa.c?
It might be worth using a selfverifying test or two for some of these since they're weird enough that someone might later come along and change the expected result without understanding the full complexity.

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 15, 2019

There's lots of overlap, so going to attempt to address everything in one comment.

Good work on a very gnarly issue.

Thanks... It was fiddly, and as you've seen, my solution isn't ideal.

Since they're separate namespaces, is it legitimate in vb to define a class Aaa.C and aaa.c?

As far as I can tell, it isn't. Given Aaa.A and aaa.B, you get a warning and Aaa.A, and aaa.B in the assembly. For Aaa.C and aaa.c you get an error about conflicting definitions.

This makes it particularly hard to test - it's completely legitimate to reference other projects or assemblies which contain any amount of casing combinations, and you will be able to reference them from VB.

In the problem/issue descriptions it only talks about namespaces, but this bit seems like it'll have the same effect on other identifiers.

This is correct - definitely a bug. I need to check the behaviour for partial classes (I suspect it merges them). For methods we dealt with this previously - it uses the case of the method in the base class/interface.

I expect a VbSyntaxGenerator would do this correctly

I intentionally didn't use this, since I don't know what it does - if you parse the syntax you get exactly the tokens you put in, without the case changing. I'll take a look at it and see how it goes.

Probably better off either fully parsing, or splitting on dots at least

I don't think it matters here - there shouldn't be a situation where there's a mismatch like you describe, since both methods parse the same namespace, and we only do the replacement when the suffixes match. I can't think of a scenario where visiting would give Blah and the symbol would give A.FooBlah.

it'd be good to pull the logic out into a method

Agreed, I'll do that.

It might be worth using a selfverifying test

That's a good idea, I'll move the test case I think.

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 15, 2019

I've added a couple of extra tests in there for the cases you pointed out. I haven't factored anything out into methods, I wasn't sure where to draw the line or what to call the methods.

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 15, 2019

Just tested this against my original project - looks like there's still more work to do here, I'll see if I can narrow it down.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Nov 15, 2019

OK, let me know when you're happy with the PR. At that point I'll:

  • Attempt to pull out the logic into methods at the end. I suspect they will feel a bit clunky but hopefully it'll be worth it.
  • Have another think through and write a test for any situations I'm still not sure of e.g. I need to revisit the A.FooBlah situation
@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 15, 2019

I am "happy with it" in that more tests pass than before, and I don't see any regressions on the real world project I'm working against.

I'm struggling to build a test case for the real-world case I have - visual studio keeps crashing while I debug the vsix (outside of code converter), which makes it very hard to figure hit a breakpoint where I need it. I'll keep poking it.

Relatedly, is there a way to set the maximum parallelism for the converter to 1 so I don't have to deal with context switching while debugging?

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 15, 2019

I am "happy with it"

By this I mean, I'm happy for you to take over - I don't plan on any more tweaks to what's there, and anything further I do I can do on top of your changes.

@GrahamTheCoder GrahamTheCoder force-pushed the mrmonday:421-namespace-casing branch from 08ce31e to 4de75e9 Nov 17, 2019
@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Nov 17, 2019

Great, I'll have a look. You can hardcode Env.MaxDop to 1 to avoid context switching.
It's worth checking the activity log of visual studio for a stacktrace too
So long as it only occurs when the converter is running, feel free to open an issue for it even if you can't pin down the details just in case someone else has another piece of the puzzle.
If you are getting a stacktrace of any kind (even outside the converter) stick it in the issue and it might be possible to take some guesses at what could cause it.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Nov 17, 2019

In fact, I've just pushed a change to master so that if there's a debugger attached to a debug build it defaults to 1. This does mean of course that if there's a parallelism specific issue we'll need to remember to tweak that.

@GrahamTheCoder GrahamTheCoder force-pushed the mrmonday:421-namespace-casing branch from 1027291 to 5f1c0c7 Nov 17, 2019
@GrahamTheCoder GrahamTheCoder self-assigned this Nov 17, 2019
@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 18, 2019

Thanks for the Env.MaxDop tip - that was all I needed to narrow it down.

I haven't made a test case, but it should just need a variant of:

A.B.SharedMethod();

To reproduce the issue I was encountering.

Changing:

if (type != null && !expressionSymbolInfo.Symbol.IsType()) {

To:

if (type != null) {

Fixes the other case that I mentioned. Since GetTypeSyntax() was never called to get the type for MemberAccessExpressions, it ended up in ConvertIdentifier, and used the source casing, rather than the symbol casing.

I'll leave it to you to do this - don't want to tred on your toes while you're working on it.

@GrahamTheCoder GrahamTheCoder force-pushed the mrmonday:421-namespace-casing branch from e27f687 to bea9258 Nov 19, 2019
@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Nov 19, 2019

I'm done making changes, I'll hand back over to you for that last case since I've not fully understood it.
Make sure to checkout the latest since I rebased it.

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 19, 2019

I've added/fixed the test case I mentioned. Should be good to go now!

Thanks for working on this with me 🙂

@GrahamTheCoder GrahamTheCoder merged commit 45c574e into icsharpcode:master Nov 19, 2019
2 checks passed
2 checks passed
icsharpcode.CodeConverter Build #20191119.4 succeeded
Details
icsharpcode.CodeConverter (Job Config_Release) Job Config_Release succeeded
Details
@mrmonday mrmonday deleted the mrmonday:421-namespace-casing branch Nov 19, 2019
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.