SIP-61 - Unroll Default Arguments for Binary Compatibility

[lihaoyi]

No description provided.

[odersky]

It would be good to add an explanation of what happens when there are multiple extensions of a method in different versions. Say in version 2 I write

def foo(s: String, n: Int = 1, @unroll b: Boolean = true, l: Long = 0)

and code like foo("", 1, false, -1L) compiles against that. Then in version 3 I would like to add another parameter with default to foo like this:

def foo(s: String, n: Int = 1, @unroll b: Boolean = true, l: Long = 0, f: Float = 0.0f)

How do I make the previous call foo("", 1, false, -1L) work? I assume I'll need another @unroll. What would the generated code be in this case?

[lihaoyi]

As currently written, @unroll generates all forwarders for parameters to the right of the annotation, and @unrollOnly generates a single forwarder for only that parameter.

We could also flip it around, and call them @unrollAll and @unroll, but the semantics would be the same. Just a question of naming

[odersky]

So, assuming unroll, would I write

def foo(s: String, n: Int = 1, @unroll b: Boolean = true, l: Long = 0, @unroll f: Float = 0.0f)

? And what would that generate?

[bjornregnell]

I think a combination of something similar to @unrollAll + @unroll is perhaps most intuitive and with a lesser risk for misinterpretation. Or perhaps the latter should be called @unrollSingle to be explicit - I mean it is not easy to know what just @unroll means...

[odersky]

Can we clarify what would happen under @unroll before discussing alternatives?

[lihaoyi]

As the proposal is currently written, it's

// Invalid, multiple `@unroll`s not allowed
def foo(s: String, n: Int = 1, @unroll b: Boolean = true, l: Long = 0, @unroll f: Float = 0.0f)

// Single `@unroll` generates forwarders for every parameter to the right
def foo(s: String, n: Int = 1, @unroll b: Boolean = true, l: Long = 0, f: Float = 0.0f)
// Generated
def foo(s: String, n: Int = 1, b: Boolean = true, l: Long = 0)
def foo(s: String, n: Int = 1, b: Boolean = true)
def foo(s: String, n: Int = 1)

// `@unrollOnly`s generates forwarders only for annotated parameters
def foo(s: String, n: Int = 1, @unrollOnly b: Boolean = true, l: Long = 0, @unrollOnly f: Float = 0.0f)
// Generated
def foo(s: String, n: Int = 1, b: Boolean = true, l: Long = 0)
def foo(s: String, n: Int = 1)

We could swap out @unroll/@unrollOnly with @unrollAll/@unroll or @unrollAll/@unrollOnly or @unroll.all/@unroll.only if people think that's more intuitive. The naming is pretty arbitrary and I don't want to spend too much time discussing it up front

[bjornregnell]

Sorry for entering "naming mode" too early :)
(I just wanted to give my initial feedback/thoughts...)

[odersky]

I would add an implementation with macro annotations to that list. Macro annotations are about to land as a SIP. We should evaluate whether and how they could implement this behavior. If they can do it, we should go down that road, I think. Then the discussion would be where the new macro should live. I would argue that it should go into the standard library in that case.

[lihaoyi]

Yes that's definitely worth a consideration. I will need to wait for the macro annotations proposal to appear before I can really say, but if we can implement this in a macro annotation rather than hardcoded in the compiler that would definitely be preferable.

[odersky]

We can propose it as a test case for the implementers of macro annotations /cc @nicolasstucki @hamzaremmal.

[lihaoyi]

I do think we do not need to block on macro annotations before reviewing this proposal.

Whether @unroll ends up being baked into the compiler, implemented as a compiler plugin (as it is today), or implemented using macro annotations, the feature is still the same. It's just a matter of what line you need in your build file to enable it.

e.g., we could decide that we want @unroll as a feature, leave it a compiler plugin while macro annotations are figured out (possibly under the github.com/scala org?), and eventually fold it into the standard library as a macro annotation when those are ready. The Scala 2 implementation will need to be done differently regardless, either baked-in or remaining as a compiler plugin.

[odersky]

Whether @unroll ends up being baked into the compiler, implemented as a compiler plugin (as it is today), or implemented using macro annotations, the feature is still the same. It's just a matter of what line you need in your build file to enable it.

It depends. First, if this can indeed be a macro annotation it's debatable whether this needs to be a SIP at all. It probably need not be, but it could. So in that sense nothing changes.

But it could also be that we need slight changes to make this work as a macro annotation. Such as an @unroll on the method as in your plugin instead of the parameter. Then it's a question whether we want to go ahead with the macro-unfriendly code anyway, and thereby complicate the language and compiler somewhat, or make concessions to be macro friendly.

[lihaoyi]

Got it.

alAn earlier version of the prototype had an @unroll("x") annotation on the method, where the "x" string parameter specified the parameter name from which to begin unrolling.

We could also do two annotations: @unroll on the method, and then @unroll2 or something on the starting parameter.

I don't have a strong opinion between these options. The current API was chosen as the most minimal while being able to provide the flexibility necessary, but the others are fine too IMO (if marginally more verbose)

[odersky]

Backwards compatible default parameters are definitely a pain point and it would be good if we could address this.

[lihaoyi]

I'd like to highlight this section as worth discussing.

I would like to be able to support non-final methods, abstract-methods-implemented-by-final-methods, and abstract-methods-implemented-by-non-final-methods. But I haven't managed to come up with an implementation strategy that I can convince myself is correct.

Right now, I'm reasonably confident that:

• @unrolling final methods works
• @unrolling override-able concrete methods cannot be done safely

There's one case I'm not sure about

• @unrolling abstract methods that are only implemented by final methods (i.e. implemented with no subsequent overrides)

If we could make this case work, it would open up the use of @unroll in traits and abstract classes, which would be a big win over the status quo of only allowing final concrete methods

[lihaoyi]

This is actually a bit simplified. We do not inline the constants true and 0, but instead get them from the this.foo$default$1 and this.foo$default$2 methods. This means that if someone overrides the default with a different, the overriden value is used. But for the vast majority of scenarios that does not make a difference to the user

[odersky]

Understood. But why do we need two generated foo functions? I would have thought if we add a new b parameter, we put the unroll on the b and get:

def foo(s: String, n: Int) = foo(s, n, true, 0)

Isn't that enough? Why do we need the three parameter foo?

[lihaoyi]

The assumption was tha new default parameters were added one at a time, so two/three/four parameter versions of foo exist and were published, and so we need the corresponding forwarders for code compiled against each version

The expected use case is you add @unroll once, then can add subsequent default parameters one at a time after in subsequent versions of your artifact, and @unroll will generate a forwarder for each and every one of them.

If that assumption is not true, then @unrollOnly gives more fine grained support on what forwarders need to be generated. If you are add default parameters more-than-one at a time per version of your artifact, and thus do not need forwarders for every single default parameter, this can save you on some unnecessary forwarders. The section on @unrollOnly touches on this

[odersky]

I still don't get it. Can you walk me through step by step? The way I see this is:

Version 1 has

def foo(s: String, n: Int = 0)

I threw in a default, just to cover that case.

Version 2 has:

def foo(s: String, n: Int = 0, @unroll b: Boolean = true, l: Long = 0)
def foo(s: String, n: Int) = foo(s, n, true, 0) // auto-generated

The default parameter on the generated version is gone. But that's OK because we have the
same default accessors, so old code calling foo("hello") expanded to foo("hello", foo$default$2) and that also works fine against the new auto-generated version.

Version 3 has:

def foo(s: String, n: Int = 0, @unroll b: Boolean = true, l: Long = 0, @unroll f: Float = 0.0f), c: Char = 'a')
def foo(s: String, n: Int, b: Boolean, l: Long) = foo(s, n, b, l, 0.0f, 'a')  // auto-generated
def foo(s: String, n: Int) = foo(s, n, true, 0, 0.0f, 'a') // auto-generated

I must be missing something, since there's always exactly one function generated, but I don't see what it is.
With that scheme, what is a scenario where we break backwards binary compatibility?

[lihaoyi]

What you described is exactly the (as-currently-written) semantics for @unrollOnly. You are not missing anything, it's correct. With 3 versions, adding two default arguments in versions 2 and 3, only 2 generated forwarders are necessary to maintain binary compatibility.

I think where your understanding differs from the current proposal, is that the current proposal calls what you described @unrollOnly rather than @unroll. In the current proposal, where @unroll is shorthand for "add @unrollOnly to the parameter annotated and every parameter to the right".

The naming should definitely be changed if it's unintuitive. If both you and @bjornregnell both find the current naming confusing, that's probably the case haha

[odersky]

Ah, but why would you use unroll then over unrollOnly? If you add multiple parameters in one version, there's no need to add a new forwarder for each prefix. I think I would prefer just one annotation. Call it unroll with the semantics of unrollOnly. Then if you want multiple forwarders, just put multiple @unrolls on your parameters. That's not a big burden, and would be less confusing.

Or, yes, maybe use @unroll and @unrollAll.

[lihaoyi]

I have updated the document to use @unroll/@unrollAll

I have not updated the prototype implementation, but it should be easy to do so when the time comes

[odersky]

Or, if we go with an annotation on the method, it could be

@unrollFrom("n") 
def foo(s: String, n: Int = 0, b: Boolean = true)

vs

@unrollAt("n") 
def foo(s: String, n: Int = 0, b: Boolean = true)

(Now it's me who caught the naming bug)

[lihaoyi]

Yes, that works, but I would argue that having a secondary annotation on the parameter itself is much better than duplicating the parameter name above the method def.

A secondary annotation on the parameter is much more clear which parameter it applies to, avoids the need for duplicating the parameter name, and is much easier to line up when the number of parameters annotated grows.

When there's one @unroll("n") annotation it doesn't really matter, but as the library evolves and we end up with 5-10 @unroll("n") parameters, having the unrolled paramter names duplicated in an annotation on top of the method is much harder to read than having them co-located:

@unrollAt("n") 
@unrollAt("b") 
@unrollAt("l") 
@unrollAt("f") 
def foo(s: String, 
        n: Int = 0, 
        b: Boolean = true,
        l: Long = 0, 
        f: Float = 0.0f))

v.s.

@unroll
def foo(s: String, 
        n: Int = 0, 
        @unroll2 b: Boolean = true,
        @unroll2 l: Long = 0, 
        @unroll2 f: Float = 0.0f)

(naming of unroll2 is arbitrary)

[lrytz]

Visible synthetic overloads would affect the user experience in IDEs, scaladocs, etc.

[odersky]

If we use annotations, then forwarders should be hidden. That's the general rule for all code generated from macro annotations. And even if this ends up not being implemented with a macro annotation, it would be confusing that the rules would change for this specific case.

[lrytz]

👍

Are macro annotation generated methods also hidden in separate compilation?

[odersky]

They are.

[lihaoyi]

CC @eed3si9n @alexarchambault @julienrf, who have worked on similar proposals/libraries/compiler-plugins in the past

[eed3si9n]

Could you link to the current incarnation of sbt-datatype, which is Contraband instead please? Contraband primarily uses GraphQL Schema as the schema language with a few extensions like using @since("1.0.0") to denote when a field was added, though we do support the JSON-style schema for internal usage.

In addition to being binary-JAR compatible, Contraband supports generation of serialization codec, which is an important aspect similar to other schemas like Protobuf and Avro. Ideally, we should support cross talk between two services with all combinations of v1 -> v2, v2 -> v1, etc.

[lihaoyi]

Thanks! I've updated the link to contraband.

[alexarchambault]

That looks great, has a larger scope than data-class, and incidentally, also has a Scala 3 implementation (unlike data-class). I hope to be able to find time to give it a try on the coursier code base, to add Scala 3 support to it.

[lihaoyi]

@alexarchambault please give it a try! Do note that the current published compiler plugin uses an older version of the proposal. You can take a look at the linked example PRs to see how to use it

[sideeffffect]

This proposal revolves around explicitly annotating items with @unroll.
Would a thinkable alternative be that the compiler unrolls all default parameters just by itself, by default?

[sjrd]

Not thinkable unless you want to break binary and TASTy compatibility of the entire world, which would mean Scala 4.

[lihaoyi]

It would also probably result in a ton of redundant forwarders that aren't actually necessary, because the Scala compiler cannot reason across published artifact versions to figure out which forwarders are necessary and which are not

Some IDL or schema compilers are able to do cross-version analysis and validation, but the Scala compiler traditionally doesn't, and adding such a workflow would be very invasive and not worth the ROI

[lihaoyi]

I have updated the prototype implementation to use the post-pickler/invisible approach to hiding the generated forwarder methods, and updated the doc accordingly

[lihaoyi]

@alexarchambault I have published the latest version of @unroll, in accordance to this proposal. You can use it in Mill via

  def scalacPluginIvyDeps = super.scalacPluginIvyDeps() ++ Agg(ivy"com.lihaoyi::unroll-plugin:0.1.12")
  def ivyDeps = super.ivyDeps() ++ Agg(ivy"com.lihaoyi::unroll-annotation:0.1.12")

[lrytz]

@unroll missing on the l: Long parameter (now that there's @unrollAll)

[sideeffffect]

In the description I've seen only @unroll in case class constructor.

Would it be possible to @unroll a whole case class as such? Meaning it would @unroll from the first parameter with default value.

[lihaoyi]

@sideeffffect can you elaborate on what you mean by "@unroll a whole case class" and how it differs from the behavior described under Case Classes?

[bjornregnell]

I guess this is the idea:

@unroll case class Data(x: Int, y: Int = 0, z: Int = 0)

So it is unrolled from y to z.

[lrytz]

In the current proposal that would be expressed

case class Data(x: Int, @unrollAll y: Int = 0, z: Int = 0)

[bjornregnell]

Yes, but I guess the idea is that it may be easier to spot/read if it's an annotation on the whole class. Less clutter in the param list. I'm not sure.

[sideeffffect]

Indeed, that's exactly what I had in mind, that

@unroll case class Data(x: Int, y: Int = 0, z: Int = 0)

would behave as

case class Data(x: Int, @unrollAll y: Int = 0, z: Int = 0)

does currently.

I think many people would prefer this over thinking where exactly in the constructor list to put the @unrollAll. I certainly would.

[lihaoyi]

Main view here is that "unroll everything" is most often not what you want.

You only want to @unroll the individual parameters that correspond to a new release of the library, which is a usually subset of the default parameters a method or case class. Often methods are defined with default parameters to begin with, and new versions of a library may add more than one default parameters to a method. You only want to @unroll the first default parameter for each new version of the library.

It doesn't do any harm to "@unroll everything", but it is wasteful in terms of generated code, and thus both compile times and classfile size. This was discussed when the SIP was initially proposed, and we decided to go with @unroll on specific parameters rather than @unrolling entire def or case classes as the default as it better reflects what you would actually want in reality despite being a bit more verbose and intrusive

[bjornregnell]

Could we then have both the shorter @unroll on individual parameters and a more explanatory similar to @unrollDefaultParams on the whole thing to cover both use cases?

You say "most often" but I'd assume that if you plan for binary compatibility then as soon as you add things all the params from the first update would need overloaded forwarders, so it seems to me that the use case of "from now on generate forwarders for all added params" would be the most common?

[lihaoyi]

You say "most often" but I'd assume that if you plan for binary compatibility then as soon as you add things all the params from the first update would need overloaded forwarders, so it seems to me that the use case of "from now on generate forwarders for all added params" would be the most common?

That is only true if you add one default parameter at a time per stable release of your library. That is sometimes the case, sometimes isn't. Even if no single PR adds multiple default parameters, a stable release often includes multiple PRs. If a stable release includes multiple default parameters, then you should only generate an overload for the first one and not all of them

[lihaoyi]

Another thing to consider is that @unrollAll still has to specify a parameter to start unrolling from. Often methods come with default parameters when first implemented, and you do not want to unroll any of those.

You only want to unroll default parameters added in subsequent stable releases, which means you need to specify a parameter from which unrolling will take place, which means putting the annotation on the entire method def/class/case class does not allow us to convey enough information to make that work. And we have to convey that information somehow, because the compiler does not have knowledge of what past versions of the codebase exist and thus cannot infer it automatically

[sideeffffect]

Continuing https://github.com/scala/improvement-proposals/pull/78/files#discussion_r1609571121 from @bjornregnell

Regarding the naming, I don't mind very much either way. But compatibility is a concept which doesn't exist within one class, but only as a relationship between that class in two versions of that code base. So maybe it would be better to name the annotation only after the mechanism (which incidentally helps with compatibility). The two Scala mechanisms at play here are "Default Parameters" and "Overloading". If we use them, the name could be

• @overloadDefaultParams
• @defaultParamsOverload
• @overloadDefaultParameters
• @defaultParametersOverload

But I have to admit that it's longer than @unroll.

[bjornregnell]

If you can annotate the whole thing the annotation can be kept on its own line and then the length of the name is not that big of a deal.

[bjornregnell]

I agree with @sideeffffect that compatibility intent is a relation so better try to find a name that reveals the mechanism.

I think I like @overloadDefaultParam best as the name of a "whole-thing-annotation".

[lihaoyi]

Yes if we annotate the whole method the length does not matter, but if we annotation individual parameters the length does matter. One potential name that keeps things short-ish is @overload, but I don't think it's significantly better or worse than @unroll (it has better meaning, but the use of a familiar term may lead people down the wrong path of understanding vs @unroll which a completely new term that they will know they need to look up

If we really must have a method-level annotation, one option is to specify individual parameter names in a method-level annotation @unroll("param1", "param2") def foo... , but that involves duplication of the parameter names and introduces an extra degree of freedom for people to make mistakes.

With the current approach of annotating the parameters themselves, it is important to keep the annotation short. And I'll say again that any one confused about @unroll is one click away from a detailed explanation in the Scaladoc, so I don't think we need to have a long and verbose name.

It's not like people unfamiliar can look at .pipe or .tap and immediately know what it means either, but because the detailed Scaladoc is one click away the initial confusion is easy to overcome. That means being concise can take priority over being verbose and spelling out exactly what the functionality does in a long and verbose identifier. I believe the same logic applies to @unroll

[bjornregnell]

Thanks for taking the time to explain all rationale! As usual, you have good answers to everything! :)

OK, I can live with @unroll on individual params. But we could still also have an annotation on the whole thing and the param to that annotation could be from where the unrolling starts, like in:

@unrollAllDefaultFrom("y")
case class Thing(x: Int, y: Int = 0, z: Int= 0)

as a more readable (when applicable) alternative to this:

case class Thing(x: Int, @unroll y: Int = 0, @unroll z: Int= 0)

[lihaoyi]

But we could still also have an annotation on the whole thing and the param to that annotation could be from where the unrolling starts, like in:

I would personally be against this. Having two ways to spell the same thing seems worse than having either one alone. When there's backwards compatibility or migration concerns, having some duplication seems fine, but since we have a chance to specify this from scratch we should really just pick one.

It basically comes down to the age-old tradeoff between "intrusive annotation" or "lookaside table", and the current proposal of "intrusive annotation" is a decent spot in a solution space. IMO there's probably no correct answer here, and there isn't that much to be gained by making it super verbose. People have IDEs these days and can navigate to the Scaladoc easily enough

[bjornregnell]

Well, the proposal already has two ways of doing things: @unroll and @unrollAll. But I agree that it's a matter of taste. I think I prefer that the unrollAll variant is placed on the whole thing with a param from where it starts. That way we both can have the cake and eat it, so to speak.

[lihaoyi]

@lrytz I have moved the Abstract Methods section out of the main proposal into the Alternatives section, as discussed during the meeting since we won't be implementing it as part of this proposal

[lrytz]

Suggested change

	3. The `@unroll`ed methods must be abstract or final
	3. The `@unroll`ed methods must be final

[lrytz]

The proposal was unanimously "Accepted for implementation" in the May SIP meeting, given these latest changes:

• @unroll is only supported on final methods; the "Abstract Methods" section is now under "Alternatives"
• @unrollAll is also under "Alternatives", so not part of the accepted proposal

[bishabosha]

@lihaoyi does the text in the merged file represent the current state approved spec?

[lihaoyi]

@bishabosha yes that's the current state of the approved spec. The Scala 3 compiler plugin in https://github.com/com-lihaoyi/unroll should mostly work, though we'd need to disable to abstract method support and replace it with error reporting for that and any non-final methods

[lihaoyi]

FYI for everyone, @bishabosha will be picking up the work integrating the current POC compiler plugin into the dotty mainline repo

[lihaoyi]

I've been playing around with this on 3.7.0-RC1, and so far hit two minor issues

• @unroll on a case class field without a default argument produces bad error message scala3#22832
• @unroll should work on final classes without needing a final on each method scala3#22833

I also brought the OS-Lib PR using @unroll up to date, using the new language feature rather than the compiler plugin. Seems to work fine

• [POC] Unroll default arguments for binary compatibility com-lihaoyi/os-lib#254

One last thing that's worth mentioning is that once we fix scala/scala3#22833, it means there will be a very concrete benefit for using final class and final case classes, as it will allow us to @unroll their methods whereas plain open class and case classes do not. The previous "manual" unrolling of methods was much more lax in this regard, and adding finals would be a breaking change for many libraries that do not have discipline around final.