Scala Refactoring and Named Parameters

[Matthias Langer]

Hello everybody,

as you might well be aware of, renaming in scala-refactoring is currently broken in connection with named parameters (see https://scala-ide-portfolio.assembla.com/spaces/scala-ide/tickets/1002501-rename-does-not-consider-named-parameters). I would very much like to fix that, but after the typer has run, there does not seem to be any information about named arguments left in the tree. To give you a concrete example, the method "g" in

class Bug {
  def f(x: Int, y: Int) = x + y
  def g(a: Int, b: Int) = f(y = b, x = a)
}

gets parsed into

DefDef(
  Modifiers(0, , List()), g, List(), List(
    List(
      ValDef(Modifiers(8192, , List()), a, Ident(Int), EmptyTree()), ValDef(Modifiers(8192, , List()), b, Ident(Int), EmptyTree())
    )
  ), TypeTree(), Apply(
    Ident(f), List(AssignOrNamedArg(Ident(y), Ident(b)), AssignOrNamedArg(Ident(x), Ident(a)))
  )
)

The named parameters are clearly represented here; after the typer has run however, I get

DefDef(
  Modifiers(), TermName("g"), List(), List(
    List(
      ValDef(
        Modifiers(PARAM), TermName("a"), TypeTree().setOriginal(Select(Ident(scala), scala.Int)), EmptyTree
      ), ValDef(
        Modifiers(PARAM), TermName("b"), TypeTree().setOriginal(Select(Ident(scala), scala.Int)), EmptyTree
      )
    )
  ), TypeTree(), Block(
    List(
      ValDef(Modifiers(ARTIFACT), TermName("x$1"), TypeTree(), Ident(TermName("b"))), ValDef(Modifiers(ARTIFACT), TermName("x$2"), TypeTree(), Ident(TermName("a")))
    ), Apply(
      Select(This(TypeName("Bug")), TermName("f")), List(Ident(TermName("x$2")), Ident(TermName("x$1")))
    )
  )
)

I could not find any useful information about the named parameters in the tree from above after careful inspection, but maybe I did not look close enough.... what are your ideas about this issue? Would it be possible to add an "original" field, where information about named parameters is retained?

Thank you very much,
Matthias

[Som Snytt]

AFAIK, transformNamedApplication is eager and destructive, and there is not an attachment such as one sees in macros to show the "before" tree.

The workaround might be to use positions to examine the source, reparsing it for pleasure.

--
You received this message because you are subscribed to the Google Groups "scala-internals" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scala-interna...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Mirko Stocker]

On Sunday 05 July 2015 13.09:21 Som Snytt wrote:
> The workaround might be to use positions to examine the source, reparsing it
> for pleasure.

I've tried this approach, but then had to revert it because it was too slow
IIRC. But the code is still there, in the ApplyExtractor and BlockExtractor
objects in PimpedTrees. The attached patch enables it, and there are even some
@ignored tests in RenameTest.

Cheers

Mirko

--
Mirko Stocker | mi...@stocker.email
Work: http://ifs.hsr.ch | http://infoq.com
Personal: http://misto.ch | http://twitter.com/m_st

[Lukas Rytz]

Would it be enough to keep the original (untyped) `Apply` node an attachment?
Do we have other examples where the presentation compiler keeps more data around?

BTW, in Scala 2.11, f(x = 1) can mean f({x = 1; ()}), but should be changed in 2.12:
  https://groups.google.com/forum/#!topic/scala-internals/MujIVZgbwEM

[iulian dragos]

On Mon, Jul 6, 2015 at 11:54 AM, Lukas Rytz <lukas...@gmail.com> wrote:

Would it be enough to keep the original (untyped) `Apply` node an attachment?

I think that should work.

 

Do we have other examples where the presentation compiler keeps more data around?

One example that comes to mind is in TypeTrees (the .original keeps the type selection, if there was one).

 

BTW, in Scala 2.11, f(x = 1) can mean f({x = 1; ()}), but should be changed in 2.12:
  https://groups.google.com/forum/#!topic/scala-internals/MujIVZgbwEM

On Mon, Jul 6, 2015 at 9:43 AM, Mirko Stocker <mi...@stocker.email> wrote:

On Sunday 05 July 2015 13.09:21 Som Snytt wrote:
> The workaround might be to use positions to examine the source, reparsing it
> for pleasure.

I've tried this approach, but then had to revert it because it was too slow
IIRC. But the code is still there, in the ApplyExtractor and BlockExtractor
objects in PimpedTrees. The attached patch enables it, and there are even some
@ignored tests in RenameTest.

Cheers

Mirko

--
Mirko Stocker | mi...@stocker.email
Work: http://ifs.hsr.ch | http://infoq.com
Personal: http://misto.ch | http://twitter.com/m_st

--
You received this message because you are subscribed to the Google Groups "scala-internals" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scala-interna...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "scala-internals" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scala-interna...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

« Je déteste la montagne, ça cache le paysage »
Alphonse Allais

[Lukas Rytz]

On Mon, Jul 6, 2015 at 12:28 PM, iulian dragos <jagu...@gmail.com> wrote:

On Mon, Jul 6, 2015 at 11:54 AM, Lukas Rytz <lukas...@gmail.com> wrote:

Would it be enough to keep the original (untyped) `Apply` node an attachment?

I think that should work.

 

Do we have other examples where the presentation compiler keeps more data around?

One example that comes to mind is in TypeTrees (the .original keeps the type selection, if there was one).

AFAIK this is not specific to the presentation compiler. For the Apply nodes we should

maybe save the overhead in the normal compiler.

[iulian dragos]

On Mon, Jul 6, 2015 at 1:00 PM, Lukas Rytz <lukas...@gmail.com> wrote:

On Mon, Jul 6, 2015 at 12:28 PM, iulian dragos <jagu...@gmail.com> wrote:

On Mon, Jul 6, 2015 at 11:54 AM, Lukas Rytz <lukas...@gmail.com> wrote:

Would it be enough to keep the original (untyped) `Apply` node an attachment?

I think that should work.

 

Do we have other examples where the presentation compiler keeps more data around?

One example that comes to mind is in TypeTrees (the .original keeps the type selection, if there was one).

AFAIK this is not specific to the presentation compiler. For the Apply nodes we should

maybe save the overhead in the normal compiler.

Do you think the overhead is significant, if done only when the named args are used? You can make it specific to the presentation compiler, of course, but seems a bit more work.

BTW, the transformation is simpler (no synthetics local vars) when named arguments are not necessary (their position matches the position in the definition).

[Matthias Langer]

I can take a closer look at this at the weekend... still, even if the performance was OK, duplicating parsing logic for something that has just been parsed seems far from ideal to me. Attaching the original tree to the transformed Apply node as discussed below makes much more sense.

[Jason Zaugg]

On Tue, Jul 7, 2015 at 6:11 AM Matthias Langer <m.lan...@gmail.com> wrote:

I can take a closer look at this at the weekend... still, even if the performance was OK, duplicating parsing logic for something that has just been parsed seems far from ideal to me. Attaching the original tree to the transformed Apply node as discussed below makes much more sense.

Here's a related issue with some discussion of using a tree attachment to keep track of the use of parameter names.

https://github.com/scala-ide/scala-refactoring/issues/44

We could also use the additional metadata to solve: https://issues.scala-lang.org/browse/SI-5920 (Support names/defaults in macros.)

-jason

[Vlad Ureche]

Hi,

I also use tree attachments to recover pre-typer information after the typer. Here's the place where I attach it: PostParserTreeTransformer.scala and where I recover it: InjectTreeTransformer.scala.

If you're curious what this does, here's a explanation and a long-ish example: https://github.com/miniboxing/ildl-plugin/wiki/Tutorial-~-Introduction

HTH,

Vlad

--

[Matthias Langer]

Yes, I think that attaching additional information to the tree like this would be very helpful... what are your plans on this?

[Matthias Langer]

Am Montag, 6. Juli 2015 09:43:45 UTC+2 schrieb Mirko Stocker:

On Sunday 05 July 2015 13.09:21 Som Snytt wrote:
> The workaround might be to use positions to examine the source, reparsing it
> for pleasure.

I've tried this approach, but then had to revert it because it was too slow
IIRC. But the code is still there, in the ApplyExtractor and BlockExtractor
objects in PimpedTrees. The attached patch enables it, and there are even some
@ignored tests in RenameTest.

I've just tried your patch; the tests pass indeed but to make this work in the IDE further tweaks are needed, as the positions passed to org.scalaide.util.eclipse.EditorUtils.enterLinkedModeUi might have wrong lengths; this should be fixable though... How did the performance problems you mentioned manifest themselves?

Cheers,
Matthias

[Lukas Rytz]

We don't have this on our short list right now, but we'd be happy to assist if you'd like to work on it,
and we can provide some pointers how things could be done.

[Matthias Langer]

We don't have this on our short list right now, but we'd be happy to assist if you'd like to work on it,
and we can provide some pointers how things could be done.

Do you think it's realistic to get this into one of the next 2.11.x releases? I could give it a try... this would be my first patch to Scala though and therefore would involve lots initial overhead.

[Lukas Rytz]

We could enable the additional metadata only when running the presentation compiler, or under

some compiler flag. Then I don't see a risk putting it into a 2.11.x release.

--

[Mirko Stocker]

On Wednesday 15 July 2015 11.25:05 you wrote:
> How did the performance problems you mentioned manifest themselves?

Sorry for the late reply, I was on vacation..

I found the original thread after which we disabled the named arguments patch:
https://groups.google.com/forum/#!topic/scala-ide-user/fmTwrowuvHc

Reading it again, we should only do these more expensive checks when renaming,
and not for mark occurrences, then the performance impact shouldn't be that
painful. What do you think?

[Matthias Langer]

I'm going to take a closer look into this in the coming week although I think that a solution relying on information attached to the tree by the compiler would be preferable.

[Matthias Langer]

>  How did the performance problems you mentioned manifest themselves?

Sorry for the late reply, I was on vacation..

I found the original thread after which we disabled the named arguments patch:
https://groups.google.com/forum/#!topic/scala-ide-user/fmTwrowuvHc

Reading it again, we should only do these more expensive checks when renaming,
and not for mark occurrences, then the performance impact shouldn't be that
painful. What do you think?

Hmm, I'm experimenting with your solution which seems quite promising so far:
https://github.com/mlangc/scala-refactoring/tree/rename-and-named-params-1002501 (scala-refactoring branch)
https://github.com/mlangc/scala-ide/tree/rename-and-named-params-1002501 (related scala-ide branch)

I've done some performance benchmarking (see https://github.com/mlangc/scala-refactoring-experiments/blob/master/src/test/scala/com/github/mlangc/experiments/refactoring/RenameBenchmark.scala) and could not see any regressions after enabling the named parameters functionality.

The problems that I see come from excessive memory consumption in scala.tools.refactoring.analysis.CompilationUnitIndexes.CompilationUnitIndex for realistically sized projects(which causes the VM to get stuck in GC); at least that's what jvisualvm and eclipse-mat are indicating. I've still trying to figure out why this happens, but I think that if we solve this problem, we have a working solution. Do you have any ideas?

If we could easily bring your implementation in scala-refactoring back into production, we would have a notably enhanced renaming feature quite soon. Of course that doesn't change the fact that technically, adapting the compiler to attach the needed information to the ASTs, is to be preferred.

[iulian dragos]

I really think that tree attachments are the right way. 2.11.8 is due around September, so there's some time to still put this in. Would you like to give it a try? Lukas offered to provide some guidance :)

iulian

 

--
You received this message because you are subscribed to the Google Groups "scala-internals" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scala-interna...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Mirko Stocker]

Hi

Again, sorry for the slow reply, I (again) was on vacation (the last one this
year :-).

> > and could not see any regressions after enabling the named parameters
> > functionality.

Ok, that sounds good!

> > If we could easily bring your implementation in scala-refactoring back
> > into production, we would have a notably enhanced renaming feature quite
> > soon. Of course that doesn't change the fact that technically, adapting
> > the
> > compiler to attach the needed information to the ASTs, is to be preferred.
>
> I really think that tree attachments are the right way. 2.11.8 is due
> around September, so there's some time to still put this in. Would you like
> to give it a try? Lukas offered to provide some guidance :)

This sounds like a great opportunity to fix the problem at the root, and your
time is probably better spent here than figuring out memory leaks and worrying
about performance regressions. What do you think?

[Matthias Langer]

Am Donnerstag, 30. Juli 2015 17:32:18 UTC+2 schrieb Mirko Stocker:

Hi

Again, sorry for the slow reply, I (again) was on vacation (the last one this
year :-).

No need to be sorry, I hope you enjoyed your vacation!
 

> > and could not see any regressions after enabling the named parameters
> > functionality.

Ok, that sounds good!


> > If we could easily bring your implementation in scala-refactoring back
> > into production, we would have a notably enhanced renaming feature quite
> > soon. Of course that doesn't change the fact that technically, adapting
> > the
> > compiler to attach the needed information to the ASTs, is to be preferred.
>
> I really think that tree attachments are the right way. 2.11.8 is due
> around September, so there's some time to still put this in. Would you like
> to give it a try? Lukas offered to provide some guidance :)

This sounds like a great opportunity to fix the problem at the root, and your
time is probably better spent here than figuring out memory leaks and worrying
about performance regressions. What do you think?

I think you are right, investing time in a proper solution is probably smarter. I'll see what I can do.