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

First pass of combining multiple union types #20014

Closed
wants to merge 2 commits into from

Conversation

LucySMartin
Copy link

I've worked with some much simpler compilers before, thought it was about time I made an attempt at something on a more serious scale.

I had an alternate attempt to address this via an extra case within TypeOps, but that seemed to have significantly more ripple effects than I was anticipating.

Avoided using a traditional set for comparisons such that I could maintain a guarantee of ordering based on the order at the inputs. The output order is deterministic for a given input type, but I have not aimed for consistency between conceptually similar types.

My understanding is that by limiting this to inferred types, it should only impact a type that is not user provided, so use cases where a user wants a non-impactful type can be maintained through compilation.

Would likely want to expand this to cover and types as well as or types in the same code.

I'm fully expecting to be ripped apart on performance, but the big win I had spotted was to make sure I was not using subset relations between OrTypes, as that appears to be expensive.

Expecting that there's still more to do here, but keen to get back into real code again, so any feedback would be very welcome

@LucySMartin
Copy link
Author

Ok - I initially thought that was just my local setup being weird.
Ill have a bit more of a look at some of the A | Null things which I might have messed up a bit.

@LucySMartin
Copy link
Author

I've signed the CLA now.

@odersky
Copy link
Contributor

odersky commented Mar 25, 2024

I think we probably need to go deeper. I would expect a fix to do some changes to mergeIfSuper, or else directly in lub. Or maybe in simplify. Fiddling with typedType clashes with the way things are done elsewhere.

@LucySMartin
Copy link
Author

Looking at the routes to the sections your referencing (mergeIfSuper -> lub) I couldn't agree more. As soon as I see the lub method, that looks like exactly where I should have gone into - not entirely sure how I missed that one to be honest.

I did have a poke at simplify before going down the route I did, but that seemed to impact a lot of things in ways I did not quite expect (a number of negative test cases around recursive types suddenly compiled, and I really couldn't work out how that chnage had impacted them).

@odersky
Copy link
Contributor

odersky commented Mar 26, 2024

I played some more with it and think I have a solution in #20027. Interestingly the problem got fixed by removing some code.

@LucySMartin
Copy link
Author

I should check my notifications more often.
Was just about to come back with a report that the relavent section was not getting passed down as far as lub, had not quite found the ideal place to send it from (and had falsely assumed that a "hard" type had already been fully locked down somewhere i had not yet found, and trying to change it at this point would lead to invalid passing of something as a type that was not what it actually was)
Anyway, your approach is clearly cleaner, I'll close this off.

@noti0na1
Copy link
Member

Thank you very much for investing this issue! Your time and effort were truly appreciated by us.

Although the issue was resolved before you had the chance to find the solution, your insights are always welcome, and we would like you to continue participating in this project.

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.

None yet

3 participants