Consider named parameters during rename refactorings #141
Conversation
|
IIRC, one of the reasons the ApplyExtracted and BlockExtracter were commented out was that it had a huge impact on performance. Do you have any idea how the current situation is? We unfortunetly don't have tests for this. |
|
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/237/ |
|
Darn, I forgot to test with Scala-2.10... |
mlangc
force-pushed
the
1002501-rename-does-not-consider-named-parameters
branch
2 times, most recently
from
511b1f7
to
d5a150c
Compare
mlangc
changed the title
|
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/253/ |
| @@ -29,22 +29,73 @@ abstract class Rename extends MultiStageRefactoring with TreeAnalysis with analy | |||
|
|
|||
| def prepare(s: Selection) = { | |||
|
|
|||
| /* | |||
| * This heuristic decides if this refactoring is local to a single file or not. Note that | |||
There was a problem hiding this comment.
Looks like a sensible trade off between safety and ease of use.
|
I played around with a few of the tests but couldn't break it. Nice job! Unfortunately, there's a minor conflict (with the recent imports removal Simon did) so we can't merge it automatically. |
This commit contains some fixes that were necessary due to changes related to our migration to SBT.
This patch reenables named parameter extraction, that was already implemented, but deactivated for performance reasons (see https://groups.google.com/forum/embed/#!topic/scala-internals/H_psZ3RV6-4). We need this functionality so that named arguments can be handled properly by rename refactorings.
This commit fixes a bug, which resulted in a serious performance problem, that was the was the reason why support for named arguments was dropped in the past. See https://groups.google.com/forum/embed/#!topic/scala-internals/H_psZ3RV6-4 for details.
Renaming class parameters is a global operation, unless the constructor is explicitly marked as private.
This commit makes sure that a rename refactoring involving a class parameter also affects locations where this parameter is used as a named argument of the constructor, like `new Foo(bar = 42)`.
Scala-2.10 generates OffsetPositions at places where Scala-2.11 has RangePositions. This commit adapts the code accordingly, and makes it simpler along the way.
Up until now, the movements related to `SourceWithMarker` did not implement
backtracking. As a consequence, `'a'.zeroOrMore ~ 'a'` would not consume "a",
because `'a'.zeroOrMore` would always move over "a", leaving no room for the
following movement. While this wasn't a severe restriction for moving forward
since rules can be adapted accordingly, this becomes a real problem when moving
back, since `('a' ~ 'a'.zeroOrMore).backward` applies `'a'.zeroOrMore` before
`'a'`.
This commit makes sure that the parameters of compiler generated `apply` and `copy` methods for case classes are associated with the related class vals.
This commit works around various limitations and bugs when extracting named parameters.
This commit makes sure that case classes owned by methods are correctly handled when doing rename refactorings. Fix #1002501, Fix #1002572 together with the last commits
mlangc
force-pushed
the
1002501-rename-does-not-consider-named-parameters
branch
from
fefcd9d
to
c7c326c
Compare
|
@misto I've just rebased. |
Up until now, `Movements.until` stuck in an infinite loop for a `skipping` movement that could be satisfied by an empty string, like `'a'.zeroOrMore`. This isn't a severe limitation to someone who is well aware of this, but I think that it can be very annoying otherwise.
|
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/263/ |
|
|
||
| { | ||
| val src = SourceWithMarker("aaaa") | ||
| assertTrue(src.moveMarker('a'.zeroOrMore.zeroOrMore).isDepleted) |
There was a problem hiding this comment.
it's not a problem with this PR but maybe in future it's worth to consider renaming isDepleted to isConsumed. Currently it looks akin to PimpedTree.
There was a problem hiding this comment.
I agree... isConsumed sounds better indeed. A rename refacoring might be in order ;-). However, this might cause some conflicts with PR #144, so maybe we should wait with this until both PRs are merged?
There was a problem hiding this comment.
+1 for a separate PR so we can merge this one
|
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/271/ |
|
Impressive work. Worked very well in my tests. LGTM |
|
Generally LGTM, but I have one general question about refactoring. There is |
|
I think both |
|
@wpopielarski I see your point: Both As for |
|
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/273/ |
|
@sschaef I just added some Scaladocs for What puzzles me for example, is that both @misto, maybe you could shine some light on this? |
|
What I attempted to do with the various XyExtractors was to sanitize or undo some of the desugaring the compiler does, so that during pattern matching over a tree, we get NamedArgument trees even though they are not really part of the AST. And IIRC, depending on how named arguments calls are ordered, they are sometimes desugared to Blocks and sometimes simply to Apply trees (I believe when the named parameters are passed in the same order as the defined parameters, it's an Apply tree). The story is similar with multiple-assignments ( |
|
And yeah, luckily we have a very high test coverage, so if you manage to simplify something and get rid of code and no test breaks, then just go for it. |
|
@misto Thanks for the clarification; I've added a comment 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/283/ |
|
Let's test this in production |
This PR implements support for named parameters when doing rename refactorings (see Ticket #1002501, as well as Ticket #1002572). To achieve this, this PR takes care of several things:
SourceWithMarkerframework, that now supports backtracking (note that I've strong evidence for the fact that the newly added backtracking support should not cause performance regressions: https://github.com/mlangc/scala-refactoring-experiments/blob/master/src/test/scala/com/github/mlangc/experiments/refactoring/SourceWithMarkerBenchmark.scala).copyandapplymethods). This is what the updates related toSymbolExpanderare about.Rename.PreparationResult.hasLocalScopeif the method might be called from outside the file that defines it.Note that it is currently not possible to rename named parameters where they are used, but only where they are defined, that is you cannot initiate a rename refactoring at
f(tryRenameMe = 22), but only atdef f(tryRenameMe: Int). In my opinion, we should take care of this in a separate PR though because