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.

Sign up for GitHub

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

ScriptEngine API Fixes #231

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

ScriptEngine API Fixes #231

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f17166b
Issue #201: Wrap EvalException in ScriptException
peter-gergely-horvath Jul 4, 2016
5020536
Ensure that scriptEngine.getBindings() and scriptEngine.getContext().…
akbertram Aug 12, 2016
44cf5f1
adjust the tests and let them pass
Alain-Bearez Dec 6, 2017
d54f359
Merge pull request #367 from FOR-Scala/script-engine-fixes
akbertram Dec 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion script-engine/src/main/java/org/renjin/script/RenjinBindings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,13 @@ public boolean containsKey(Object key) {

@Override
public Object get(Object key) {
return frame.getVariable(toSymbol(key));
Symbol symbol = toSymbol(key);
SEXP value = frame.getVariable(symbol);
if(value == Symbol.UNBOUND_VALUE) {
return null;
} else {
return value;
}
}

private Symbol toSymbol(Object key) {
Expand Down
21 changes: 15 additions & 6 deletions script-engine/src/main/java/org/renjin/script/RenjinScriptContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@

public class RenjinScriptContext implements ScriptContext {

private Context context;
private Map<String,Object> attributes = new TreeMap<>();

private final Context context;
private final Map<String,Object> attributes = new TreeMap<>();
private final RenjinBindings engineBindings;

RenjinScriptContext(Context context) {
this.context = context;
this.engineBindings = new RenjinBindings(context.getEnvironment().getFrame());
}

public Context getContext() {
Expand All @@ -42,9 +44,16 @@ public int getAttributesScope(String arg0) {
}

@Override
public Bindings getBindings(int arg0) {
// TODO Auto-generated method stub
return null;
public Bindings getBindings(int scope) {
switch(scope) {
case ScriptContext.ENGINE_SCOPE:
return engineBindings;

default:
case ScriptContext.GLOBAL_SCOPE:
throw new UnsupportedOperationException();

}
}

@Override
Expand Down
21 changes: 7 additions & 14 deletions script-engine/src/main/java/org/renjin/script/RenjinScriptEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public class RenjinScriptEngine implements ScriptEngine, Invocable {
private final Context topLevelContext;

// jsr context, which wrap renjincore context.
private final ScriptContext scriptContext;
private final RenjinScriptContext scriptContext;

RenjinScriptEngine(RenjinScriptEngineFactory factory, Session session) {
super();
Expand Down Expand Up @@ -55,15 +55,7 @@ public Object get(String key) {

@Override
public Bindings getBindings(int scope) {
switch(scope) {
case ScriptContext.ENGINE_SCOPE:
return new RenjinBindings(topLevelContext.getEnvironment().getFrame());

default:
case ScriptContext.GLOBAL_SCOPE:
throw new UnsupportedOperationException();

}
return scriptContext.getBindings(scope);
}

@Override
Expand Down Expand Up @@ -145,14 +137,15 @@ private Object eval(Reader reader, Context context, String filename) throws Scri
return eval(context, source);
}

private Object eval(Context context, SEXP source) {
private Object eval(Context context, SEXP source) throws ScriptException {
try {
return context.evaluate( source, context.getEnvironment());
} catch(BreakException e) {
throw new EvalException("no loop for break");
throw new ScriptException("no loop for break");
} catch(NextException e) {
throw new EvalException("no loop for next");

throw new ScriptException("no loop for next");
} catch (EvalException e) {
throw new ScriptException(e);
}
}

Expand Down
31 changes: 30 additions & 1 deletion script-engine/src/test/java/org/renjin/script/RenjinScriptEngineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.renjin.script;

import org.hamcrest.CoreMatchers;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.renjin.sexp.*;
Expand All @@ -10,6 +11,7 @@

import static org.hamcrest.CoreMatchers.equalTo;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;


public class RenjinScriptEngineTest {
Expand Down Expand Up @@ -136,5 +138,32 @@ public void readResource() throws ScriptException {
assertThat(vector.length(), equalTo(1));
assertThat(vector.getElementAsString(0), equalTo("hello world"));
}



@Test
public void sameBindingsFromEngineAndContext() throws ScriptException {

ScriptEngineManager scriptEngineManager = new ScriptEngineManager();
ScriptEngine scriptEngine = scriptEngineManager.getEngineByName("Renjin");
if (scriptEngine == null) {
throw new IllegalStateException("Could not load ScriptEngine: Renjin");
}

Bindings bindingsFromEngine = scriptEngine.getBindings(ScriptContext.ENGINE_SCOPE);

Bindings bindingsFromContext = scriptEngine.getContext().getBindings(ScriptContext.ENGINE_SCOPE);

assertTrue(bindingsFromEngine == bindingsFromContext); // identity test

final String key = "foo";
final String value = "bar";

Assert.assertNull(bindingsFromEngine.get(key));
Assert.assertNull(bindingsFromContext.get(key));

bindingsFromEngine.put(key, value);
Assert.assertEquals(bindingsFromEngine.get(key), value);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peter-gergely-horvath does the API require that value from get(key) is equal to the original value set?

Renjin's implementation automatically converts scalars like instances of java.lang.String or `java.lang.Number' to an R object (org.renjin.sexp.SEXP), so what you put in does not always equal what you get out.

Copy link
Contributor

@Alain-Bearez Alain-Bearez Nov 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the Javadoc:

Retrieves a value set in the state of this engine. The value might be one which was set using setValue or some other value in the state of the ScriptEngine, depending on the implementation.

https://docs.oracle.com/javase/8/docs/api/javax/script/ScriptEngine.html#get-java.lang.String-

This makes sense if you consider as example an implementation for the JavaScript language: while you put a string value for a key in your bindings, the execution of some script might result in the variable to get an array value.

As a consequence, the assertions on lines 165 and 167 have to be rewritten considering the fact that Renjin wraps the given java.lang.String value into an expression of type StringArrayVector.

    Assert.assertEquals(StringVector.valueOf(value), bindingsFromEngine.get(key));

    Assert.assertEquals(StringVector.valueOf(value), bindingsFromContext.get(key));


Assert.assertEquals(bindingsFromContext.get(key), value);
}
}