Conversation
|
Maybe a useful separation of concerns. |
There was a problem hiding this comment.
what is the reason for having this commented line? is this method of getting the script engine tested somewhere else?
|
This LGTM otherwise, but I'm not really familiar enough with JSR223 |
|
I think we should get this one in. It needs rebasing, though. |
|
I'll have to get familiar enough with JSR223. |
99ee333 to
8876c1d
Compare
|
Added |
|
The following problem manifests only when using sbt as explained; it works when building locally and running Take this and run |
|
(the scala library is not on the compilation classpath, i'm trying to figure out why) |
|
I guess it's that Anyway, this problem seems unrelated to the changes in this PR. |
|
I'm still lacking JSR223 familiarization, but to me this PR looks very good. It could maybe get a little more testing. Here's some log of my playground |
|
@rjolly would you be interested to also review this PR? |
| //intp quietBind ("engine" -> this.asInstanceOf[ScriptEngine]) | ||
| //intp quietBind NamedParam[DynamicContext]("$ctx", dynamicContext) | ||
| intp quietBind NamedParamClass(intp.ctx, "scala.tools.nsc.interpreter.Scripted#DynamicContext", dynamicContext) | ||
| intp quietBind NamedParam[IMain]("$intp", intp)(StdReplTags.tagOfIMain, classTag[IMain]) |
There was a problem hiding this comment.
Regarding this line, what I intended was to make the interpreter available as a ScriptEngine, that is, it should be "this" and not "intp" (and the name was "engine", without $, it would be cool if it could remain like this). Otherwise, LGTM.
|
This is proof-of-concept; I don't know what the use cases are, for interoperating with Objects; and I haven't exercised it much, for the usual reasons of ignorance and laziness, and not knowing if it's useful to pursue. Suggestions welcome. @rjolly Am I right that you can usually throw weird names in backticks? The REPL isn't currently as backtick-friendly as it ought to be. |
|
@som-snytt I tested it, and it seems my two concerns above are solved on the Java side. For one thing, jrunscript does not bind long names like javax.script.filename and javax.script.argv anymore. Now it binds an "arguments" variable, and also "engine" with the ScriptEngine, which solves my first issue. |
|
There is one more caveat, still related to binding variables. With your new scheme, one needs to jarlist not only scala-library.jar, but also scala-compiler.jar and scala-reflect.jar too (as the former depends on it). If not done, one gets (using 2.12.0-8876c1d-SNAPSHOT): The problem is caused by these lines: , which go through the following statement: , causing the errors. Formerly the problem was avoided by casting the interpreter to javax.script.ScriptEngine, which is known outside of scala-compiler.jar . To solve it in the present construct, we could define an interface for DynamicContext , which we would put in scala-library.jar . And again regarding IMain, we should not bind it, but instead "this" (the Scripted instance) as instance of ScriptEngine (or nothing at all, as jrunscript is taking care of it, as seen above). |
|
I'm trying to resurrect a year-old branch of our project to see if I can get this to work for us. Our use case is a bit twisty; We have a Play Framework 2.3 application that embeds a Java library that supports JSR-223 script engines...and I need to have the automated tests operate under
I'm also kind of stumbling around as to how I reference stuff in the script context. Are my comments about my floundering best made here, or back on SI-7916? @lukas Rytz invited me to pop in here and contribute, and kindly provided a 2.12 build with this on it. |
|
Thanks @rjolly and @MaggieLeber , I'll make time to push it forward. This is the right place to comment on this implementation. I guess they don't require a SIP or SLIP, or a RIP (repl improvement). I kind of which there were a RIP. |
|
I don't have this working yet in my use case. I think I need to do a cut-down exemplar without Play 2.3 and all the myriad dependencies of our app, most of whom don't to yet have 2.12 binaries available. I'm still unclear how I refer to values in the passed My use case looks like this:
|
|
OK, gave up on sorting that mess; it's too complicated. So now I'm trying to run Scala scripts in jMeter as JSR223 samplers. jMeter finds the
Although if you sneak up on it you can catch it:
Update: Progress! Update: it works if you restore the erased type info!!!:
|
|
@MaggieLeber Yes, currently all you get is an Previously, it took apart the bound name as a One could hijack the API with the colon trick, or add a method that includes type info. |
|
Unfortunately, I think any approach that erases type information in the objects in a ScriptContext is in for a very rough ride from folks approaching Scala as a JSR223 implemention. It's common for a JSR223 host to chain script executions, sharing their contexts between executions which may occur under different ScriptEngines. Activiti and jMeter are good examples. That said, I suppose it's better than not working at all. |
isn't this an inherent issue with JSR223 itself, rather than a shortcoming of our implementation in particular? (or am I confused?) |
|
One option is to wrap bound objects in |
|
I wanted to spend a couple of hours on this but hit a snag. Testing with a bad ref, it throws as shown: https://gist.github.com/som-snytt/361d43d2c78e31364cc6d558e22f0638 I'll push and maybe @lrytz will hint at something. For @rjolly hint, it pre-compiles a wrapper for the current context and also the dynamic adapter (which isn't strictly required). |
8876c1d to
d232d38
Compare
|
I'll take a look if I can reproduce - |
|
I just noticed that adding the I'm distracted b/c heading to my day job and didn't intend to spend hours on this. But it makes me appreciate what FOSS means as a commitment. Actually I wanted to finish my dotty repl last weekend. :( |
right, it's actually the main loop that moves compiler phases forward: https://github.com/scala/scala/blob/2.12.x/src/compiler/scala/tools/nsc/Global.scala#L1480
your commitment is quite unique! feel free to put your efforts where your motivation drags you. but this one will be worth pushing through! |
|
So a compile error will throw: I always doubt if the position is useful... |
|
I encountered the wontfix where |
|
ah, you mean #4663? maybe when you're on this one next time, a rebase on current 2.12.x would be useful and unblock CI validation (assuming we manage to unblock CI itself – https://gitter.im/scala/contributors?at=5715ffbd4c2125fc3f03a200) |
|
This was already rebased, but I haven't looked at squashing yet. In case I wanted to back something out; but probably I'll just move forward. The other desirable was to handle bound objects more dynamically. I think implicit conversion to Dynamic is not supported? That would have made falling back to reflective access easy. |
3f2c039 to
c6309fc
Compare
|
Is this that old problem where spiders would poke an abort link? |
|
/rebuild |
|
If my assessment of this long thread is accurate, everyone's requests have been satisfied and all that remains is resolving the merge conflicts that have crept in. Are there any objections against merging the current state? |
|
yes, i think so too |
|
I'll rebase. I think there is a future opportunity for improving the user experience of working with dynamically bound objects, but not a blocker for this PR. |
Refactor the ScriptEngine support to an adaptor atop the IMain API. Allow references to resolve to context attributes. (The attributes must be defined at compilation time, though they may resolve to updated values at evaluation time.) This means that attributes are not bound statically in REPL history. In particular, we forgo the trick of binding attributes named "name: Type" as typed values. Instead, an `x` bound in dynamic context is injected into the script as a dynamic selection `$ctx.x` where `ctx` performs the look-up in the script context. When a compiled script is re-evaluated, a new instance of the script class is created and defined symbols are rebound. The context stdout writer is handled with `Console.withOut`, with bytes decoded using the default charset. Compilation errors are thrown as ScriptException with the first reported error. This commit doesn't attempt dynamic selection from objects in context. Currently, script must cast.
c6309fc to
3cddeaa
Compare
|
No objection. |
|
/rebuild |
Fix reference to script engine factory in META-INF/services Follow up for #4819
Refactor the ScriptEngine support to an adaptor atop the
IMain API.
Allow references to resolve to context attributes. (The
attributes must be defined at compilation time, though
they may resolve to updated values at evaluation time.)
This means that attributes are not bound statically in
REPL history. In particular, we forgo the trick of binding
attributes named "name: Type" as typed values.
Instead, an
xbound in dynamic context is injected intothe script as a dynamic selection
$ctx.xwherectxperforms the look-up in the script context.
When a compiled script is re-evaluated, a new instance of
the script class is created and defined symbols are
rebound.