-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 automatically virtualize two types in the same hierarchy, unless one is deeper than the other #6024
Conversation
6fdb2e5
to
7067190
Compare
This was a PR I wasn't expecting to see (I thought the issue was unfixable), but you've worked out a simple, easy to understand semantic to work around it, and this PR has made my day. Thank you so much. |
Yeah, I was cleaning up branches and I accidentally deleted my old attempt at it. Then I said "maybe I can try it once more and see how bad it was". I probably did something different this time, because it didn't become that much worse. I think at least in the parser it would be worth it to add explicit casts to ASTNode in a few places to avoid the many different union types. The main issue here is that this is a guess game, unless we somehow provide a tool to list methods or places where big (or not so big) union types are formed. I don't feel comfortable generating redundant code under the hood, but that's how the language worked all the time (in some code spots, usually when you have a big hierarchy like ASTNode has, so I believe it's more relevant in parsers and compilers), and so far it didn't seem to bother anyone other than me, so ai guess it's fine. |
…s one is deeper than the other
7067190
to
26635f1
Compare
I amended the commit to add a few explanatory comments. |
I tested this in a few projects (the one that compile with master) and it seems to be working well. We can always revert it if we find out there's a huge problem with this. |
Docs about the old behaviour (virtual types) will also have to be updated. |
Hey, since this happened, my project (https://gitlab.com/WitchKnight/ParadoxPyramid) can't compile anymore... |
I'll just use a macro to generate the union type of all the subclasses but it feels really dirty |
Crystal won't merge subtypes as their parent type anymore, you must specify the parent type explicitly. Using the issue example, you can cast like so, for example: var = (rand < 0.5 ? Bar.new : Baz.new).as(Foo) |
You probably want to cast |
wow thanks ! |
Fixes #2661
Fixes #4303
What this means is that with this PR, the language will now behave like this:
Previously the type would go up to
Foo
.However:
because
Bar < Foo
, so that gets "upcasted" toFoo
(orFoo+
, if you know what I mean).This makes it possible to have more accurate information about the type of a variable. For example in the compiler's source code we have a
TypeDeclaration
node whosevar
is of typeASTNode
, where in reality it can only beVar | InstanceVar | ClassVar | Global
. So the compiler has some "otherwise, raise bug" that could be removed.Somehow, compilation times for the compiler don't get much more slower. Memory consumption goes a bit higher (most probably because now there are some union types that previously would go up to
ASTNode
but not now, so a bit more duplicated code is generated), but I don't think we should care anymore about compile times or memory consumption of the compiler (Slack alone, or Chrome, already consume 1GB from your computer and nobody seems to complain... or maybe yes, but well, "if they do it..."). Or, as usual, we can worry about such things later, or maybe a superhero will come with a compiler rewrite or some patches to improve performance 😛PLEASE, if you can, checkout this branch, compiler the compiler, and try it in your projects. Many things can happen:
[foo, bar]
, you'll probably now have to write[foo, bar] of T
for someT
because (taking the above example),Array(Foo)
is not compatible withArray(Bar | Baz)
.Once and if we can go ahead with this change, I'd like to revisit #4837, as it can be another great addition for more type safey and accuracy in the language, avoiding useless
else raise "Impossible"
and generally making the code more robust to changes.