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

ToolBox doesn't respect relative imports #6393

Open
scabug opened this Issue Sep 18, 2012 · 14 comments

Comments

Projects
None yet
3 participants
@scabug
Copy link

scabug commented Sep 18, 2012

import scala.reflect.runtime.universe._
import scala.reflect.runtime.{currentMirror => cm}
import scala.tools.reflect.ToolBox

object Test extends App {
  val tb = cm.mkToolBox()
  val expr = tb.parse("math.sqrt(4.0)")
  println(tb.eval(expr))
}
C:\Projects\Kepler\sandbox @ ticket/5943>myke run .
scala.tools.reflect.ToolBoxError: reflective compilation has failed:

object sqrt is not a member of package math
        at scala.tools.reflect.ToolBoxFactory$ToolBoxImpl$ToolBoxGlobal.throwIfErrors(ToolBoxFactory.scala:294)
        at scala.tools.reflect.ToolBoxFactory$ToolBoxImpl$ToolBoxGlobal.compile(ToolBoxFactory.scala:227)
        at scala.tools.reflect.ToolBoxFactory$ToolBoxImpl.compile(ToolBoxFactory.scala:391)
        at scala.tools.reflect.ToolBoxFactory$ToolBoxImpl.eval(ToolBoxFactory.scala:394)
        at Test$delayedInit$body.apply(t5943b2.scala:8)
        at scala.Function0$class.apply$mcV$sp(Function0.scala:40)
        at scala.runtime.AbstractFunction0.apply$mcV$sp(AbstractFunction0.scala:12)
        at scala.App$$anonfun$main$1.apply(App.scala:61)
        at scala.App$$anonfun$main$1.apply(App.scala:61)
        at scala.collection.immutable.List.foreach(List.scala:309)
        at scala.collection.generic.TraversableForwarder$class.foreach(TraversableForwarder.scala:32)
        at scala.collection.mutable.ListBuffer.foreach(ListBuffer.scala:45)
        at scala.App$class.main(App.scala:61)
        at Test$.main(t5943b2.scala:5)
        at Test.main(t5943b2.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 scala.tools.nsc.util.ScalaClassLoader$$anonfun$run$1.apply(ScalaClassLoader.scala:71)
        at scala.tools.nsc.util.ScalaClassLoader$class.asContext(ScalaClassLoader.scala:31)
        at scala.tools.nsc.util.ScalaClassLoader$URLClassLoader.asContext(ScalaClassLoader.scala:139)
        at scala.tools.nsc.util.ScalaClassLoader$class.run(ScalaClassLoader.scala:71)
        at scala.tools.nsc.util.ScalaClassLoader$URLClassLoader.run(ScalaClassLoader.scala:139)
        at scala.tools.nsc.CommonRunner$class.run(ObjectRunner.scala:28)
        at scala.tools.nsc.ObjectRunner$.run(ObjectRunner.scala:45)
        at scala.tools.nsc.CommonRunner$class.runAndCatch(ObjectRunner.scala:35)
        at scala.tools.nsc.ObjectRunner$.runAndCatch(ObjectRunner.scala:45)
        at scala.tools.nsc.MainGenericRunner.runTarget$1(MainGenericRunner.scala:74)
        at scala.tools.nsc.MainGenericRunner.process(MainGenericRunner.scala:96)
        at scala.tools.nsc.MainGenericRunner$.main(MainGenericRunner.scala:105)
        at scala.tools.nsc.MainGenericRunner.main(MainGenericRunner.scala)
@scabug

This comment has been minimized.

Copy link

scabug commented Sep 18, 2012

Imported From: https://issues.scala-lang.org/browse/SI-6393?orig=1
Reporter: @xeno-by
Affected Versions: 2.10.0, 2.11.0
Duplicates #9096

@scabug

This comment has been minimized.

Copy link

scabug commented Sep 19, 2012

@xeno-by said:
It looks like we're doomed w.r.t this one.

The problem is that Scala reflection and reflective compiler (which is underlying toolboxes) use a different model of classfile loading than vanilla scalac does. Vanilla compiler has its classpath as a list of directories/jars on the filesystem, so it can exhaustively enumerate the packages on the classpath. Reflective compiler works with arbitrary classloaders, and classloaders don't have a concept of enumerating packages.

As a result, when a reflective compiler sees "math" having "import scala.; import java.lang." imports in the lexical context, it doesn't know whether that "math" stands for root.math, scala.math or java.lang.math. So it has to speculate and provisionally creates a package for root.math, which ends up being a wrong choice.

We could probably support a notion of "overloaded" packages, so that the compiler doesn't have to speculate and can store all the possible options, but that'd require redesign of reflection and probably of the typer as well.

@scabug

This comment has been minimized.

Copy link

scabug commented Sep 24, 2012

@gkossakowski said:
Ok, shall we make this major and include in 2.10.0's list of "known issues"?

@scabug

This comment has been minimized.

Copy link

scabug commented Sep 24, 2012

@xeno-by said:
It seems so. I hoped someone will magically save the day before the release, but well...

@scabug

This comment has been minimized.

Copy link

scabug commented Sep 25, 2012

@paulp said:
"We could probably support a notion of "overloaded" packages, so that the compiler doesn't have to speculate and can store all the possible options, but that'd require redesign of reflection and probably of the typer as well."

This is important for other reasons, independent of reflection. #6039. I have gone to a lot of trouble to work around it only a small ways, because I am plagued by compilation errors whenever I try to recompile a subset of compiler sources.

scala/scala@55b6094
scala/scala@ea0d891

But that was only the beginning, and deterioration began immediately.

Yet another reason it is important is because scanning the entire classpath up front is a very expensive piece of startup time.

@scabug

This comment has been minimized.

Copy link

scabug commented Nov 28, 2012

@paulp said:
By request, a link to an old branch of mine with a rewrite of classpaths. https://github.com/paulp/scala/tree/topic/classpath

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 15, 2013

@xeno-by said:
Can we provide special treatment for URLClassLoaders?

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 15, 2013

@gkossakowski said:
Eugene, care to elaborate?

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 15, 2013

@retronym said:
That was my suggestion from our meeting today. I don't think we should consider other ToolBox bugs without having a clear plan for this one, otherwise ToolBoxes are not really useful.

Rather than deal with the lowest common denominator (the ClassLoader API), we could try to do a better job for the more powerful, and very common, URLClassLoader, which would let use get at the underlying folders and JAR files to enumerate the classpath, as is done in the compiler proper.

This might be an opt-in feature, or even a feature implemented in an external library, that takes advantage of extra hooks we expose.

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 15, 2013

@xeno-by said (edited on Jan 15, 2013 6:49:53 PM UTC):
When Jason and I were triaging reflection bugs today, we came to the conclusion that this one is a blocker for using toolboxes seriously, e.g. as a replacement for scala.tools.nsc.interactive. Well, it indeed is, because relative imports are very common in Scala programs.

As to the comment itself, a possible solution would be to distinguish URLClassLoaders, which can exhaustively enumerate their contents, and treat them specially in the reflective compiler. That would bring conceptual feature parity to toolboxes w.r.t the regular compiler in 99% of the use cases.

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 15, 2013

@gkossakowski said:
Will that work with sbt, ide and Maven?

Let's start with sbt and IDE.

I don't know how sbt is handling classloaders so I added Mark to watchers and let him comment on that. Same for Iulian.

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 15, 2013

@retronym said:
The IDE doesn't call ToolBox compilers. SBT already has a means to communicate the classpath to the embedded interpreter, see "Use the Scala REPL from project code¶ http://www.scala-sbt.org/release/docs/Howto/scala.html.

But it is likely that some environments (maybe OSGi) will be fundamentally incompatible with classpath enumeration, and will not support ToolBox compilation. We might have to live with that (or provide hooks for someone else to build a bridge). But we shouldn't punish the mainstream.

I believe that Paul's branch a few comments above has a little code for URLClassLoader enumeration.

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 15, 2013

@gkossakowski said:
The IDE might want to call it for features like conditional breakpoints where you want to compile a little piece of code, no?

If we are designing API we should think about all future clients not only current ones.

I agree that we should provide connivence for common case but we need to ensure that we have a way to support more exotic environments. In order to assure that, could we start with designing an API that allows one to pass classpath enumerator to Reflection API, try that with {{URLClassLoader}}-based enumerator and only then make it a default choice?

@scabug

This comment has been minimized.

Copy link

scabug commented Jan 15, 2013

@retronym said:
Yeah, that's what I meant by "provide hooks". We would just provide a standard implementation of that hook for URLClassLoader, SBT.

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