Skip to content

Constant fold all the number conversion methods#17446

Merged
TheElectronWill merged 2 commits into
scala:mainfrom
TheElectronWill:fix-13990
May 24, 2023
Merged

Constant fold all the number conversion methods#17446
TheElectronWill merged 2 commits into
scala:mainfrom
TheElectronWill:fix-13990

Conversation

@TheElectronWill

Copy link
Copy Markdown
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 :)

Comment thread compiler/src/dotty/tools/dotc/typer/ConstFold.scala Outdated
@som-snytt

Copy link
Copy Markdown
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
Copy Markdown
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

Comment thread tests/pos/13990.scala Outdated
@TheElectronWill

Copy link
Copy Markdown
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

sjrd commented May 10, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
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

sjrd commented May 10, 2023

Copy link
Copy Markdown
Member

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

mbovel commented May 11, 2023

Copy link
Copy Markdown
Member

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

mbovel commented May 11, 2023

Copy link
Copy Markdown
Member

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.

Comment thread tests/pos/i13990.scala Outdated
Co-authored-by: Eugene Flesselle <eugenefle@users.noreply.github.com>
Co-authored-by: Martin Kucera <github@mkucera.cz>
@TheElectronWill

Copy link
Copy Markdown
Contributor Author

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

@TheElectronWill
TheElectronWill requested a review from mbovel May 24, 2023 12:21

@mbovel mbovel left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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

TheElectronWill commented May 25, 2023

Copy link
Copy Markdown
Contributor Author

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