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 binary expressions for nullable types in VB->C# conversion #860

Merged
merged 5 commits into from
Apr 10, 2022

Conversation

Yozer
Copy link
Member

@Yozer Yozer commented Apr 3, 2022

Fixes #840

I finally found some time to put all pieces together and create a PR.
Few things to note:

  1. I had to update roslyn version to 3.4.0 so I can use recursive patterns. I guess it would mean end of support for VS2017?
    Is converter still supporting it?
    https://docs.microsoft.com/en-us/visualstudio/extensibility/roslyn-version-support?view=vs-2022
    BTW I noticed that some projects are already using 3.4.0 version but CodeConverter project was referencing 2.8.2

  2. I couldn't force x is {} a to format nicely. Roslyn was creating something very ugly:

    x is 
    {
    } a

    I tried to play with removing trivia etc but it didn't work. The only solution I found was to use Parse method. Look at NotFormattedIsPattern.

  3. The code it generates is correct and passes all tests that I could think of.
    Unfortunately, when dealing with complex binary expressions the result can quickly become hard to read. Although Resharper/Roslynator are quite handy in simplifying them I'm still not convinced that this solution is the way to go.
    There is some room for improvement like using patterns only if required (variables, auto-properties, constants etc).

@Yozer Yozer requested a review from GrahamTheCoder April 3, 2022 15:47
@GrahamTheCoder
Copy link
Member

I'll hopefully have time to look at this later on the week. I'm OK with dropping 2017 support now so long as it is needed for this feature which it sounds like it is. To do so properly will require a few other changes which I can sort out.

I have a big reformat pr I'm working on, but it's all automated changes so I'll hopefully be able to reapply the changes on top of this easily.

@Yozer
Copy link
Member Author

Yozer commented Apr 3, 2022

@GrahamTheCoder you can merge first your big-pr - mine can wait.
We should upgrade Roslyn first anyway (in separate PR).
Would be nice to use the latest 3.x version instead of 3.4.0 which doesn't have some features like the negated-is pattern.

@GrahamTheCoder
Copy link
Member

Yeah I think it's a good idea to do the language version increase in its own PR since there are a few bits to get right
I've created an issue here with the things that come to mind: #866

@GrahamTheCoder
Copy link
Member

GrahamTheCoder commented Apr 5, 2022

I rewrote master to have the file-scoped change first, and then rebased this branch on top of it (and unintentionally rewritten the author - sorry 😬!). Even with that, my merge tool (kdiff3) struggles to know what to do here (I think because the start of the file changed so much). Winmerge and indeed Github's "resolve conflicts" button seems to do much better, and from looking at the diff it seems easy enough to port the changes across.

Sorry if what I've done ends up being confusing in a way that didn't really help...Just let me know and I can finish off merging master into it or rebasing onto master

@Yozer
Copy link
Member Author

Yozer commented Apr 5, 2022

No worries. My changes in other files were minimal so in the worst case, I can take master and manually apply my change on top.

@GrahamTheCoder
Copy link
Member

I've rebased onto master to solve the last few conflicts. Once #866 is done, we can merge this.

Copy link
Member

@GrahamTheCoder GrahamTheCoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely makes some simple cases (e.g. comparing directly to the literals true or false) much more complex than I'd like, but since it makes something work that didn't before, I think it's best to get it merged (once it's gone through the build system) and leave improvements for future PRs.

Another area I wondered about that'd be good to get tests for: Do expressions with side-effects get executed the same?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VB -> C#: Converter ignores the three-valued logic for nullable types and relational operators.
2 participants