-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow multiple spreads in function arguments #23855
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
base: main
Are you sure you want to change the base?
Conversation
44c0133
to
fd598a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments on the implementation strategy. Overall it looks like the right design, but some details can be improved IMO.
then addArg(typedArg(arg, formal), formal) | ||
else addArg(typedArg(arg, elemFormal), elemFormal) | ||
else | ||
val typedArgs = harmonic(harmonizeArgs, elemFormal): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, there is no equivalent code in the code path with multiSpreads
. I understand it may be tricky, but it's at least worth calling out at the spec/SIP level. IIUC It means we get:
def foo[A](xs: A*): List[A] = xs.toList
foo(5, 5.6) // List[Double]
val doubles = List(5.6)
foo(5, doubles*) // List[AnyVal]
Basically having any spread in a vararg argument disables harmonization entirely.
@sjrd I got harmonization working for spreads and also implemented the other review suggestions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I'm not sure what happens if we provide an Array[S]
where S <: T
. For example in Scala 3.7.3 we can do:
scala> def foo(x: CharSequence*): String = x.mkString
def foo(x: CharSequence*): String
scala> val strings: Array[String] = Array("foo", "bar")
val strings: Array[String] = Array(foo, bar)
scala> foo(strings*)
val res0: String = foobar
What is then the desugaring of
scala> foo(("oof": CharSequence), strings*)
It seems we can't call addArray(strings)
, because it requires an Array[CharSequence]
and strings
is an Array[String]
.
Good question! In fact it runs fine, but fails -Ycheck. To fix this, I changed the generated code to add a cast if needed. The example foo(("oof": CharSequence), strings*) now compiles to foo(
scala.runtime.VarArgsBuilder.ofRef[CharSequence](1 + strings.length)
.add("oof":CharSequence)
.addArray(strings.$asInstanceOf[Array[CharSequence]]).result()*
) |
Ah yes, that works. |
@sjrd Do you have more comments, or can we merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tests/run/i11419.scala
Outdated
object Test { | ||
def main(args: Array[String]): Unit = { | ||
val arr: Array[String] = Array("foo") | ||
val lst = List("x", arr*) // error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stale // error
comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty file added by accident?
We need to access them twice because we first need to take their length, then append them to the buffer. If a spread might have side effects, lift all side-effecting arguments out in the order of occurrence.
2b8afc7
to
3af6d6f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No further comments. I haven't thoroughly reviewed all of the implementation, but the compilation scheme looks good to me.
Implements SIP 70. Currently, only
Seq
s andArray
s can be unpacked whereas the SIP also specifiesOption
unpacking.