Skip to content

Conversation

arainko
Copy link
Contributor

@arainko arainko commented Sep 2, 2025

The Scala 3 version of the plugin now closely follows the various checks introduced in the SIP implementation (with the actual impl being a mix of these 3 diffs: diff 1, diff 2, diff 3).

For the Scala 2 version I had to get a tad bit more creative since it lacked (or I just couldn't find the correct combinations of methods, lol) built-in checks for the likes of a method being local (which is now checked by iterating over the owners and checking if any of them is a method) or it had straight up different behavior compared to dotty (e.g. constructors not being considered effectively final).

All of the original test cases (with the exception of abstractClassMethod and abstractTraitMethod which had to be converted into negative tests) work after annotating their respective classes (or just the methods that were there before) with final.

There's also the aforementioned negative module which has two variants - a Scala 3 and a Scala 2 one (the only difference between those being an additional check for rejecting trait constructors on the Scala 3 side). This module is not automatically checked in any shape or form (pretty much functions as an 'eyeball' test for whoever looks at the code).
FTR these are the compilation errors being reported at the time of writing this PR:

Scala 3
aleksander@pop-os:~/repos/unroll-1$ ./mill -i -k "unroll[3.7.1].negative.compile"
Another Mill process is running 'IDE:buildTargetDependencyModules', waiting for it to be done...
[156/156] unroll[3.7.1].negative.compile
[156] [info] compiling 1 Scala source to /home/aleksander/repos/unroll-1/out/unroll/3.7.1/negative/compile.dest/classes ...
[156] [error] -- Error: /home/aleksander/repos/unroll-1/unroll/negative/scala3/src/invalidunroll.scala:6:28 
[156] [error] 6 |  def foo(int: Int, @unroll str: String = "1") = int.toString() + str
[156] [error]   |                            ^
[156] [error]   |       Cannot unroll parameters of method foo because it can be overridden
[156] [error] -- Error: /home/aleksander/repos/unroll-1/unroll/negative/scala3/src/invalidunroll.scala:9:8 
[156] [error]  9 |    def inner(a: Int, @unroll str: String = "1") = {
[156] [error]    |    ^
[156] [error]    |   Cannot unroll parameters of method inner because it is a local method
[156] [error] 10 |      def anEvenMoreInnerMethUnrolled(@unroll int: Int = 1) = ()
[156] [error] 11 |      str
[156] [error] 12 |    }
[156] [error] -- Error: /home/aleksander/repos/unroll-1/unroll/negative/scala3/src/invalidunroll.scala:18:29 
[156] [error] 18 |  def foo(s: String, @unroll n: Int = 1): String
[156] [error]    |                             ^
[156] [error]    |     Cannot unroll parameters of method foo because it can be overridden
[156] [error] -- Error: /home/aleksander/repos/unroll-1/unroll/negative/scala3/src/invalidunroll.scala:22:29 
[156] [error] 22 |  def foo(s: String, @unroll n: Int = 1): String
[156] [error]    |                             ^
[156] [error]    |     Cannot unroll parameters of method foo because it can be overridden
[156] [error] -- Error: /home/aleksander/repos/unroll-1/unroll/negative/scala3/src/invalidunroll.scala:21:27 
[156] [error] 21 |trait FooTrait(@unroll val param: Int = 1, @unroll val param2: String = "asd") {
[156] [error]    |                           ^
[156] [error]    |implementation restriction: Cannot unroll parameters of a trait constructor
[156] [error] -- Error: /home/aleksander/repos/unroll-1/unroll/negative/scala3/src/invalidunroll.scala:21:55 
[156] [error] 21 |trait FooTrait(@unroll val param: Int = 1, @unroll val param2: String = "asd") {
[156] [error]    |                                                       ^
[156] [error]    |implementation restriction: Cannot unroll parameters of a trait constructor
[156] [error] -- Error: /home/aleksander/repos/unroll-1/unroll/negative/scala3/src/invalidunroll.scala:26:12 
[156] [error] 26 |  final def copy(int: Int = int, @unroll str: String = str): FooCaseClass = FooCaseClass(int, str)
[156] [error]    |            ^
[156] [error]    |Cannot unroll parameters of method copy of a case class: please annotate the class constructor instead
[156] [error] -- Error: /home/aleksander/repos/unroll-1/unroll/negative/scala3/src/invalidunroll.scala:30:6 
[156] [error] 30 |  def apply(int: Int, @unroll str: String): FooCaseClass = FooCaseClass(int, str)
[156] [error]    |      ^
[156] [error]    |Cannot unroll parameters of method apply of a case class companion object: please annotate the class constructor instead
[156] [error] 8 errors found
[156] unroll[3.7.1].negative.compile failed
[156/156, 1 failed] ============================== unroll[3.7.1].negative.compile ============================== 9s
1 tasks failed
unroll[3.7.1].negative.compile Compilation failed
Scala 2.13
aleksander@pop-os:~/repos/unroll-1$ ./mill -i -k "unroll[2.13.12].negative.compile"
Another Mill process is running 'IDE:buildTargetJavacOptions', waiting for it to be done...
[156/156] unroll[2.13.12].negative.compile
[156] [info] compiling 1 Scala source to /home/aleksander/repos/unroll-1/out/unroll/2.13.12/negative/compile.dest/classes ...
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:6:29: Cannot unroll parameters of method foo because it can be overridden
[156] [error]   def foo(int: Int, @unroll str: String = "1") = int.toString() + str
[156] [error]                             ^
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:9:9: Cannot unroll parameters of method inner because it is a local method
[156] [error]     def inner(a: Int, @unroll str: String = "1") = {
[156] [error]         ^
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:10:11: Cannot unroll parameters of method anEvenMoreInnerMethUnrolled because it is a local method
[156] [error]       def anEvenMoreInnerMethUnrolled(@unroll int: Int = 1) = ()
[156] [error]           ^
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:18:30: Cannot unroll parameters of method foo because it can be overridden
[156] [error]   def foo(s: String, @unroll n: Int = 1): String
[156] [error]                              ^
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:22:30: Cannot unroll parameters of method foo because it can be overridden
[156] [error]   def foo(s: String, @unroll n: Int = 1): String
[156] [error]                              ^
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:26:42: Cannot unroll parameters of method copy of a case class companion object: please annotate the class constructor instead
[156] [error]   final def copy(int: Int = int, @unroll str: String = str): FooCaseClass = FooCaseClass(int, str)
[156] [error]                                          ^
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:30:31: Cannot unroll parameters of method apply of a case class companion object: please annotate the class constructor instead
[156] [error]   def apply(int: Int, @unroll str: String): FooCaseClass = FooCaseClass(int, str)
[156] [error]                               ^
[156] [error] 7 errors found
[156] unroll[2.13.12].negative.compile failed
[156/156, 1 failed] ============================== unroll[2.13.12].negative.compile ============================== 5s
1 tasks failed
unroll[2.13.12].negative.compile Compilation failed
Scala 2.12
aleksander@pop-os:~/repos/unroll-1$ ./mill -i -k "unroll[2.12.18].negative.compile"
[156/156] unroll[2.12.18].negative.compile
[156] [info] compiling 1 Scala source to /home/aleksander/repos/unroll-1/out/unroll/2.12.18/negative/compile.dest/classes ...
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:6:29: Cannot unroll parameters of method foo because it can be overridden
[156] [error]   def foo(int: Int, @unroll str: String = "1") = int.toString() + str
[156] [error]                             ^
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:9:9: Cannot unroll parameters of method inner because it is a local method
[156] [error]     def inner(a: Int, @unroll str: String = "1") = {
[156] [error]         ^
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:10:11: Cannot unroll parameters of method anEvenMoreInnerMethUnrolled because it is a local method
[156] [error]       def anEvenMoreInnerMethUnrolled(@unroll int: Int = 1) = ()
[156] [error]           ^
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:18:30: Cannot unroll parameters of method foo because it can be overridden
[156] [error]   def foo(s: String, @unroll n: Int = 1): String
[156] [error]                              ^
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:22:30: Cannot unroll parameters of method foo because it can be overridden
[156] [error]   def foo(s: String, @unroll n: Int = 1): String
[156] [error]                              ^
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:26:42: Cannot unroll parameters of method copy of a case class companion object: please annotate the class constructor instead
[156] [error]   final def copy(int: Int = int, @unroll str: String = str): FooCaseClass = FooCaseClass(int, str)
[156] [error]                                          ^
[156] [error] /home/aleksander/repos/unroll-1/unroll/negative/scala2/src/invalidunroll.scala:30:31: Cannot unroll parameters of method apply of a case class companion object: please annotate the class constructor instead
[156] [error]   def apply(int: Int, @unroll str: String): FooCaseClass = FooCaseClass(int, str)
[156] [error]                               ^
[156] [error] 7 errors found
[156] unroll[2.12.18].negative.compile failed
[156/156, 1 failed] ============================== unroll[2.12.18].negative.compile ============================== 6s
1 tasks failed
unroll[2.12.18].negative.compile Compilation failed

Fixes #12

@lihaoyi
Copy link
Member

lihaoyi commented Sep 2, 2025

@arainko those tests should be converted into negative tests: we should assert the failure and provide an appropriate error message. We should also ensure @unroll on non-final defs on non-final classes to make sure they fail as well with a proper error message

@arainko
Copy link
Contributor Author

arainko commented Sep 2, 2025

So before I go on an adventure: do you have a goto mechanism for testing compilation failures? There's scala.compiletime.testing.typeCheckErrors for Scala 3 but it doesn't seem to take plugins into consideration (Scala 2 has Universe#typecheck which could be resurfaced as a macro as well but I suppose it also doesn't care about plugins?). Or am I on a path to directly invoking the published compiler JAR? 😄

@lihaoyi
Copy link
Member

lihaoyi commented Sep 2, 2025

uTest has assertCompileError, but this repo doesn't use it. I think for now we can just run __.test with --keep-going and eyeball the ones that fail to make sure it's the right ones with the right error

def foo(int: Int, @unroll str: String = "1") = int.toString() + str

def fooInner(int: Int) = {
def inner(a: Int, @unroll str: String = "1") = str
Copy link
Contributor Author

@arainko arainko Sep 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

full disclosure - local methods don't seem to get picked up by the plugin at all so they don't go through the validation phase which means that they are just silently ignored (on all Scala versions). I left this test here for visibility and discussion - should I remove the local checks and this definition?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way to make them get picked up by the compiler plugin? I would expect compiler plugins to have access to the entire AST of the file to walk through, which should let it find these problematic usages. If not that's fine too, but it seems like it should be doable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I feel like it should be possible - I'll stare at it some more and report back with my findings

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Managed to make it work with an additional traversal of the tree after we've matched a toplevel method.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 13, 2025

I think this looks fine @arainko, if you could update the PR description with your latest implementation details and (manual?) testing strategy I think we can merge this and close out the bounty

@arainko
Copy link
Contributor Author

arainko commented Sep 13, 2025

Done, thanks for taking a look!

@lihaoyi lihaoyi merged commit 48a4d96 into com-lihaoyi:main Sep 14, 2025
3 checks passed
@lihaoyi
Copy link
Member

lihaoyi commented Sep 14, 2025

Thanks @arainko ! please email me your international bank transfer details and I will close out the bounty

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@unroll should only apply to final or effectively final methods (300USD Bounty)
2 participants