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

Opt-in autocast for integers and floats #11431

Merged

Conversation

beta-ziliani
Copy link
Member

@beta-ziliani beta-ziliani commented Nov 10, 2021

Fixes #9565

Closes #9618

For the moment, it's guarded behind a compiler flag, in order to give us a time to test it. The following program compiles with crystal build -Dnumber_autocast:

def foo(x : Int64)
  x
end

def bar
  1_i32
end

foo(bar)

UPDATE: This feature became opt-out in #11529

spec/compiler/codegen/automatic_cast_spec.cr Show resolved Hide resolved
spec/compiler/semantic/automatic_cast_spec.cr Outdated Show resolved Hide resolved
src/compiler/crystal/semantic/ast.cr Outdated Show resolved Hide resolved
src/compiler/crystal/semantic/ast.cr Outdated Show resolved Hide resolved
@HertzDevil
Copy link
Contributor

Some possible additional specs:

def foo(x : Int64 | UInt64)
  x
end

def bar
  1_i32
end

foo(bar) # should fail
def foo(x : Int64 | String)
  x
end

def bar
  1_i32 || ""
end

foo(bar) # should be 1_i64 on codegen

@beta-ziliani
Copy link
Member Author

@HertzDevil good examples, yet the second one fails (I don't honestly know why, since the mechanism is the same as for e.g. symbol autocast)

@asterite
Copy link
Member

When I originally did this PR, I didn't add code to support that second example. So if you used that same code, it won't work. And, for now, I wouldn't worry about that case. Autocasting when unions are involved doesn't work at all, not even in the current autocasting:

def foo(x : Int64 | String)
  puts x, typeof(x)
end

foo(1 || "a")

Gives:

In foo.cr:5:1

 5 | foo(1 || "a")
     ^--
Error: no overload matches 'foo' with type (Int32 | String)

Overloads are:
 - foo(x : Int64 | String)
Couldn't find overloads for these types:
 - foo(x : Int32)

@Sija
Copy link
Contributor

Sija commented Nov 11, 2021

Would be nice to add these examples as pending anyhow.

@straight-shoota
Copy link
Member

straight-shoota commented Nov 15, 2021

@Sija I don't think pending examples make sense here. That's just an independent, additional feature which isn't even completely well understood and finalized. We don't add pending specs for that. Instead, there should be an issue to track this as a feature request.

@beta-ziliani beta-ziliani added this to the 1.3.0 milestone Nov 17, 2021
@straight-shoota straight-shoota merged commit 5b18256 into crystal-lang:master Nov 18, 2021
@asterite
Copy link
Member

asterite commented Dec 1, 2021

@beta-ziliani @straight-shoota Could we make this opt-out? That is, enable it by default with a way to stop this behavior if things go wrong.

Why?

  • If we make it opt-in, nobody will use it. Nobody will know that this thing exists. Then you won't get any feedback about this, and you won't know whether to eventually make it the default.
  • I'm almost sure this doesn't break existing code. And if it does, you can opt-out of it.

@beta-ziliani
Copy link
Member Author

Sounds reasonable to me 👍

@straight-shoota
Copy link
Member

Sure, makes sense to enable this for evaluation on nightly releases. We can reconsider before finalizing 1.3 whether we want to release it as opt-in. Until then we have at least a couple of weeks for testing.

@elorest
Copy link
Contributor

elorest commented Dec 2, 2021

I definitely prefer Opt-Out. We've been using autocast in quite a few libraries for a year or more and now it doesn't work.

@straight-shoota
Copy link
Member

How can you be using this feature for a year? It has just been implemented and merged a couple of weeks ago.

@elorest
Copy link
Contributor

elorest commented Dec 6, 2021

How can you be using this feature for a year? It has just been implemented and merged a couple of weeks ago.

A couple years ago a feature was added which allowed for UInt8 to be passed in to params which were larger like UInt64. I have a couple lines of code which used to compile that don't now. It's nothing that can't be easily fixed though.

UPDATE:

What used to work as of a couple years ago is this.

https://carc.in/#/r/cep4

It also still works so I guess I had assumed that the feature from 2019 went further than it did. Sorry.

Either way I really like the idea of having it by default and opting out since this is the expectation people coming from C have.

@Blacksmoke16
Copy link
Member

Blacksmoke16 commented Dec 6, 2021

@elorest I think that's moreso #6074, since it's passing a number literal as an argument to the method, while this issue/PR is related to the case where you pass a variable/method return value as the argument.

So maybe the opt out check was a bit too restrictive?

@beta-ziliani
Copy link
Member Author

beta-ziliani commented Dec 6, 2021

So maybe the opt out check was a bit too restrictive?

@Blacksmoke16 what do you mean here?

@Blacksmoke16
Copy link
Member

@beta-ziliani Just that @elorest said:

I have a couple lines of code which used to compile that don't now.

So was assuming the opt-in also made that existing feature opt-in.

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.

No auto-upcast for assignment of smaller ints to larger ones.
7 participants