-
-
Notifications
You must be signed in to change notification settings - Fork 4
[Issue 12] @unroll
on final or effectively final methods
#15
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
Conversation
@arainko those tests should be converted into negative tests: we should assert the failure and provide an appropriate error message. We should also ensure |
So before I go on an adventure: do you have a goto mechanism for testing compilation failures? There's |
uTest has |
def foo(int: Int, @unroll str: String = "1") = int.toString() + str | ||
|
||
def fooInner(int: Int) = { | ||
def inner(a: Int, @unroll str: String = "1") = str |
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.
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?
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.
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
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.
Yeah, I feel like it should be possible - I'll stare at it some more and report back with my findings
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.
Managed to make it work with an additional traversal of the tree after we've matched a toplevel method.
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 |
Done, thanks for taking a look! |
Thanks @arainko ! please email me your international bank transfer details and I will close out the bounty |
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
andabstractTraitMethod
which had to be converted intonegative
tests) work after annotating their respective classes (or just the methods that were there before) withfinal
.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 rejectingtrait
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
Scala 2.13
Scala 2.12
Fixes #12