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

Deprecate numeric conversions that lose precision #7405

Closed
wants to merge 1 commit into from

Conversation

@smarter
Copy link
Contributor

smarter commented Nov 8, 2018

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

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Nov 8, 2018
@smarter smarter changed the title Deprecate numeric conversions that lose precisions Deprecate numeric conversions that lose precision Nov 8, 2018
@smarter smarter force-pushed the smarter:remove-bad-conversions branch from b6cd037 to efdfd6e Nov 8, 2018
@smarter smarter force-pushed the smarter:remove-bad-conversions branch from efdfd6e to 7a960f2 Nov 8, 2018
@smarter

This comment has been minimized.

Copy link
Contributor Author

smarter commented Nov 8, 2018

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

@Ichoran

This comment has been minimized.

Copy link
Contributor

Ichoran commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor Author

smarter commented Nov 8, 2018

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 smarter force-pushed the smarter:remove-bad-conversions branch from 7a960f2 to 832486c Nov 8, 2018
@smarter

This comment has been minimized.

Copy link
Contributor Author

smarter commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor

Ichoran commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor Author

smarter commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor

Ichoran commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor Author

smarter commented Nov 8, 2018

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

@Ichoran

This comment has been minimized.

Copy link
Contributor

Ichoran commented Nov 8, 2018

What happens with

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

under the deprecation as it currently stands?

@smarter

This comment has been minimized.

Copy link
Contributor Author

smarter commented Nov 8, 2018

@Ichoran

This comment has been minimized.

Copy link
Contributor

Ichoran commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor Author

smarter commented Nov 8, 2018

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

@Ichoran

This comment has been minimized.

Copy link
Contributor

Ichoran commented Nov 9, 2018

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

This comment has been minimized.

Copy link
Contributor Author

smarter commented Nov 9, 2018

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 lampepfl/dotty#5358 and lampepfl/dotty#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 Ichoran self-assigned this Jan 10, 2019
@Ichoran

This comment has been minimized.

Copy link
Contributor

Ichoran commented Jan 14, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

smarter commented Jan 15, 2019

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

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Jan 31, 2019

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

@SethTisue SethTisue modified the milestones: 2.13.0-RC1, 2.13.1 Feb 5, 2019
@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Jun 18, 2019

@smarter needs rebase

@smarter smarter force-pushed the smarter:remove-bad-conversions branch from f4612f7 to a118f89 Jun 18, 2019
@smarter

This comment has been minimized.

Copy link
Contributor Author

smarter commented Jun 18, 2019

Rebased.

@smarter smarter force-pushed the smarter:remove-bad-conversions branch from a118f89 to d4d3470 Jun 18, 2019
Int to Float, Long to Float and Long to Double are dangerous conversions
and should never be done automatically.
@smarter smarter force-pushed the smarter:remove-bad-conversions branch from d4d3470 to c40a183 Jun 19, 2019
@smarter

This comment has been minimized.

Copy link
Contributor Author

smarter commented Jun 19, 2019

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.

@szeiger szeiger modified the milestones: 2.13.1, 2.13.2 Aug 30, 2019
@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Jan 31, 2020

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

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Jan 31, 2020

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

@lrytz

This comment has been minimized.

Copy link
Member

lrytz commented Jan 31, 2020

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

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Jan 31, 2020

Yes, I would like to see this in 2.13.2.

@eed3si9n

This comment has been minimized.

Copy link
Member

eed3si9n commented Jan 31, 2020

This fixes scala/bug#10773.

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Feb 2, 2020

@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

This comment has been minimized.

Copy link
Contributor Author

smarter commented Feb 2, 2020

Can't deprecationWarning be fixed to not be eager?

@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Feb 2, 2020

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

This comment has been minimized.

Copy link
Member

lrytz commented Feb 3, 2020

Merged in #8679.

@lrytz lrytz closed this Feb 3, 2020
@SethTisue SethTisue removed this from the 2.13.2 milestone Feb 3, 2020
@SethTisue SethTisue removed the release-notes label Feb 3, 2020
@som-snytt

This comment has been minimized.

Copy link
Contributor

som-snytt commented Feb 21, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.