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

Constant fold all the number conversion methods #17446

Merged
merged 2 commits into from
May 24, 2023

Conversation

TheElectronWill
Copy link
Contributor

Co-authored-by: Eugene Flesselle eugenefle@users.noreply.github.com
Co-authored-by: Martin Kucera github@mkucera.cz

Fixes #13990. Made during the Scala Spree :)

@som-snytt
Copy link
Contributor

Wrong number of errors encountered when compiling tests/neg/serialversionuid-not-const.scala
expected: 3, actual: 2
Unfulfilled expectations:
tests/neg/serialversionuid-not-const.scala:1
Unexpected errors:

-> following the errors:
 at 3: The argument passed to @SerialVersionUID must be a constant
 at 4: The argument passed to @SerialVersionUID must be a constant

line 1 (one) should be marked OK now.

@som-snytt
Copy link
Contributor

This is nice but I guess we won't get range checking like normal constant conversion.

scala> Int.MaxValue: Byte
           ^
       error: type mismatch;
        found   : Int(2147483647)
        required: Byte

scala> Int.MaxValue.toByte
val res1: Byte = -1

tests/pos/13990.scala Outdated Show resolved Hide resolved
@TheElectronWill
Copy link
Contributor Author

This is nice but I guess we won't get range checking like normal constant conversion.

scala> Int.MaxValue: Byte
           ^
       error: type mismatch;
        found   : Int(2147483647)
        required: Byte

scala> Int.MaxValue.toByte
val res1: Byte = -1

that's right, it won't generate an error. It may be better to at least emit a warning in order to catch some mistakes, WDYT?

@sjrd
Copy link
Member

sjrd commented May 10, 2023

This might have an unintended side effect that we should protect against. Unless I'm mistaken, this allows to define constants (in the sense of Constant Expression in the spec) whose type is a Byte or Short:

object Foo {
  final val x = 5.toByte // previously typed as Int, now ConstantType(5: Byte)
  inline val x = 5.toShort // same thing
}

As much as I would like to have Byte and Short Constants, I don't think we should allow that to happen, because we cannot write such a constant type.

It would be good to ensure that inferred types widen ConstantType's for Bytes and Shorts to non-constant Ints (edit: Bytes and Shorts), as it did before.

@TheElectronWill
Copy link
Contributor Author

This might have an unintended side effect that we should protect against. Unless I'm mistaken, this allows to define constants (in the sense of Constant Expression in the spec) whose type is a Byte or Short:

object Foo {
  final val x = 5.toByte // previously typed as Int, now ConstantType(5: Byte)
  inline val x = 5.toShort // same thing
}

As much as I would like to have Byte and Short Constants, I don't think we should allow that to happen, because we cannot write such a constant type.

It would be good to ensure that inferred types widen ConstantType's for Bytes and Shorts to non-constant Ints, as it did before.

I think that Nicolas expected that to be allowed when he wrote #13990, didn't he?

I agree that it might be a bit weird because we cannot write such a constant type. On the other hand, it might also be confusing to allow 5.toByte but make it an Int.

@sjrd
Copy link
Member

sjrd commented May 10, 2023

On the other hand, it might also be confusing to allow 5.toByte but make it an Int.

Oh yes sorry. That should have been make it a non-constant Byte or Short. Clearly we don't want to turn a Byte into an Int.

@mbovel
Copy link
Member

mbovel commented May 11, 2023

As much as I would like to have Byte and Short Constants, I don't think we should allow that to happen, because we cannot write such a constant type.

It would be good to ensure that inferred types widen ConstantType's for Bytes and Shorts to non-constant Ints, as it did before.

I think that Nicolas expected that to be allowed when he wrote #13990, didn't he?

Yeah, @nicolasstucki had this in mind.

But I see how it's a problem to infer types that cannot be written explicitly.

So, if we don't want that, I guess the solution is just to remove the cases for Byte and Short?

@mbovel
Copy link
Member

mbovel commented May 11, 2023

that's right, it won't generate an error. It may be better to at least emit a warning in order to catch some mistakes, WDYT?

No, I think you have the right behavior. to* don't throw errors at runtime; they just silently wrap. So we should do the same at compile-time.

Byte the way, maybe this behavior should be described in the standard library documentation. Seems like it is similar to WebAssembly's wrap operation:

The wrap instruction, is used to convert numbers of type i64 to type i32. If the number is larger than what an i32 can hold this operation will wrap, resulting in a different number.

One can think of wrap either as reducing the value mod 2^32, or as discarding the high 32 bits to produce a value containing just the low 32 bits.

tests/pos/i13990.scala Outdated Show resolved Hide resolved
Co-authored-by: Eugene Flesselle <eugenefle@users.noreply.github.com>
Co-authored-by: Martin Kucera <github@mkucera.cz>
@TheElectronWill
Copy link
Contributor Author

I've removed the const-folding for toByte and toShort, and fixed the tests.

Copy link
Member

@mbovel mbovel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@TheElectronWill TheElectronWill merged commit 5262680 into scala:main May 24, 2023
@som-snytt
Copy link
Contributor

I don't understand the point about omitting toByte.

I expected 42.toByte to be a nicer way to write 42: Byte.

I have that expectation even if literal folding doesn't warn me that my literal doesn't fit. (Which I also don't understand, as it's an obvious gotcha and an easy lint win.)

The ticket says to allow inline val, which to me means as if the RHS were ascribed 42: Byte.

well, as you can see, I don't even know how to use Scala 3.

➜  ~ scala
Welcome to Scala 2.13.10 (OpenJDK 64-Bit Server VM, Java 20.0.1).
Type in expressions for evaluation. Or try :help.

scala> final val x = 42: Byte
val x: Byte = 42

scala>
:quit
➜  ~ sdk use scala 3.2.2

Using scala version 3.2.2 in this shell.
➜  ~ scala
Welcome to Scala 3.2.2 (20.0.1, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> final val x = 42: Byte
-- [E088] Syntax Error: ------------------------------------------------------------------------------------------------
1 |final val x = 42: Byte
  |      ^^^
  |      Expected start of definition
  |
  | longer explanation available when compiling with `-explain`

scala>

@TheElectronWill
Copy link
Contributor Author

TheElectronWill commented May 25, 2023

The ticket says to allow inline val, which to me means as if the RHS were ascribed 42: Byte.

but the type of that value is Byte, not the constant type "42 (byte)". For instance, for ints, we want inline val x: 12 = 12 with the constant type "12 (int)", not Int.

@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constant fold toInt, toLong, toDouble, toFloat, toByte and toShort
5 participants