Support autocasting from smaller integer to large integer, or float, for vars #9618

asterite · 2020-07-18T16:51:01Z

Implements #9565

@BrucePerens mades a good point that if you pass an Int32 variable to a method that expects Int64, then there's no problem at all. There's no precision loss, there's no danger. In general, it seems it's safe to do this as long as the range of the value is inside the range of the target type.

With this PR you can do this:

def foo(x : Int64)
  x
end

a = 1
foo(a) # => 1
typeof(foo(a)) # => Int64

Also this:

a = [] of Int64
x = 1
a << x # okay!

The compiler remains with the unintuitive names with_literal, NumberLiteralType, etc., where the name "autocast" is probably better, because now it starts applying to things other than literals. However, I don't want to rename things yet because I'd like to merge #9501 and so I don't want to introduce an unnecessary conflict.

Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>

bcardiff

If we are performing autocast on method calls with vars, the same should apply to symbols and enums.

I would expect the following to compile before merding this PR

enum Foo
  Bar
  Baz
end

def foo(x : Foo = :bar)
  pp!({x, typeof(x)})
end

s = :baz
foo(s) # => {Baz, Foo}

In a similar manner to the failing spec stated in a comment, the following should also work to make it consistent.

s = :baz
e : Foo = :bar
e = s
pp!({e, typeof(e)}) # => {Baz, Foo}

Something that is not tested in this PR and seems possible is that not only variable expressions are autocasted if needed, but any other expression also. Maybe some specs should cover this explicitly.

bcardiff · 2020-07-22T15:43:32Z

src/compiler/crystal/semantic/ast.cr

+        case self_type = self.type?
+        when IntegerType
+          case self_type.kind
+          when :i8, :u8, :i16, :u16, :i32, :u32


Why i64/u64 are not listed here?

With Int128 in mind, they should be autocasted. The fact that 64 bit integer can't be autocasted to a specific target type depends on the width of the target, and not to the 64 bit interger type kind. Right?

bcardiff · 2020-07-22T15:58:16Z

spec/compiler/semantic/automatic_cast_spec.cr

+    assert_type(%(
+      a : Int64 = 0_i64
+      x = 1
+      a = x


While building

a : Int64 = 0_i64 x = 1 a = x a

I'm getting

In foo.ignoreme.cr:30:1 30 | a = x ^ Error: type must be Int64, not (Int32 | Int64)

And the CI is seems to be complaining also https://github.com/crystal-lang/crystal/pull/9618/checks?check_run_id=885278722#step:5:2488

I didn't implement this because autocasting in this situation is a bit different. I will try to do it, but I honestly don't think it's very useful, or put another way: nobody puts type annotations in variables. But I'll try...

Oh, sorry, I injected this spec because I initially thought about implementing it, but it depended on another PR that wasn't yet merged at that point.

I'll try to make it work again.

Actually, let me just remove the spec.

This is very hard to implement. The reason is that when the compiler sees x = 1 it checks whether that can be autocasted, and since "1" can't change type it's very easy to know.

With x = a, the type of a might later change... but right now the check for "can it be autocast" happens once, and it's very hard to know that... maybe a doesn't have a type at that point. Maybe it does and it will change later on.

Can we leave this "feature" for a separate PR, or even after 1.0? I don't think this is necessary to enjoy the real benefits of this PR, which are much more common (method calls).

Ok. I'm fine not covering this.

asterite · 2020-07-22T17:25:53Z

@bcardiff

If we are performing autocast on method calls with vars, the same should apply to symbols and enums.

The title of this PR is "Support autocasting from smaller integer to large integer, or float, for vars", I never said this is for symbols and enums.

For symbols and enums this is much harder to do, I'd say "impossible" because of how difficult it is. At runtime a symbol is just an int. We can't know, at runtime, whether that symbol actually matches one of the enum members. And even if we knew that, there's no easy way to know how to translate that symbool int value into the enum int value.

bcardiff · 2020-07-22T17:34:04Z

For symbols and enums this is much harder to do [..]

Ok, I forgot about that. But I it's harder to explain, why the same autocasting works in some cases and not in others 😞

asterite · 2020-07-22T17:36:54Z

But I it's harder to explain, why the same autocasting works in some cases and not in others

Autocasting

Autocasting works in the following scenarios:

When casting an integer literal to any integer or float type where that integer value fits the target
When casting an integer type to any integer or float type where the bounds of the source type fit in the target type
When casting a symbol literal to an enum type where there's an enum member blah blah blah

Just 3 rules :-)

asterite · 2020-07-22T17:39:17Z

Something that is not tested in this PR and seems possible is that not only variable expressions are autocasted if needed, but any other expression also. Maybe some specs should cover this explicitly.

You are right! I just added such a spec. It's actually a really good spec because I didn't realize this also applies to any expression... this is really powerful/convenient :-)

bcardiff · 2020-07-22T19:40:30Z

I feel there is a bit of tension between the platform dependant int and allowing all this autocast, like the former is not that need if we end going to the platform dependant route for default int. But I guess it wont hurt, as long as the changes are sound.

Having any expression be autocasted might trigger expectation for expressions of type Int32? to be autocasted as Int64?. But auto-cast does not work on unions. This could be seen as a lack of completeness.

I guess the compiler performance for already compiling code is not affected by this PR since the exact method lookup is performed before the autocast, right?

asterite · 2020-07-22T19:44:26Z

Yeah, I agree with you. With a single recommended Int type there's less need for autocasting like this. So if we go with that, we can probably close this PR and maybe implement it later on if really needed.

asterite · 2020-07-22T19:45:59Z

However, it can help migrate existing code. If people are using Int32 right now, all calls to the std api that use Int will continue to work.

asterite · 2020-07-22T19:46:53Z

I also had to do a lot of manual casts when introducing Int, before I based that code on this PR. Once I did that the changes I had to make were much fewer. I still think this is worth on its own.

Sija · 2020-07-26T22:20:21Z

@asterite Mate, what this pr-closing-spree is about? :/

asterite · 2020-07-26T22:50:25Z

Old PRs, and none of these PRs are going to be merged. And I usually code in a hacky way so I prefer others to implement these things in a more robust way. I don't have time anymore to think about how to solve these things.

BrucePerens · 2020-07-26T23:07:17Z

Month-old PR, and I am tempted to refile it and let it sit until addressed again. Thanks Bruce

…

On Sun, Jul 26, 2020, 3:50 PM Ary Borenszweig ***@***.***> wrote: Old PRs, and none of these PRs are going to be merged. And I usually code in a hacky way so I prefer others to implement these things in a more robust way. I don't have time anymore to think about how to solve these things. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9618 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAINM33AVYCXW6VSCE6BZGLR5SXLZANCNFSM4PADXL5A> .

Sija · 2021-02-03T12:09:57Z

Could this get reopened, please?

asterite · 2021-02-03T12:30:39Z

Not by me :-)

Sija · 2021-02-03T12:43:13Z

I don't care by whom, if there's a will to push this forward any1 can copy-paste it into the new PR, that's not a problem ;) What I'd like to know is whether that would be pursued at all by any of the core team members.

asterite · 2021-02-03T12:54:38Z

Let's reopen it. I'd love to have this in the language.

Oh, actually, it seems it can't be reopened.

BrucePerens · 2021-02-03T15:08:34Z

I continue to write constructs like 0i64, which seem silly to me but the compiler won't take 0. It doesn't seem that the addition of a machine-native Int will help in all cases.

…

On Wed, Feb 3, 2021, 4:55 AM Ary Borenszweig ***@***.***> wrote: Reopened #9618 <#9618>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9618 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAINM37Y7WI4WOKID2OODETS5FBSVANCNFSM4PADXL5A> .

asterite and others added 8 commits July 17, 2020 21:53

Suppoort autocasting from smaller integer to large integer for vars

309ad3d

Update spec/compiler/semantic/automatic_cast_spec.cr

683a3b3

Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>

Shorter way to write supports_autocast?

3538f36

with_literals -> with_autocast, LiteralType -> AutocastType

524f2aa

Replace SymbolLiteralType with SymbolType in macro methods specs

6fd4812

Support autocasting between integer and float type variables

c4daab2

Rename automatic_cast.cr to automatic_cast_spec.cr

ec3f474

Implement pending spec

1dd2ec0

asterite added kind:feature topic:compiler:semantic labels Jul 18, 2020

asterite mentioned this pull request Jul 18, 2020

Support autocasting from smaller integer to large integer for vars #9569

Closed

asterite requested a review from bcardiff July 18, 2020 17:16

bcardiff reviewed Jul 22, 2020

View reviewed changes

Remove wrong spec

4d81b9a

asterite added 2 commits July 22, 2020 14:37

Allow autocasting from Int64 to Int128

ff038ff

Add a spec to prove that any expression can be autocast, not just vars

438355e

asterite closed this Jul 26, 2020

asterite deleted the feature/integer-autocast branch July 26, 2020 14:48

asterite mentioned this pull request Feb 3, 2021

Allow Int64 values within IO#read_at #10356

Merged

asterite restored the feature/integer-autocast branch February 3, 2021 12:54

asterite reopened this Feb 3, 2021

beta-ziliani mentioned this pull request Nov 10, 2021

Opt-in autocast for integers and floats #11431

Merged

straight-shoota closed this in #11431 Nov 18, 2021

straight-shoota deleted the feature/integer-autocast branch December 23, 2021 13:46

HertzDevil mentioned this pull request May 17, 2022

Autocasting should be done in the semantic phase entirely #12063

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support autocasting from smaller integer to large integer, or float, for vars #9618

Support autocasting from smaller integer to large integer, or float, for vars #9618

asterite commented Jul 18, 2020

bcardiff left a comment

bcardiff Jul 22, 2020

bcardiff Jul 22, 2020

asterite Jul 22, 2020

asterite Jul 22, 2020

asterite Jul 22, 2020

bcardiff Jul 22, 2020

asterite Jul 22, 2020

asterite commented Jul 22, 2020

bcardiff commented Jul 22, 2020

asterite commented Jul 22, 2020

asterite commented Jul 22, 2020

bcardiff commented Jul 22, 2020

asterite commented Jul 22, 2020

asterite commented Jul 22, 2020

asterite commented Jul 22, 2020

Sija commented Jul 26, 2020

asterite commented Jul 26, 2020

BrucePerens commented Jul 26, 2020 via email

Sija commented Feb 3, 2021

asterite commented Feb 3, 2021

Sija commented Feb 3, 2021 •

edited

Loading

asterite commented Feb 3, 2021

BrucePerens commented Feb 3, 2021 via email

Support autocasting from smaller integer to large integer, or float, for vars #9618

Support autocasting from smaller integer to large integer, or float, for vars #9618

Conversation

asterite commented Jul 18, 2020

bcardiff left a comment

Choose a reason for hiding this comment

bcardiff Jul 22, 2020

Choose a reason for hiding this comment

bcardiff Jul 22, 2020

Choose a reason for hiding this comment

asterite Jul 22, 2020

Choose a reason for hiding this comment

asterite Jul 22, 2020

Choose a reason for hiding this comment

asterite Jul 22, 2020

Choose a reason for hiding this comment

bcardiff Jul 22, 2020

Choose a reason for hiding this comment

asterite Jul 22, 2020

Choose a reason for hiding this comment

asterite commented Jul 22, 2020

bcardiff commented Jul 22, 2020

asterite commented Jul 22, 2020

Autocasting

asterite commented Jul 22, 2020

bcardiff commented Jul 22, 2020

asterite commented Jul 22, 2020

asterite commented Jul 22, 2020

asterite commented Jul 22, 2020

Sija commented Jul 26, 2020

asterite commented Jul 26, 2020

BrucePerens commented Jul 26, 2020 via email

Sija commented Feb 3, 2021

asterite commented Feb 3, 2021

Sija commented Feb 3, 2021 • edited Loading

asterite commented Feb 3, 2021

BrucePerens commented Feb 3, 2021 via email

Sija commented Feb 3, 2021 •

edited

Loading