Skip to content

Try to refactor convertTo method for improved type handling with union types #24585

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+34 −40
Draft

Try to refactor convertTo method for improved type handling with union types #24585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 29 additions & 40 deletions compiler/src/dotty/tools/dotc/core/Constants.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,46 +147,35 @@ object Constants {
case _ => throw new Error("value " + value + " is not a Double")
}

/** Convert constant value to conform to given type.
*/
def convertTo(pt: Type)(using Context): Constant | Null = {
def classBound(pt: Type): Type = pt.dealias.stripTypeVar.stripNull() match {
case tref: TypeRef if !tref.symbol.isClass && tref.info.exists =>
classBound(tref.info.bounds.lo)
case param: TypeParamRef =>
ctx.typerState.constraint.entry(param) match {
case TypeBounds(lo, hi) =>
if (hi.classSymbol.isPrimitiveValueClass) hi //constrain further with high bound
else classBound(lo)
case NoType => classBound(param.binder.paramInfos(param.paramNum).lo)
case inst => classBound(inst)
}
case pt => pt
}
pt match
case ConstantType(value) if value == this => this
case _: SingletonType => null
case _ =>
val target = classBound(pt).typeSymbol
if (target == tpe.typeSymbol)
this
else if ((target == defn.ByteClass) && isByteRange)
Constant(byteValue)
else if (target == defn.ShortClass && isShortRange)
Constant(shortValue)
else if (target == defn.CharClass && isCharRange)
Constant(charValue)
else if (target == defn.IntClass && isIntRange)
Constant(intValue)
else if (target == defn.LongClass && isLongRange)
Constant(longValue)
else if (target == defn.FloatClass && isFloatRange)
Constant(floatValue)
else if (target == defn.DoubleClass && isNumeric)
Constant(doubleValue)
else
null
}
/** Convert constant value to conform to given type. */
def convertTo(pt: Type)(using Context): Constant | Null = pt.dealias.stripTypeVar match
case ConstantType(value) if value == this => this
case _: SingletonType => null
case tref: TypeRef if !tref.symbol.isClass && tref.info.exists =>
convertTo(tref.info.bounds.lo)
case param: TypeParamRef =>
ctx.typerState.constraint.entry(param) match
case TypeBounds(lo, hi) =>
val hiResult = convertTo(hi)
if hiResult != null then hiResult
else convertTo(lo)
case NoType => convertTo(param.binder.paramInfos(param.paramNum).lo)
case inst => convertTo(inst)
case pt: OrType =>
val leftResult = convertTo(pt.tp1)
if leftResult != null then leftResult
else convertTo(pt.tp2)
Comment on lines +164 to +167
Copy link
Member

@hamzaremmal hamzaremmal Nov 28, 2025

Choose a reason for hiding this comment

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

This is the part where I would say I don't agree with and why I say it is a can of worms.
What if we have val x: Byte | Short = 0, maybe this should be a Short instead of a Byte (take the largest type). This is something that needs to be decided and what approach we will take.

Copy link
Member Author

@noti0na1 noti0na1 Nov 28, 2025

Choose a reason for hiding this comment

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

Do you have a concrete example why this matters? I think if there is any ambiguity inside the expected union type, it's user's responsibility to specify a type first.

Copy link
Member

@hamzaremmal hamzaremmal Nov 28, 2025

Choose a reason for hiding this comment

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

You could say why does it matter what is the type of the constant of the rhs.
It matters because of macros first. Second, we are thinking about how to encode Byte, Short,... literals in the language. We need to then know that literal, what would be its exact type.
Third, transparent inline def foo: Byte | Short = 0, since this is a transparent inline, we cannot just ignore the type information to see that it was a Byte | Short, we need to know what kind of 0 and we can clearly not say it was an Int by stripping the type information completely.

Copy link
Member

@hamzaremmal hamzaremmal Nov 28, 2025

Choose a reason for hiding this comment

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

We were writing at the same time 😅

Copy link
Member

@hamzaremmal hamzaremmal Nov 28, 2025

Choose a reason for hiding this comment

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

it's user's responsibility to specify a type first.

Yes, maybe the user should not write this and in my opinion they shouldn't but Scala 3 has a specification and even if this is a very niche edge case that no one should write, it is still important to specify it and understand all the possible designs we could have chosen and chose one wisely (because once we do, there is no changing it).

Copy link
Member

@hamzaremmal hamzaremmal Nov 28, 2025

Choose a reason for hiding this comment

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

I just had a chat with @odersky about this and he thinks that this should not be fixed at all.
Even val x: Byte | String = 0 should fail.

Copy link
Contributor

@odersky odersky Nov 28, 2025

Choose a reason for hiding this comment

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

I think this would need at least a SIP. Feature interaction is a problem here. What if there was an implicit conversion from Int to Byte | String? Then that would have been picked before but now it would be bypassed.

Copy link
Member Author

@noti0na1 noti0na1 Nov 28, 2025

Choose a reason for hiding this comment

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

How about: it should fail if there are different ways to convert according to the union type; success if there is only one result.

val a: Byte | String | Byte = 0 // ok
val b: Byte | Int = 0 // fails

Copy link
Member

@hamzaremmal hamzaremmal Nov 28, 2025

Choose a reason for hiding this comment

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

I think this would need at least a SIP.

I completely agree on this.

Feature interaction is a problem here. What if there was an implicit conversion from Int to Byte | String? Then that would have been picked before but now it would be bypassed.

That's also something to consider if we were to pursue these improvements.

Copy link
Member

@hamzaremmal hamzaremmal Nov 28, 2025

Choose a reason for hiding this comment

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

How about: it should fail if there are different ways to convert according to the union type; success if there is only one result

That's also another way of handling the situation. My point is (and was this morning too), this is not as simple as opening a PR with one of these solutions. There is many factors to take into account and this will always result into a change of spec (so a SIP as Martin mentioned too). The T | Null was an obvious one that needed to be fixed and we did, so pressure is off.

case pt =>
val target = pt.typeSymbol
if target == tpe.typeSymbol then this
else if (target == defn.ByteClass) && isByteRange then Constant(byteValue)
else if (target == defn.ShortClass) && isShortRange then Constant(shortValue)
else if (target == defn.CharClass) && isCharRange then Constant(charValue)
else if (target == defn.IntClass) && isIntRange then Constant(intValue)
else if (target == defn.LongClass) && isLongRange then Constant(longValue)
else if (target == defn.FloatClass) && isFloatRange then Constant(floatValue)
else if (target == defn.DoubleClass) && isNumeric then Constant(doubleValue)
else null

def stringValue: String = value.toString

Expand Down
5 changes: 5 additions & 0 deletions tests/pos/i24571.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,16 @@ val n3: Int = 2
val n4: Int | Null = 2222
val n5: Int | Byte = 2
val n6: Byte | Int = 10000
val n7: 1 | Null = 1
val n8: Byte | String = 2

val x: Option[Byte] = Option(2)
val x2: Option[Byte] = Option[Byte](2)
val x3: Option[Int] = Option(2)
val x4: Option[Null] = Option(null)
val x5: Option[Byte | Null] = Option(2)
val x6: Option[1 | Null] = Option(1)
val x7: Option[Byte | String] = Option(2)

trait MyOption[+T]

Expand All @@ -22,3 +26,4 @@ val test2: MyOption[Byte] = MyOption.applyOld(2)
val test3: MyOption[Int] = MyOption(2)
val test4: MyOption[Null] = MyOption(null)
val test5: MyOption[Byte | Null] = MyOption(2)
val test6: MyOption[Byte | String] = MyOption(2)
Loading