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

VB -> C#: Fix implicit `ElementAtOrDefault` usage [#362] #407

Merged
merged 2 commits into from Nov 11, 2019

Conversation

@mrmonday
Copy link
Contributor

mrmonday commented Nov 11, 2019

Ensure parameterless property access short-circuit only happens when there are no parameters. This allows the existing code for ElementAtOrDefault to be triggered correctly.

Fixes #362

Problem

The shortcut for property access caused the ElementAtOrDefault handling to not trigger.

Solution

Checking the parameter length first allows this to work as expected.

  • At least one test covering the code changed
mrmonday and others added 2 commits Nov 11, 2019
Ensure parameterless property access short-circuit only happens when
there are no parameters. This allows the existing code for
`ElementAtOrDefault` to be triggered correctly.

Fixes #362
@@ -651,7 +651,7 @@ public override async Task<CSharpSyntaxNode> VisitInvocationExpression(VBasic.Sy
return await CreateElementAccess();
}

if (expressionSymbol != null && expressionSymbol.IsKind(SymbolKind.Property)) {
if (expressionSymbol != null && expressionSymbol.IsKind(SymbolKind.Property) && invocationSymbol.GetParameters().Length == 0) {

This comment has been minimized.

Copy link
@GrahamTheCoder

GrahamTheCoder Nov 11, 2019

Member

I have a nagging feeling that there's a more general case that may still be missed. I wonder whether this if statement should be below the other one. I'll merge this since it's definitely better than it was, just wondering if you have any thoughts on more general cases - I'll post here if I think of a specific case where it's still broken.

This comment has been minimized.

Copy link
@mrmonday

mrmonday Nov 11, 2019

Author Contributor

I can't think of anything off the top of my head. I think you're probably right about the ordering - though unless we think of another case I don't think it currently makes a difference.

This comment has been minimized.

Copy link
@GrahamTheCoder

GrahamTheCoder Nov 11, 2019

Member

Yep, I've thought about it a bit more and they seem mutually exclusive so I think this covers it, thanks!

@GrahamTheCoder GrahamTheCoder merged commit bb95c9f into icsharpcode:master Nov 11, 2019
2 checks passed
2 checks passed
icsharpcode.CodeConverter Build #20191111.9 succeeded
Details
icsharpcode.CodeConverter (Job Config_Release) Job Config_Release succeeded
Details
@mrmonday mrmonday deleted the mrmonday:362-dict-missing-index branch Nov 11, 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.