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

Don't merge tuple metaclasses through instance types #6342

Merged

Conversation

asterite
Copy link
Member

@asterite asterite commented Jul 6, 2018

Fixes #5384

@asterite
Copy link
Member Author

asterite commented Jul 6, 2018

To explain this a bit more...

When you have:

x = {1} || {"a"}

what type does x has? Normally it's the union of both types, so Tuple(Int32) | Tuple(String). However, for this particular case, the compiler turns it into Tuple(Int32 | String). This is done for the cases where you do stuff like this:

def foo
  if some_condition
    {:error, null}
  else
    {:ok, 1}
  end
end

The return type is Tuple(Symbol, Int32?), and it's easier to deal with (you probably don't expect a union of tuples but a tuple of unions).

But maybe that wasn't a good idea after all (it makes the compiler's code a lot more complex). This special logic also happens for named tuples.

Back to the original issues, when deciding what's the type of a union of metaclasses, we usually do it first for the instance types, and then obtain the metaclass from there. The bug was that if this is done for tuple metaclasses, the type is the metaclass of a tuple of unions (incorrect), instead of the metaclass of a union of tuples (correct). So for tuple metaclasses it's always just the simple union of metaclasses.

@ysbaddaden ysbaddaden merged commit 13edef6 into crystal-lang:master Jul 6, 2018
@ysbaddaden ysbaddaden added this to the Next milestone Jul 6, 2018
@asterite asterite deleted the bug/5384-tuple-metaclass-merge branch July 6, 2018 22:54
@RX14 RX14 modified the milestones: Next, 0.26.0 Jul 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants