Deprecate numeric conversions that lose precision

[smarter]

Int to Float, Long to Float and Long to Double are dangerous conversions
and should never be done automatically.

[smarter]

Assuming this gets in, what do people think of deprecating Char to Float/Double too ? These strike me as rather weird to have.

[Ichoran]

I don't think deprecation is the right thing to do. Instead, it should be a warning (that can independently be turned off if you know what you're doing). I agree that it's dangerous.

I'm fine with Char to Float. It doesn't lose any information, and Char can be used as a u16 type, kinda. I'm also fine with no Char to Float. It's weird. Being explicit about the cast would probably help everyone understand what is going on (if not why).

[smarter]

I don't think deprecation is the right thing to do

Why not ? I'd like to get rid of this eventually. If we agree it's bad, then there's no point in keeping it.

[smarter]

There seems to be an underlying bug in error reporting exposed by this PR:

scala> val x = 0
x: Int = 0

scala> x min 0
       ^
       warning: Automatic conversion from Int to Float is deprecated (since 2.13.0) because it loses precision. Write `.toFloat` instead.
res1: Int = 0

Thankfully, the generated code does not contain a conversion to float, what seems to happen is that implicit search at some point tries the floatWrapper conversion, then backs out of it and uses intWrapper. But then the warning should not be displayed since it's part of a discarded implicit search result. @retronym Any idea what's going on here ?

[Ichoran]

I don't agree that it's bad, only that it's bad if you're surprised by it. Right now, the default is to be surprised. That's not how we should leave things.

But when you're working with Doubles, and everything is a Double that you care about, the cognitive overhead of having to switch your Longs but not your Ints (coming in from somewhere else) is grating. You might lose precision, but you won't be far wrong.

I like the Rust-style convert-absolutely-everything-explicitly style of dealing with primitives. And I like the Scala style of convert-everything-that-fits. Either way, there's an underlying simplicity--not too many rules to remember. The problem is that the notion of "fits" is different in different contexts. This favors the precise-but-more-fiddly bit depth "fits" over the less-precise-but-easier-to-remember bigness "fits". If you actually mean bigness-fits, I think you should be able to use it, since there's no real principled way to say which one is more important.

Alternatively, we could try to make the only operation widening within the same category. In that view, only these transitions are allowed:

Byte -> Short -> Int -> Long
Float -> Double

Everything else is deprecated. This has the advantage of being especially easy to remember and protecting people from other issues like a / b not working the same way if you have integers as if you have floating point values. That is a much more serious and common problem than losing a few bits of precision when you stuff an Int into a Float.

I actually think that's what I'd prefer. But it's a big change, so we should be gentle about it if we do it.

[smarter]

You might lose precision, but you won't be far wrong.

I strongly disagree here. Losing precision means losing information. If you were planning to use Float all along that's probably something you factored in, but here the real danger is that you may be thinking that you're only dealing with Ints, and accidentally calling a method that takes a Float instead without realizing it, I'm not aware of any other statically-typed language that would allow you to do that given how dangerous it is.

having to switch your Longs but not your Ints (coming in from somewhere else) is grating.

I somehow really doubt this is a problem, if you design your API well you shouldn't have conversions flying around everywhere. Even if you need all these conversions and don't want to be explicit about it, nothing prevents you from defining your own implicit conversion from Int to Float, but having it by default makes it a mental tax on every user of the language (assuming they're even aware of it).

[Ichoran]

the real danger is that you may be thinking that you're only dealing with Ints, and accidentally calling a method that takes a Float instead without realizing it

And this doesn't solve that problem, because it still does that with Double and chances are fair that you won't get the answer you hoped because of the differences between Double and Int.

If you want to avoid that problem, you have to enact my suggestion to keep widening within similar-style primitives (signed integers, floating-point).

[smarter]

I would be open to also deprecating Int => Double, though I think that has a larger cost/benefit ratio.

[Ichoran]

What happens with

val weight = 5.3f
val count = 15
val total = count * weight

under the deprecation as it currently stands?

[smarter]

That one is fine, * is overloaded: https://github.com/scala/scala/blob/2.13.x/src/library/scala/Int.scala#L410

[Ichoran]

Okay, so under your proposal

val i = 5
val j = 5f
def sum(a: Float, b: Float) = a + b
val ans = sum(i, j)

will not compile, but

val i = 5
val j = 5f
val ans = i + j

will compile? This seems kind of arbitrary. If silent conversion when using functions is bad, isn't silent conversion when using the very most common operations on numbers also bad?

[smarter]

That one doesn't shock me too much because arithmetic operations should already be considered dangerous since they can overflow/underflow.

[Ichoran]

So there is some essential difference that you see between using +, -, *, / and a method with arguments, which means that widening with the mathematical operators is okay, but with min is not?

I'm having trouble pinning down what that would be. For instance:

val x = 1
val y = 2f

x == y               // All good, it's universal equality (and is correct)
(x compare y) == 0   // Nope, wrong types
x - y == 0           // Sure thing (but might be spuriously wrong)

Also, I don't think the interaction with AnyVal is all that great:

Array(1, 2f)  // Array[Float] because numeric constant 1 coerced to float
Array(1, y)   // Same
Array(x, y)   // Array[AnyVal] after the change?
Array(x, 2f)  // Array[AnyVal] after the change?

This kind of thing works for Rust because they're brutally consistent. It's quite a pain tbh, but there are very few surprises.

let v = vec![1, 2f32];

error[E0308]: mismatched types
 --> src/main.rs:2:21
  |
2 |     let v = vec![1, 2f32];
  |                     ^^^^ expected integral variable, found f32
  |
  = note: expected type `{integer}`
             found type `f32`
]

Anyway, I like the idea in principle, but in practice I'm not sure this is more a case of a helpful improvement than adding weird rules that have to be remembered. If we're going to be more strict, I think we have to make other changes to restore a kind of internal consistency that makes it all easier to remember.

Right now, you just remember, "If it needs to be a type that can hold a bigger value, it's going to change to it."

[smarter]

x == y // All good, it's universal equality (and is correct)

Not sure what you mean by correct but I would argue it's broken:

scala> (Int.MaxValue - 1) == Int.MaxValue.toFloat
res0: Boolean = true

This should be solvable by using Dotty's multiversal equality to disallow this.

Also, I don't think the interaction with AnyVal is all that great:

So, weak conformance is a different but related issue, there's a lot opinions on this, but see scala/scala3#5358 and scala/scala3#5371 for the latest attempt at making sense of this.

Right now, you just remember, "If it needs to be a type that can hold a bigger value, it's going to change to it."

I would argue this is exactly what this PR achieves. Float is not bigger than Int, they share a subset of common values. I don't think it's such a burden to have to remember this (it's something you have to learn before attempting to write numeric code or disasters will happen anyway).

[Ichoran]

@smarter - I guess we can try merging this to see how painful it is and revert it if necessary. It's an improvement in correctness even if we could go farther. I still don't think it's a conceptual improvement, but at least making the compiler doing of the thinking is a move in the right direction.

[smarter]

@Ichoran That would be great, but I'm still stuck because of what appears to be a compiler bug: #7405 (comment)

[SethTisue]

@som-snytt any insight into the compiler bug? it would be nice if we could get this into RC1

[SethTisue]

@smarter needs rebase

[smarter]

Rebased.

[smarter]

The weird behavior I mentioned in #7405 (comment) is still present, so this is still waiting for a compiler expert to have a look at what's going on.

[lrytz]

@SethTisue is this realistic for 2.13.2? If so I'll take a look at the error reporting issue.

[som-snytt]

@lrytz I'm taking a look at why deprecationWarning is stickier than regular warning.

[lrytz]

Thanks! Make sure to rebase first, there might have been some change in this area.

[SethTisue]

Yes, I would like to see this in 2.13.2.

[eed3si9n]

This fixes scala/bug#10773.

[dwijnand]

@smarter see #8679, @som-snytt's sussed out that you need to use context.warning over context.deprecationWarning apparently. I'm not sure how you guys want to consolidate the PRs.

[smarter]

Can't deprecationWarning be fixed to not be eager?

[som-snytt]

The quick fix of if !silent then deprecationWarning suppresses too much. Usually deprecation warning happens in refchecks, post-typer, so it only sees resulting trees. I considered but didn't pursue an attachment to the toFoo selection which could be refchecked later.

[lrytz]

Merged in #8679.

[som-snytt]

The follow-up includes that idea of sticking the deprecation warning in an attachment, so it's only triggered if seen in RefChecks https://github.com/scala/scala/pull/8730/files