Rewriting dup doesn't work if a case class has an implicit parameter

[GoogleCodeExporter]

Trying to rewrite instances of classes like this fails:

    case class A[T : Manifest](t: T)

Original issue reported on code.google.com by inkytonik on 15 Jun 2012 at 9:20

[GoogleCodeExporter]

Original comment by inkytonik on 15 Jun 2012 at 9:22

• Changed state: Accepted

[GoogleCodeExporter]

This issue was reported by Eric Torreborre.

I've been trying to reproduce this issue, but have had no luck so far. Eric, 
can you please take a look at the following when you have a chance and let me 
know how if differs from what you are doing?

Given the classes

    case class A (bs : List[B])
    abstract class B
    case class C[T : Manifest] (v : T) extends B
    case class D[T : Manifest] (v : T) extends B

I tried testing a rewrite of C's into D's:

    val t = A (List (C (1), C (2), C ("3")))
    val r = rule {
                case C (v) =>  D (v)
            }
    val s = everywheretd (r)
    expect (Some (A (List (D (1), D (2), D ("3"))))) (s (t))

This test passes, so there must be some extra aspect that I am missing.

Original comment by inkytonik on 26 Jun 2012 at 10:00

[GoogleCodeExporter]

I finally found a failing test!

    val elements = A(List (C(D(1), 0)))
    val r = rule {
      case C(D(i: Int), j: Int) => C(D(i), j+1)
      case D(i: Int)            => D(i + 1)
    }
    everywheretd (r)(elements) === Some (A (List (C(D(2), 1))))

case class A (bs : List[B])
sealed trait B
case class C[T : Manifest] (b: B, t: T) extends B
case class D[T : Manifest] (t: T) extends B

The issue seems to reside in lines like (in Rewriter):

  val numchildren = p.productArity

where productArity doesn't count implicit constructor arguments.

Original comment by etorrebo...@gmail.com on 27 Sep 2012 at 5:52

[GoogleCodeExporter]

Reproduced. Top of stack trace is 

    java.lang.RuntimeException: dup illegal arguments: public C(B,java.lang.Object,scala.reflect.Manifest) (D(2),1), expects 3

Original comment by inkytonik on 28 Sep 2012 at 7:18

[GoogleCodeExporter]

The problem triggered here is that the constructor of a case class that has an 
implicit parameter has the implicit parameter as an argument. So, C's 
constructor in the example above has three parameters. dup fails because the 
generic traversal only supplies the new values for the two explicit arguments.

At first glance, the solution is to replace uses of productArity with a process 
to calculate the actual number of arguments that the constructor has. We should 
be able to do this via reflection.

However, this change would create more complications. A generic traversal 
operator such as "all" processes all of the children. If we say that there are 
three children in the case of C above, then the implicit child will be passed 
to the rewrite strategy too. In a call "all (s)", s would be responsible for 
"rewriting" the Manifest/ClassTag in accordance with the rest of the rewrites 
that it is doing. This makes sense, since s knows what is appropriate for that 
value, but it makes the implicit quite explicit in the rewrites.

We could make "all" and similar operations just traverse to the actual product 
children and pass implicit values through. But this would be wrong in some 
cases for C because if the type T changes as a result of the rewrite, then you 
need to change the manifest/classtag too.

Finally, the use of reflection to work out the children count is overkill for 
all applications that use case classes with no implicits. We would be slowing 
down most of the use cases.

So, it's all messy and I'm not clear on how to proceed. Suggestions welcome!

Original comment by inkytonik on 30 Sep 2012 at 10:44

[GoogleCodeExporter]

> We could make "all" and similar operations just traverse to the actual 
product children and pass implicit values through. But this would be wrong in 
some cases for C because if the type T changes as a result of the rewrite, then 
you need to change the manifest/classtag too.

That was my expectation and I don't understand how "T changes as a result of 
the rewrite". What would be an example of that.

>Finally, the use of reflection to work out the children count is overkill for 
all applications that use case classes with no implicits. We would be slowing 
down most of the use cases.

Maybe this can be determined once and for all, then memoized? Or tried when the 
"dup" method fails? Or be a parameter of the "rewrite" method?

Original comment by etorrebo...@gmail.com on 30 Sep 2012 at 12:46

[GoogleCodeExporter]

An example of "T changes as a result of the rewrite" would be if you have a 
rule like

    val r = rule {
        case C(D(i: Int), j: Int) => C(D(true), false)
    }

The T of the new C is Boolean. That would be possible, wouldn't it?

Memoization would be possible, I think.

Doing it just when the dup fails won't be simple, since dup would have to 
return this information to the generic traversals, since they may need to react 
to the problem differently.

Using a parameter to "rewrite" might work. to switch out to a different 
implementation (or parameterised implementation) of the generic traversals. We 
would lose the ability to run strategies directly to get the same effect (or 
add the parameter to every strategy combinator).

Original comment by inkytonik on 30 Sep 2012 at 12:57

[GoogleCodeExporter]

Since there's no good solution to this problem, I decided to go with explicitly 
declaring attributes for my (previously) implicit parameters. So maybe you can 
close this issue with a "wontfix" for now.

One note though, when using custom extractors I ended up with a Scala 2.9.2 
compiler crash: https://gist.github.com/3892008

Original comment by etorrebo...@gmail.com on 16 Oct 2012 at 10:10

[GoogleCodeExporter]

Ok, wontfix for now. Have you tried 2.10.0-RC1 for that crash? Or is your code 
not on 2.10 yet?

Original comment by inkytonik on 16 Oct 2012 at 11:24

• Changed state: WontFix
• Added labels: Milestone-Unknown, Priority-Low
• Removed labels: Milestone-Release1.3, Priority-High

[GoogleCodeExporter]

I tried to, but the code is not ready for 2.10.

Original comment by etorrebo...@gmail.com on 16 Oct 2012 at 11:38