-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Implement SIP-61 @unroll
annotation
#21693
base: main
Are you sure you want to change the base?
Conversation
@bishabosha some high level comments:
|
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 |
@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? |
one implication I guess is that someone might not plan to use or we put in documentation to pre-introduce |
I think forcing At least as currently designed, if code can override an So for For |
CC @lrytz since you're the manager, not sure if you should be reviewing this? |
@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. |
@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 |
631a6bb
to
4868397
Compare
Just added in the documentation |
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 |
b0ece26
to
aec0618
Compare
Co-authored-by: Jamie Thompson <bishbashboshjt@gmail.com> Co-authored-by: Li Haoyi <haoyi.sg@gmail.com>
Traits are forbidden in source code from having secondary constructors, which is what the current transform would generate. Trait parameters are encoded in concrete implementing classes as getter methods, perhaps unroll could provide default implementations, but this is unexplored.
Now, all forwarders directly call the unrolled method. This means that there is no need to resolve an invisible method, so the "hack" using TERMREFdirect is no longer needed. This increases bytecode size but reduces stack depth. Also removed scala.annotation.internal.UnrolledForwarder, as it is only needed for the TASTy hack.
Also standardise error messages as Declaration Errors
6d1fffa
to
a6b757c
Compare
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
@unroll
Underscore
notEmptyTree
in pattern match default caseThere 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.inferoverride
when overriding trait methods that "appear" abstract in source code, but were actually implemented by the unroll annotationdo not add theInvisible
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 annotationscala.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 annotationscala.annotation.internal.UnrollForwarder
- marker on any generated forwarder - which is used for special casing TASTy pickling of method calls to the forwardersbecause forwarders get theInvisible
flag, this makes them impossible to select from TASTy normally (viaSELECTin
), (invisible def are filtered out "before typer") so I intercept theSelect
trees to beTERMREFdirect
, and then restore them to Select after TASTY. perhaps someone has a better idea, or we could change the resolution rules forInvisible
? or invent a new TASTy node? (I also tried generating aIdent
tree rather than aSelect
, but this had a type error)