Gen return types for single expressions #66
Conversation
|
Test PASSed. See Console Output in the link below for an update site containing this PR binary artefacts. Refer to this link for build results: https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-validator/41/ |
|
LGTM, but it wouldn't hurt to have @misto or @huitseeker have a look. I'm not very knowledgeable of refactoring code. |
|
See my comment on the original bug-report |
|
@huitseeker, since you mentioned the quick fix... that one uses the Scala parser and tokenizer to find the insertion point. |
|
FYI, there are some helper methods in the CommentsUtils object that strip comments from source code. |
|
@misto Comments aren't the problem. Save actions should use the quickkfix's way of dealing with this. |
|
@huitseeker I know, I just wanted to have mentioned it because it's sometimes useful to be able to strip comments. |
|
About the save action that is the origin of this PR: I already need scala-refactoring to find definitions where a type can be added - continue using scala-refactoring by adding the return type is just logical. And the quick fix logic is also not bulletproof - moving to it for the transformation wouldn't even be an advantage. |
|
@sschaef : did you report the holes you found against the quick fix ? I just thought integrating the quickfix logic can be a temporary solution for the Save action if it grants better correctness than refactoring currently allows. |
In case
def foo = {
0
}
should be transformed to
def foo: Int = {
0
}
by adding its return type, the result was
def foo: Int = 0
}
Fixes #1002268
I would prefer to fix this somewhere in the printing logic and not in
the construction logic of the tree but I didn't find a way to
differentiate the Ident trees correctly in the printer. For
object X { def x = 0 }
the Ident trees of `X.type` and `X.x` seem to have the same shape at the
beginning.
Fixes #1002233
Fixing this was complicated because the compiler removes braces from the tree when the code would compile without them. Therefore we manually have to restore them. To get the braces (and whitespace) back, we search for an equal sign in the area of the DefDef (which indicates the start of the rhs). Once it is found, all contents between the equal sign and the start and the expression of the rhs are put back into the tree. Because an equal sign can also occur in a comment, we have to parse the region instead of simply looking for the equal sign. This adds some overhead, but hopefully not too much. Furthermore, as reviewers suggested, some variable names are renamed. This also fixes a bug in the test suite where the source file is passed to the refactoring logic.
kiritsuku
force-pushed
the
gen-return-types-for-single-expressions
branch
from
9fde188 to
98c0a55
Compare
|
I finally updated this PR. Sorry for the long delay! The last commit is new, it took me only a day to create it. Please review this commit carefully, it introduces a token cache to |
|
Refer to this link for build results (access rights to CI server needed): https://jenkins.scala-ide.org:8496/jenkins/job/ghprb-scala-refactoring-validator/52/ |
|
The last commit breaks source compatibility with the IDE and probably other repos as well (because |
|
Thank you! I'll review it over the weekend. Are you sure the additional self type is actually a problem? If they just use the SourceGenerator - which they should - then it should be fine, right? |
|
In the IDE there is |
|
Oh yes, |
|
@sschaef I just noticed you have a bunch of outstanding PRs against this repo. Are they rotten or worth saving? |
|
I should invest some time to finish these changes. I think it is best to keep the PRs open for now, even though I don't have time (or better interest) currently to fix it. It is not that they do any harm. |
|
Test FAILed. |
Some bugfixes that were made visible through scala-ide/scala-ide#761