Implement SIP-61 @unroll annotation

[bishabosha]

Still need to write documentation,
Documentation is written also

I am doing this work on behalf of @lihaoyi

The main implementation follows com-lihaoyi/unroll but with some changes:

• @unroll annotation is @experimental
• run before pickling
• increase validation checks for incorrect usage of @unroll
• Underscore not EmptyTree in pattern match default case
• ensure correct spans in TASTy
• ensure symbols are correctly substituted in types

There is one main library addition: scala.annotation.unroll, i.e. the @unroll annotation that appears on parameters,

commits are a bit of a mess - a lot of churn

Edit: removed changes:

• reuse the symbol when generating the "forwarder" of an abstract method.
• infer override when overriding trait methods that "appear" abstract in source code, but were actually implemented by the unroll annotation
• do not add the Invisible flag to abstract unrolled methods - this means they can actually still be visible in separately compiled compilation units (because unrolling runs before pickler now)
• Internal annotation scala.annotation.internal.AbstractUnroll - marker used to indicate that an unrolled abstract method was abstract in source code (unrolling phase actually provides a default implementation) - this enables downstream implementers to avoid the override flag. (Maybe we shouldn't allow this convenience?)
• Internal annotation scala.annotation.internal.UnrollForwarder - marker on any generated forwarder - which is used for special casing TASTy pickling of method calls to the forwarders
• because forwarders get the Invisible flag, this makes them impossible to select from TASTy normally (via SELECTin), (invisible def are filtered out "before typer") so I intercept the Select trees to be TERMREFdirect, and then restore them to Select after TASTY. perhaps someone has a better idea, or we could change the resolution rules for Invisible? or invent a new TASTy node? (I also tried generating a Ident tree rather than a Select, but this had a type error)

fixes #21728

[lihaoyi]

@bishabosha some high level comments:

1. We ended up removing the abstract def support from the SIP (SIP-61 - Unroll Default Arguments for Binary Compatibility improvement-proposals#78) since we weren't confident in the semantics, so let's disable it in the implementation as well and raise an error

2. It's not obvious to me from the tests, but is it possible to raise errors for the following cases:

   • @unroll on an invalid statement (e.g. on a val, or on a type foo: @unroll
   • @unroll on a non-final class/trait method: @unroll and its delegation model generally only works with final methods, and cannot support overrides with things getting wonky. object methods are final by default so we don't need to worry, but we should ask people to mark trait and class methods as final if they want to use @unroll on them

3. The original compiler plugin didn't have support for trait parameter lists. Would that be easy to add? It's probably not critical, but would be nice to have for consistency

[bishabosha]

Right, I should have noted that the abstract method support should have been dropped - now it is, I will push further commits with more invalidation checks, and see if trait constructor unroll can work

[bishabosha]

@lihaoyi there isn't a way to support trait constructor parameters that isn't a rewrite that a user could do manually, so I think this is unexplored territory - e.g. providing default implementations in bytecode for param accessors of traits

All the other concerns were addressed in above commits

[lihaoyi]

@lihaoyi there isn't a way to support trait constructor parameters that isn't a rewrite that a user could do manually, so I think this is unexplored territory - e.g. providing default implementations in bytecode for param accessors of traits

All the other concerns were addressed in above commits

Sounds good, let's skip trait params for now.

Anyone from the Scala 3 side able to review the code itself and the integration into the scala 3 codebase?

[bishabosha]

one implication I guess is that someone might not plan to use @unroll when they first made the API, so not make the method final, and then are potentially restricted from introducing @unroll in the future, because some client may have added an override. I guess we should include in our binary compatibilty documentation that you should make (all?) methods final

or we put in documentation to pre-introduce @deprecatedOverriding for this?

[lihaoyi]

I think forcing final is probably necessary, either explicitly by asking the user to put the keyword, or implicitly by having the compiler add it.

At least as currently designed, if code can override an @unrolled method, it can result in different callsites running different logic depending on what version of the @unrolled API they were compiled against, which violates all sorts of expectations.

So for case classes and object methods and constructors, things are already final so the requirement doesn't make a difference.

For class and trait methods, if they're not final but nobody is overriding them, then having the upstream API add final changes nothing. If they are not final but someone is overriding them, then adding final upstream is the difference between a loud JVM linkage error and a silent misbehavior, and I think the loud JVM linkage error is the preferable failure mode

[lihaoyi]

CC @lrytz since you're the manager, not sure if you should be reviewing this?

[bishabosha]

@Gedochao I thought experimental stuff can go in a patch? - I guess the tasty peculiarity - but that will have experimental flag in the tasty if used

[Gedochao]

@Gedochao I thought experimental stuff can go in a patch? - I guess the tasty peculiarity - but that will have experimental flag in the tasty if used

Ah, if it's behind a flag then it's fine, my bad.

[bishabosha]

@Gedochao I added the label back - this would not be correct to backport to 3.5.2 (no block at the tasty level), so it should be in 3.6.0 minimum

[bishabosha]

Just added in the documentation

[bishabosha]

I added commit b1c2ec0 which removes the TASTy hack. The commit changes the way forwarders are generated - i.e. they all call the original unrolled method, (rather than "telescoping" and calling the next invisible forwarder).

Since we no longer need to resolve calls to Invisible methods, the TASTy hack is no longer needed

[bishabosha]

I was talking to @sjrd today, and there is still a problem with Invisible - we have to consider the case of an inline method that calls a method, and then in V2 they add an unrolled parameter. The TASTy for the inline method when spliced will still try to resolve the old method by name+signature (i.e. try to resolve the new invisible forwarder) and fail.

[lihaoyi]

How about if we leave the synthetic overloads visible. They're meant to be equivalent semantically - it should not matter which one ends up getting called. That's the current manual status quo and it works well enough.

[sjrd]

That would definitely have user-visible consequences. For starters, type inference gets worse as soon as a method has overloads: you don't get expected types, so you don't get lambda param type inference, and implicit conversions may be inserted later or sooner than otherwise.

If we need to go that far, it's a language design change compared to the SIP, so we need to go back there. But I don't think we need to do that; if Invisible does not have the semantics we want, we can introduce a new TASTy flag with the semantics we want.

[lihaoyi]

A new flag sounds reasonable if it's possible on the implementation side

[bishabosha]

I added a test in ae4ceba that fails to resolve a forwarder when inlining a transparent inline method, then I fixed the test by introducing a new flag SourceInvisible (SOURCEINVISIBLE in tasty) that is identical to Invisible, except in the case of resolving SELECTin from TASTy

perhaps we can fold the behaviour of SourceInvisible into Invisible, depending on discussion

[bishabosha]

Green ci again

[lihaoyi]

Bumping reviewers @sjrd @lrytz, since it's been a few weeks

[lrytz]

Looks great! I didn't look at transform/UnrollDefinitions.scala in detail.

Is there anything that should be done in MiMa, or in tasty-mima?

[bishabosha]

I'll work on the feedback tonight

[bishabosha]

I've added in b894262 support for //> using target.platform jvm|scala-js in vulpix tests - which filter out files not matching the "test platform" - which also allows to have a separate check file for each platform.

If this is acceptable, should it be lifted to a separate PR?

[bishabosha]

@lrytz All comments addressed - CI green
@Gedochao @WojciechMazur who else should look at this?

[Gedochao]

@bishabosha we'll discuss this on this week's Scala Core

[Gedochao]

This is planned to be included in 3.7.0.
Waiting for one last round of review (cc @sjrd)

[lrytz]

Is there anything that should be done in MiMa, or in tasty-mima?

@bishabosha ping on that question

[bishabosha]

Is there anything that should be done in MiMa, or in tasty-mima?

for MiMa, the forwarders should look like ordinary scala methods (except with invisible flag), and will be present in TASTy/bytecode as usual.

For tasty-mima - there is the new semantic of the Invisible flag which is that it should be visible when resolved from the TASTy of an inline method.

[sjrd]

I don't think we need anything in tasty-mima. tasty-query ignores the Invisible flag, which corresponds to the new semantics, as far as it is concerned.

[sjrd]

@bishabosha Is the git history relevant? Or should it all be squashed to help future git blames?

[bishabosha]

Is the git history relevant? Or should it all be squashed to help future git blames?

there is perhaps 3-commit split - the initial commit which is copy-paste the com-lihaoyi/unroll code, commit 2 which is "the rest of the work" and perhaps a third commit which adds the vulpix directive parsing stuff

[sjrd]

I'll let you decompose as you see fit. Then we can merge.

[bishabosha]

@sjrd I squashed the commits