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

fixed #325: #326

Merged
merged 21 commits into from Jul 21, 2019

Conversation

@dmzaharov
Copy link

commented Jul 16, 2019

Problem

Issue #325 is rooted in the VB.NET language model, which parses (A) array element access as (B) method invocation. Because of this, difficulty of distinguishing between A and B, and rewriting the tree accordingly falls onto converter.

Solution

  • Fixes #325

  • Fixes #328

  • Fixes #335
    Fix works upon existing tree rewrite mechanism of method invocation into array element access within VisitInvocationExpression routine. Logic was originally added to rewrite VB's imaginary member .Item() into array access. Issue #325 appears of similar nature. Proposed fix extends utility method TryGetElementAccessExpressionToConvert with another expression rewrite candidate selection criteria, and lets existing rewrite logic complete the work.

  • Added new test MethodCallArrayIndexerBrackets

  • All tests pass, except 4 solution/project-related that were failing before

Dmitry Zaharov added some commits Jul 16, 2019

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

Thanks for this. Looks on the right track, I'll have a closer look now.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

I noticed some cases that were still broken so I added more tests. In order to make them pass I went in search of how the VB compiler deals with this sort of pain and discovered the Operation API. The tests now pass using that Edit: Some were missing from my test session, but they will all pass shortly...

Do you want to try writing some other tests to try and break it? e.g. cases that would hit one of the early return statements in VisitMemberAccessExpression thus missing the logic I added, or cases that use property indexers or dictionaries that aren't really tested yet. I'm keen to get this logic pretty solid since it's had a few iterations already that weren't quite right.

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

Issue #325 appears of similar nature

That was the issue you raised and had already mentioned. Is there another issue you've seen that's related?

Add back comment on what's going on here
Even with the semantic API handling the heavy lifting, and with the test coverage, this is still needed
@dmzaharov

This comment has been minimized.

Copy link
Author

commented Jul 18, 2019

What a comprehensive rewrite! Just added new test case for conditional dictionary access using imaginary "Item" member, MethodCallDictionaryAccessConditional. This test currently fails.

@dmzaharov dmzaharov closed this Jul 18, 2019

@dmzaharov dmzaharov reopened this Jul 18, 2019

Dmitry Zaharov
@dmzaharov

This comment has been minimized.

Copy link
Author

commented Jul 18, 2019

Sorry closed pull request by accident...now reopened...

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

Sorry I got a bit carried away in the rewrite - once I'd understood the insight you'd had into how VB parses things I realized there must be an implementation already in the roslyn API and figured that would let me delete a whole bunch of code. Thanks for the test. I'm busy for a few days so do you want to fix the production code to make it pass?

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

The chr issue you saw might be this one just raised #331
Let's have a go at fixing the dictionary issue your test covers on this PR but if it's proving tricky move it out separately if it's not a regression

@GrahamTheCoder

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

^ Another test case we should add here

@GrahamTheCoder GrahamTheCoder self-assigned this Jul 21, 2019

GrahamTheCoder added some commits Jul 21, 2019

@GrahamTheCoder GrahamTheCoder merged commit 17ddf6a into icsharpcode:master Jul 21, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
TestConversionVisualBasicToCSharp(@"Public Class A
Public Sub Test()
Dim dict = New Dictionary(Of String, String) From {{""a"", ""AAA""}, {""b"", ""bbb""}}
Dim v = dict?.Item(""a"")

This comment has been minimized.

Copy link
@mrmonday

mrmonday Jul 21, 2019

Contributor

I don't see a test case for plain ?("a"), without the Item anywhere?

Shouldn't make a difference, but better to explicitly test it?

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