Don't automatically virtualize two types in the same hierarchy, unless one is deeper than the other #6024

asterite · 2018-04-28T00:15:54Z

What this means is that with this PR, the language will now behave like this:

class Foo
end

class Bar < Foo
end

class Baz < Foo
end

var = rand < 0.5 ? Bar.new : Baz.new
typeof(var) #=> Bar | Baz

Previously the type would go up to Foo.

However:

var = rand < 0.5 ? Bar.new : Foo.new
typeof(var) #=> Foo

because Bar < Foo, so that gets "upcasted" to Foo (or Foo+, 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 whose var is of type ASTNode, where in reality it can only be Var | 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:

Your code won't compile for some reason. This usually happens when you have [foo, bar], you'll probably now have to write [foo, bar] of T for some T because (taking the above example), Array(Foo) is not compatible with Array(Bar | Baz).
Your code will explode (after fixing possible errors in 1). This is what I'd like to know, as it probably means this PR is buggy.

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.

RX14 · 2018-04-28T10:52:56Z

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.

asterite · 2018-04-28T12:02:51Z

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

asterite · 2018-04-29T00:28:29Z

I amended the commit to add a few explanatory comments.

asterite · 2018-04-30T00:43:36Z

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.

asterite · 2018-04-30T00:44:11Z

Docs about the old behaviour (virtual types) will also have to be updated.

Valfather · 2018-06-25T08:05:12Z

Hey, since this happened, my project (https://gitlab.com/WitchKnight/ParadoxPyramid) can't compile anymore...
First I generated a lot of subtypes for a class using a macro, and then I used the parent type as an umbrella type for all the subtypes and passed an array of the parent type , with possibly all the different subtypes inside. Then I could just pattern match as needed if I needed to do subclass-specific operations.
Here's the error : https://play.crystal-lang.org/#/r/4dpl
I'd like to know a workaround.. tried to include subclasses of a subclass in a type alias and use array of that instead (because then it wouldn't be on the same level) but I get the same error.

Valfather · 2018-06-25T08:31:55Z

I'll just use a macro to generate the union type of all the subclasses but it feels really dirty

ysbaddaden · 2018-06-25T08:37:26Z

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)

See https://play.crystal-lang.org/#/r/4dqh

RX14 · 2018-06-25T09:07:35Z

You probably want to cast .as(Label) in your to_label method. Should be the minimal code change.

Valfather · 2018-06-25T09:10:36Z

wow thanks !

asterite self-assigned this Apr 28, 2018

asterite force-pushed the feature/dont_virtualize branch from 6fdb2e5 to 7067190 Compare April 28, 2018 00:19

RX14 approved these changes Apr 28, 2018

View reviewed changes

Don't automatically virtualize two types in the same hierarchy, unles…

26635f1

…s one is deeper than the other

asterite force-pushed the feature/dont_virtualize branch from 7067190 to 26635f1 Compare April 29, 2018 00:28

RX14 approved these changes Apr 30, 2018

View reviewed changes

asterite merged commit d02c267 into master Apr 30, 2018

asterite added kind:feature topic:compiler breaking-change labels Apr 30, 2018

RX14 added this to the Next milestone Apr 30, 2018

oprypin mentioned this pull request Jun 18, 2018

Usecase: overriding constants #3241

Closed

ysbaddaden deleted the feature/dont_virtualize branch June 25, 2018 08:30

RX14 mentioned this pull request Jun 25, 2018

crystal deps has been removed Papierkorb/qt5.cr#13

Closed

This was referenced Jul 7, 2018

Compiler crashes due to an invalid memory access #6349

Closed

Revert: Don't automatically virtualize two types in the same hierarchy, unless one is deeper than the other #6351

Merged

RX14 mentioned this pull request Aug 4, 2018

Restrict exp.@var to types in the same hierarchy #6358

Merged

RX14 mentioned this pull request Aug 23, 2018

0.26.0 change in union type inference, casting #6591

Closed

alsoijw mentioned this pull request Jul 9, 2021

[RFC] Don't unify types under the same hierarchy as a base virtual type #2661

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't automatically virtualize two types in the same hierarchy, unless one is deeper than the other #6024

Don't automatically virtualize two types in the same hierarchy, unless one is deeper than the other #6024

asterite commented Apr 28, 2018 •

edited

RX14 commented Apr 28, 2018

asterite commented Apr 28, 2018

asterite commented Apr 29, 2018

asterite commented Apr 30, 2018

asterite commented Apr 30, 2018

Valfather commented Jun 25, 2018

Valfather commented Jun 25, 2018

ysbaddaden commented Jun 25, 2018

RX14 commented Jun 25, 2018 •

edited

Valfather commented Jun 25, 2018

Don't automatically virtualize two types in the same hierarchy, unless one is deeper than the other #6024

Don't automatically virtualize two types in the same hierarchy, unless one is deeper than the other #6024

Conversation

asterite commented Apr 28, 2018 • edited

RX14 commented Apr 28, 2018

asterite commented Apr 28, 2018

asterite commented Apr 29, 2018

asterite commented Apr 30, 2018

asterite commented Apr 30, 2018

Valfather commented Jun 25, 2018

Valfather commented Jun 25, 2018

ysbaddaden commented Jun 25, 2018

RX14 commented Jun 25, 2018 • edited

Valfather commented Jun 25, 2018

asterite commented Apr 28, 2018 •

edited

RX14 commented Jun 25, 2018 •

edited