search

scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

2.9.0+: Runtime exception with structural-type reflection cache and self-types #4560

Closed scabug closed 12 years ago

scabug commented 13 years ago

=== What steps will reproduce the problem? ===

object Pimper {
 implicit def pimp(i: Int) = new {
    def test: String = i.toString
  }
}

class A

trait B {
  self: A =>

  def test {
    import Pimper.pimp

    println(5.test)
  }
}

object Test extends A with B {
  def main(args: Array[String]) {
    test
  }
}

=== What is the expected behavior? === 5

=== What do you see instead? ===

A runtime error:

java.lang.NoSuchFieldError: reflPoly$$Cache1
    at T1$$class.reflMethod$$Method1(Test.scala:16)
    at T1$$class.test(Test.scala:16)
    at Test$$.test(Test.scala:20)
    at Test$$.main(Test.scala:22)
    at Test.main(Test.scala)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at sbt.Run.invokeMain(Run.scala:69)
    at sbt.Run.run0(Run.scala:60)
    at sbt.Run.execute$$1(Run.scala:48)
    at sbt.Run$$$$anonfun$$run$$2.apply(Run.scala:51)
    at sbt.Run$$$$anonfun$$run$$2.apply(Run.scala:51)
    at sbt.TrapExit$$.executeMain$$1(TrapExit.scala:33)
    at sbt.TrapExit$$$$anon$$1.run(TrapExit.scala:42)

=== Additional information ===

This happens since 2.9.0.RC2 (RC1 was fine) and happens only with the self-type in place. The problem seems to be that the reflection cache inside of B$$class looks for the cache field inside of class B:

public static java.lang.reflect.Method reflMethod$$Method1(java.lang.Class);
  Code:
   Stack=5, Locals=2, Args_size=1
   0:   getstatic       SI-33; //Field B.reflPoly$$Cache1:Ljava/lang/ref/SoftReference;

In another [http://groups.google.com/group/spray-user/msg/59eb57b6cb2ce512 case] for some reason the cache was looked for in a class declared as the self-type.

scabug commented 13 years ago

Imported From: https://issues.scala-lang.org/browse/SI-4560?orig=1 Reporter: @jrudolph

scabug commented 13 years ago

@paulp said: This is due to r24762, which was intended to fix #4283.

scabug commented 13 years ago

@odersky said: (In r24922) Closes #4560. Review by dragos.

scabug commented 13 years ago

@dragos said: Still fails if you change the self type to be a class instead of a trait. I changed the description to reflect this.

scabug commented 13 years ago

@odersky said: Fixed by r24968.

scabug commented 13 years ago

@jrudolph said: We've got the same problem now with 2.9.0 in a case where only traits are involved but with multiple a self-type like this:

trait A
trait B
trait C { self: A with B =>

}

My brief tests with the Scala 2.10.0-SNAPSHOT from 17.5. shows that this case seems to be fixed with the last commit as well but maybe you want to add another test-case for this case anyways.

scabug commented 13 years ago

@propensive said (edited on Jun 2, 2011 11:59:21 PM UTC): This appears to induce the same problem, tested on 2.9.0.1 and 2.10 nightly:

object C { implicit def y(s : String) = new { def x(implicit x : List[Int] = Nil) = "" } }
import C._
object A extends B { def main(args : Array[String]) : Unit = fail() }
trait B { this : A.type => def fail() = "".x }
scabug commented 13 years ago

@paulp said: Here is enough for a nice runtime failure in 2.9.0 and beyond.

trait B {
  this: A.type =>

  def y = new { def f() = () }
  def fail() = y.f()
}
object A extends B {
  def main(args: Array[String]): Unit = fail()
}
// java.lang.NoSuchFieldError: reflPoly$Cache1
//  at B$class.reflMethod$Method1(0602.scala:5)
scabug commented 13 years ago

@retronym said (edited on Jul 13, 2011 11:36:27 AM UTC): I can't directly vote for this as it is marked as resolved; this comment is my proxy.

scabug commented 13 years ago

@odersky said: I stepped in with some quick fixes when we released 2.9.0, but I do not have the bandwidth to take on structural reflection bugs. Maybe Gilles, who wrote that code can have a look? Since he is not in the list of possible assignees I mark as unassigned instead.

scabug commented 13 years ago

@jrudolph said: Just a quick reminder that this is not only a structural reflection bug but also symbols don't work in those classes. I filed this issue under #4601 and it was resolved along with the previous fix for this one but it still fails for the additional situations documented in this bug.

scabug commented 12 years ago

@jrudolph said: I tracked the problem down to this commit:

cb4fd65825f3c88908103e48d0d7e89d70d26c22

which "tentatively" replaces the this.type of implementation classes of traits with the declared self type of the trait. This may or may not have any other consequences aside from this issue. Reverting the ThisType.apply part of the commit and the former workarounds for this issue seems to fix the issue. I can't comment on the original purpose of the commit and if the "lift build problem" is still an issue.

scabug commented 12 years ago

@odersky said: Johannes, could you revert the commit and workarounds as you suggested? I see no other way than to do this and then ask the lift people whether they still have an issue.

scabug commented 12 years ago

@jrudolph said: Doing the changes ([https://github.com/scala/scala/pull/970]) some tests start failing with NoSuchMethodErrors:

https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/645/consoleFull

scabug commented 12 years ago

@jrudolph said: All the three cases from the failing tests are probably not specific to the tests failing but due to a broken library.

There seem to be three cases:

All of those classes again have self-types. So what we have here could mean:

a) the original fix ("tentative fix for RC5 lift build problem.") actually fixed a related problem to this ticket but created a regression for the cases listed here b) the workarounds which were created to fix this ticket are masking another problem which was introduced after the workarounds c) when reverting I resolved conflicts wrong d) something else

scabug commented 12 years ago

@paulp said:

Example error:

java.lang.NoSuchMethodError: scala.xml.parsing.MarkupHandler.xToken(C)V
  at scala.xml.parsing.MarkupParser$class.element(MarkupParser.scala:543)

That method is defined in trait MarkupParserCommon, which MarkupParser extends. It may be relevant that MarkupParserCommon is private[scala]. The MarkupHandler target must be taken from MarkupParser's self type, which is

  self: MarkupParser with MarkupHandler =>

But MarkupHandler by itself does not define any xToken method.

In the current working version, the generated call is

         6: invokeinterface #183,  2          // InterfaceMethod scala/xml/parsing/MarkupParserCommon.xToken:(C)V
scabug commented 12 years ago

@paulp said: "a) the original fix ("tentative fix for RC5 lift build problem.") actually fixed a related problem to this ticket but created a regression for the cases listed here"

I think it's this one.

scabug commented 12 years ago

@jrudolph said: Yep, seems so, redoing the original commit seems to fix those issues from the pull request build.

scabug commented 12 years ago

@paulp said: Actually, this one:

  [partest] java.lang.NoSuchMethodError: scala.reflect.api.Symbols$SymbolApi.scala$reflect$api$Symbols$TermSymbolApi$$$outer()Lscala/reflect/api/Symbols;

Appears to me likely to also depend on martin's recent elimination of redundant $outer fields.

scabug commented 12 years ago

@paulp said: This one is especially revealing - it's looking for a method on java.lang.Thread, that's pretty far off.

  [partest] java.lang.NoSuchMethodError: java.lang.Thread.CHECK_FREQ()I
 scala.actors.scheduler.TerminationService$class.liftedTree1$1(TerminationService.scala:42)

private[scheduler] trait TerminationService extends TerminationMonitor {
  _: Thread with IScheduler =>

  protected val CHECK_FREQ = 50

What I see is: if a class appears in the self type (possibly there are more conditions) then it acts like everything is in that class. But if you have

trait Foo extends Bar { self: Baz with Quux =>

... }

Then a member might be in the implementation class of Foo, Bar, or anything inherited from Bar, or in Baz, Quux, or anything inherited through them, or the implementation classes of any of those.

scabug commented 12 years ago

@jrudolph said: Here's a minification of the TerminationService case (failing in the pull request):

object Outer {
  class Tester
  private[Outer] trait B4 { _: Tester =>
    protected val FREQ = 23
    def fail() = {
      println(FREQ)
    }
  }
  object C4 extends Tester with B4
}

Note the necessity of private[Outer].

This is a case which still worked in 2.9.0.RC1. So there might be another possibility: The original issue was something else but the original fix then started to mask issues.

scabug commented 12 years ago

@jrudolph said: The MarkdownHandler case can be minified to this one:

object Outer2 {
  abstract class A5
  private[Outer2] trait C5 {
    def impl() { println("SUCCESS") }
  }
  trait B5 extends C5 { self: A5 =>
    def fail() { impl() }
  }
  object Test5 extends A5 with B5 with C5
}

So again a class is the self-type and the implementing trait is private.

scabug commented 12 years ago

@jrudolph said: Here's a summary:

we've got now 4 more or less different failure scenarios: the original three in this report (which are in the test file in the pull request) plus the one from the previous comment (I'm putting both previous ones into one category). Archaeological research showed the following:

My previous explanation that the tentative fix was the original commit introducing the regression is definitely wrong simply because it was already in the codebase long before the regression (actually I can't figure out any more from where I dug this commit out). It is a curious thing that the commit still is related otherwise reverting wouldn't have such an effect.

I'm not so sure where to go from here: I wouldn't say the best solution is reverting a bunch of stuff if the outcome is at least one new previously unknown regression. In contrast, it isn't probably too hard to extend the current workaround to handle the last known remaining issue (self-type is singleton-type). The price is that we still have a workaround in the codebase for an issue for that no one seems to understand exactly where it comes from and which other variations may exist.

Maybe I'll start a bisect between 2.9.0.RC1 and 2.9.0.RC2 to find out where it really showed up first.

(In the meantime while scalac is compiling I read one of Alan Turing's articles, here's an excerpt about the "Learning Machine":

bq.An important feature of a learning machine is that its teacher will often be very largely ignorant of quite what is going on inside, although he may still be able to some extent to predict his pupil's behavior. This should apply most strongly to the later education of a machine arising from a child machine of well-tried design (or programme). This is in clear contrast with normal procedure when using a machine to do computations one's object is then to have a clear mental picture of the state of the machine at each moment in the computation. This object can only be achieved with a struggle. The view that "the machine can only do what we know how to order it to do,"' appears strange in face of this. Most of the programmes which we can put into the machine will result in its doing something that we cannot make sense.

Sometimes I worry we are getting more and more into the area of trying to educate the compiler what to do instead of having "a clear mental picture of the state of the machine at each moment" but maybe that's just my ignorance)

scabug commented 12 years ago

@jrudolph said: Seems like 8707c9ec introduced the regression between 2.9.0.RC1 and 2.9.0.RC2 (which btw I could only accurately identify because I had the old git repository lying around, in the new one it would have been this commit)

I think the solution would then be to fix it properly at the place where the error was introduced (val hostClass = qualifier.tpe.typeSymbol.orElse(sym.owner) seems to naive to take self-types properly into account) and then try reverting the former two workaround commits.

scabug commented 12 years ago

@paulp said: After a few minutes of "educating the compiler" your test case outputs this for me:

'Test
Success 1
'Test
Success 2
'Test
Success 3

Since you've been so helpful in this ticket already, if there are more cases not well covered by that one test it would be awesome if you could save me dredging around for more. I think I have this nailed but I'd feel a lot better with more tests.

scabug commented 12 years ago

@paulp said: I also cobbled together this one


object Outer {
  class Tester
  private[Outer] trait B4 { _: Tester =>
    protected val FREQ = 23
    def fail() = {
      println(FREQ)
    }
  }
  object C4 extends Tester with B4
}

object Outer2 {
  abstract class A5
  private[Outer2] trait C5 {
    def impl() { println("SUCCESS") }
  }
  trait B5 extends C5 { self: A5 =>
    def fail() { impl() }
  }
  object Test5 extends A5 with B5 with C5
}

object Test {
  def main(args: Array[String]): Unit = {
    Outer.C4.fail()
    Outer2.Test5.fail()
  }
}

Which prints 23 and SUCCESS as expected.

scabug commented 12 years ago

@paulp said: https://github.com/scala/scala/pull/988