Fix Constant tag of Expr(null: String)

[mbovel]

Fixes #23008. Started during the Scala 3 Compiler Spree of Monday, April 28th, 2025.

The Constant is an object in Dotty core that has several apply overloads, including Constant(String) and Constant(Null), that create Constant instances with the appropriate tags:

scala3/compiler/src/dotty/tools/dotc/core/Constants.scala

Line 229 in bbe2dde

def apply(x: Null): Constant = new Constant(x, NullTag)

scala3/compiler/src/dotty/tools/dotc/core/Constants.scala

Line 238 in bbe2dde

def apply(x: String): Constant = new Constant(x, StringTag)

It is called from the quotes reflection API with an argument x: String, which always dispatched to the former overload:

scala3/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala

Line 2532 in 9e7aab7

def apply(x: String): StringConstant = dotc.core.Constants.Constant(x)

When this API is called from code without explicit nulls, null can be passed as argument x. That still dispatches to the Constant(String) overload, yielding a Constant with the wrong tag.

This PR fixes the issue by forbidding null as an argument of StringConstant, but allowing it as an argument of StringToExpr and returning a proper NullConstant (a constant with tag NullTag) in that case.

[hamzaremmal]

This is a fix to what I believe is a soundness hole in explicit nulls. I'm going to try to have another example with more details about this soundness issue.

[noti0na1]

If there is unsafe data feed into explicit nulls, there is nothing we can do other than runtime check.

[He-Pin]

If this is a valid fix, then comments is needed inline

[Alexey-NM]

May be it is better to add:

object dotc.core.Constants.Constant{
  val nullConstant: Constant  = new Constant(null, NullTag)
}

def apply(x: String): StringConstant = if (x==null) {dotc.core.Constants.nullConstant} else {dotc.core.Constants.Constant(x)}

[mbovel]

I was also surprised when I discovered true/false/null constants are not re-used/interned/hash-consed. I don't know if that would have a significant impact. I think this could be discussed in a separate issue or PR.

[olhotak]

Is the intended behaviour of Expr(null: String) specified anywhere, such as in the Javadoc comments of Expr in the scala.quoted API, or anywhere else? I couldn't find it. Is one allowed to pass null here or not?

[HarrisL2]

Is the intended behaviour of Expr(null: String) specified anywhere, such as in the Javadoc comments of Expr in the scala.quoted API, or anywhere else? I couldn't find it. Is one allowed to pass null here or not?

I believe its not allowed, but not very well documented.

scala3/compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala

Line 575 in 08f2e4a

assert(const.value != null, const) // TODO this invariant isn't documented in `case class Constant`

[Alexey-NM]

I believe its not allowed

If this is the case, then Expr(null: String) should throw an exception. We spent a lot of time trying to understand why exprString.show was throwing an error. Actually we didn't explicitly write Expr(null: String) instead, we performed a conversion from a Java POJO. This was not immediately obvious, and we were quite frustrated to discover that the root cause was related to Expr, which is defined far away and comes with implicit imports.

If this is indeed how it works, it represents a rather unfriendly design when interacting with Java objects.

[Alexey-NM]

IMHO: The example of a good design is Option

  /** An Option factory which creates Some(x) if the argument is not null,
   *  and None if it is null.
   *
   *  @param  x the value
   *  @return   Some(value) if value != null, None if value == null
   */
  def apply[A](x: A): Option[A] = if (x == null) None else Some(x)

[mbovel]

I believe its not allowed, but not very well documented.

The point here is that a Dotty Constant with tag StringTag cannot have the value null:

scala3/compiler/src/dotty/tools/backend/jvm/BCodeBodyBuilder.scala

Lines 574 to 575 in 08f2e4a

	case StringTag =>
	assert(const.value != null, const) // TODO this invariant isn't documented in `case class Constant`

However, Constant(null, NullTag) is valid. Therefore, I think the questions to accept null: Null or null: String as arguments to Expr, StringToExpr, and StringConstant in the quotes API should be treated independently of this assertion.

Is the intended behaviour of Expr(null: String) specified anywhere, such as in the Javadoc comments of Expr in the scala.quoted API, or anywhere else? I couldn't find it. Is one allowed to pass null here or not?

I also could not find any documentation.

Thus, it seems up to us to decide whether Expr(null: String) should throw an exception or instead produce a well-formed null constant (as this PR proposes).

Accepting null seems reasonable to me: I only see advantages so far (notably compatibility with code without explicit nulls). Are there any drawbacks?

[olhotak]

If we decide to allow null, can we document this by making the signature of StringConstant.apply take a String|Null instead of String?

[mbovel]

If we decide to allow null, can we document this by making the signature of StringConstant.apply take a String|Null instead of String?

Indeed, that would also look like nicer to us! We discussed it on Monday during the Spree and concluded that would be a backward-incompatible change, but I am actually not sure.

[mbovel]

By the way, while trying to change the parameter type to String | Null, I noticed Constant(x: String | Null) in compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala seems to resolve to Constant(String) and not to Constant(Any), as I would expect with explicit nulls enabled. Any idea why?

[noti0na1]

By the way, while trying to change the parameter type to String | Null, I noticed Constant(x: String | Null) in compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala seems to resolve to Constant(String) and not to Constant(Any), as I would expect with explicit nulls enabled. Any idea why?

I guess the file calling the quotes api is not in explicit nulls, so String | Null will still be a subtype of String.

[mbovel]

Something else I just noticed: the method returns a StringConstant, which is defined as a Constant with tag StringTag:

scala3/compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala

Lines 2526 to 2529 in bbe2dde

	object StringConstantTypeTest extends TypeTest[Constant, StringConstant]:
	def unapply(x: Constant): Option[StringConstant & x.type] =
	if x.tag == dotc.core.Constants.StringTag then Some(x.asInstanceOf[StringConstant & x.type]) else None
	end StringConstantTypeTest

So we shouldn't return a Constant with tag NullTag under this return type.

We could change the return type to StringConstant | NullConstant, but that starts to feel convoluted.

Maybe it's better after all to just require a non-null argument?

[mbovel]

I guess the file calling the quotes api is not in explicit nulls, so String | Null will still be a subtype of String.

But isn't the overload statically resolved, in the file with explicit nulls? In the situation I describe, the call Constant(x: String | Null) is in compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala, not in a foreign file.

[noti0na1]

Oh, I see what you meam. No, explicit nulls is disabled in QuotesImpl by importing unsafeNulls.

[mbovel]

Oh, I see what you meam. No, explicit nulls is disabled in QuotesImpl by importing unsafeNulls.

Ah, that explains it! Thanks, I missed this import.

[noti0na1]

Maybe it's better after all to just require a non-null argument?

I'm ok with this, given a null string is always not allowed, it's better to throw the exception ealier.

[Alexey-NM]

Maybe it's better after all to just require a non-null argument?

I'm ok with this, given a null string is always not allowed, it's better to throw the exception ealier.

We can solve the issue in StringToExpr
So this is not the reason to throw the exception

[mbovel]

We can solve the issue in StringToExpr
So this is not the reason to throw the exception

What do you suggest for StringToExpr? To handle null there?

In any case, we still need to do something for StringConstant itself as it is also part of the public API, don't we?

[Alexey-NM]

In any case, we still need to do something for StringConstant itself as it is also part of the public API, don't we?

IIUC: Our primary issue is that StringToExpr return broken expression without any exception:

 import quotes.reflect.*
 val str: String = null
 val exprString = Expr(str)

It seems we can fix it just to rewrite StringToExpr

  given StringToExpr[T <: String]: ToExpr[T] with {
    def apply(x: T)(using Quotes) =
      import quotes.reflect.*
      Literal(if (x==null) {nullListeral} else {StringConstant(x)}).asExpr.asInstanceOf[Expr[T]]
  }

it seems the issue can be resolved without modifying the declaration of StringConstant.

[Alexey-NM]

In any case, we still need to do something for StringConstant itself as it is also part of the public API, don't we?

IMHO: Yes, StringConstant must not return broken result silently.

[mbovel]

I have just pushed a commit that allows null in StringToExpr, but not in StringConstant, following @noti0na1 and @Alexey-NM's comments.

[mbovel]

But now if we allow null for StringToExpr, maybe we should allow it for all other ToExpr instances as well, which might be overkill 😕

[Alexey-NM]

But now if we allow null for StringToExpr, maybe we should allow it for all other ToExpr instances as well, which might be overkill 😕

IMHO: There are no other java primitives in the ToExpr which can be null.
May be:

• BigIntToExpr
• BigDecimalToExpr

But it is scala types and may be it should not be null in any case.

[Alexey-NM]

But now if we allow null for StringToExpr, maybe we should allow it for all other ToExpr instances as well, which might be overkill 😕

Throwing exception in dotc.core.Constants.Constant(x) for broken cases may be usefull.
It is actual for String and Type.

[mbovel]

There are no other java primitives in the ToExpr which can be null.

Yes, but what about non-primitive types? We also get runtime errors in these cases if we pass null, although these are not related to Constant, for example:

  def nullTestImpl()(using Quotes): Expr[Unit] =
    Expr(null: Option[Int]) // MatchError: null
    Expr(null: (Int, Int)) // NullPointerException: Cannot invoke "scala.Tuple2._1()" because "tup$2" is null
    Expr(null: Array[Int]) // NullPointerException: Cannot read the array length because "array" is null

[Alexey-NM]

Yes, but what about non-primitive types? We also get runtime errors in these cases, for example:

IMHO:

• Expr(null: Option[Int]) // MatchError: null
  Exception is correct because such writing should not occur.

• Expr(null: Array[Int])
  Since It is a Java type, it would be useful to allow convertion for better interoperability

• Expr(null: (Int, Int))
  I just do not know, it seems such writing should not occur.