Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
scabug opened this issue May 10, 2011 · 27 comments
Closed

Comments

@scabug
Copy link

scabug commented May 10, 2011

=== 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
Copy link
Author

scabug commented May 10, 2011

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

@scabug
Copy link
Author

scabug commented May 10, 2011

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

@scabug
Copy link
Author

scabug commented May 11, 2011

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

@scabug
Copy link
Author

scabug commented May 16, 2011

@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
Copy link
Author

scabug commented May 16, 2011

@odersky said:
Fixed by r24968.

@scabug
Copy link
Author

scabug commented May 17, 2011

@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
Copy link
Author

scabug commented Jun 2, 2011

@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
Copy link
Author

scabug commented Jun 3, 2011

@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
Copy link
Author

scabug commented Jul 13, 2011

@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
Copy link
Author

scabug commented Aug 9, 2011

@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
Copy link
Author

scabug commented Aug 9, 2011

@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
Copy link
Author

scabug commented Mar 19, 2012

@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
Copy link
Author

scabug commented Jul 21, 2012

@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
Copy link
Author

scabug commented Jul 22, 2012

@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
Copy link
Author

scabug commented Jul 22, 2012

@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:

  • scala.xml.parsing.MarkupParser
  • scala.actors.scheduler.TerminationService
  • scala.reflect.api.Symbols$SymbolApi

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
Copy link
Author

scabug commented Jul 22, 2012

@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
Copy link
Author

scabug commented Jul 22, 2012

@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
Copy link
Author

scabug commented Jul 22, 2012

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

@scabug
Copy link
Author

scabug commented Jul 22, 2012

@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
Copy link
Author

scabug commented Jul 22, 2012

@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
Copy link
Author

scabug commented Jul 22, 2012

@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
Copy link
Author

scabug commented Jul 22, 2012

@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
Copy link
Author

scabug commented Jul 23, 2012

@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:

  • In 2.8.0.Beta1-RC5 (that's the release before the tentative fix) none of the tests failed
  • In 2.9.0.RC1 still none of the tests failed
  • Between RC1 and 2.9.0.RC2 the three cases reported here originally started to fail
  • With the two workarounds (r24922 and r24968, which I figured are probably 124cf3f9c and 7127d829) only one test is failing which is the one where the self-type is the singleton type
  • With my proposed "fix" which reverts cb4fd6582, 124cf3f9c, and 7127d829, only the test from the previous comment fails (which is a scenario not observed before)

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
Copy link
Author

scabug commented Jul 23, 2012

@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
Copy link
Author

scabug commented Jul 25, 2012

@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
Copy link
Author

scabug commented Jul 25, 2012

@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
Copy link
Author

scabug commented Jul 25, 2012

@paulp said:
scala/scala#988

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants