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

Correctly handle type promoted module symbols [#332, #375, #401] #414

Merged
merged 11 commits into from Nov 14, 2019

Conversation

@mrmonday
Copy link
Contributor

mrmonday commented Nov 12, 2019

Fixes #332
Fixes #375
Fixes #401

Problem

We get the incorrect type when analysing type promoted module symbols, causing the incorrect symbol to be used when qualifying names explicitly.

Additionally, renamed imports are removed when they match an import that's added automatically.

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

Solution

Use the containing type of the symbol to determine the correct LHS for a MemberAccessExpression. This conveniently handles type promotion, so we can remove the special case.

If there are multiple imports of the same namespace, prefer the import with an alias, and use that for name resolution.

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

  • At least one test covering the code changed
@mrmonday mrmonday mentioned this pull request Nov 12, 2019
1 of 1 task complete
@mrmonday mrmonday changed the title Correctly handle type promoted module symbols [#375] Correctly handle type promoted module symbols [#375, #401] Nov 12, 2019
moduleSymbol = null;
return false;
}

This comment has been minimized.

Copy link
@GrahamTheCoder
@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 13, 2019

Looks like the other PR accidentally got merged in here... I'll clean it up and close the other PR - they touch similar code, so it's worth doing them together.

@mrmonday mrmonday force-pushed the mrmonday:375-global-names branch from 2d9c7e6 to 77552b6 Nov 13, 2019
@mrmonday mrmonday changed the title Correctly handle type promoted module symbols [#375, #401] Correctly handle type promoted module symbols [#332, #375, #401] Nov 13, 2019
@mrmonday mrmonday mentioned this pull request Nov 13, 2019
1 of 1 task complete
@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 13, 2019

Rebased and merged everything into this single PR - I won't touch it again until I hear back from you.

I've left the clean-up commits mostly in tact, I could squash them into the relevant other commits though if you'd prefer.

@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 13, 2019

Looks like this has some issues with converting things in the global namespace - I'll see if I can narrow down a test case and take a look at it tomorrow.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

GrahamTheCoder commented Nov 13, 2019

Yep that easily surpasses my standards for git history tidiness, no need to do any more tidying.
I see green tests, nice one! Yep global case makes sense to get in this PR too, go for it

@mrmonday mrmonday force-pushed the mrmonday:375-global-names branch from ea19a49 to 4bb8e7e Nov 14, 2019
@mrmonday

This comment has been minimized.

Copy link
Contributor Author

mrmonday commented Nov 14, 2019

Global case is now handled, this should be good to go.

@mrmonday mrmonday mentioned this pull request Nov 14, 2019
1 of 1 task complete
@GrahamTheCoder GrahamTheCoder merged commit 0904218 into icsharpcode:master Nov 14, 2019
2 checks passed
2 checks passed
icsharpcode.CodeConverter Build #20191114.2 succeeded
Details
icsharpcode.CodeConverter (Job Config_Release) Job Config_Release succeeded
Details
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.