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

Changing type of global variables gives TypeError #4178

Closed
dmac100 opened this Issue Sep 25, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@dmac100

dmac100 commented Sep 25, 2016

Environment

jruby-complete-9.1.5.0.jar
Java 1.8.0_25
Linux x86_64

Expected Behavior

With:

import javax.script.ScriptEngine;
import javax.script.ScriptEngineManager;

public class Main {
    public static void main(String[] args) throws Exception {
        System.setProperty("jruby.backtrace.style", "full");
        ScriptEngine engine = new ScriptEngineManager().getEngineByName("jruby");
        System.out.println(engine.eval("$x = 10"));
        System.out.println(engine.eval("$x = 'a'"));
    }
}

The output should be '10' followed by 'a'.

Actual Behavior

I'm getting '10', followed by:


Exception in thread "main" org.jruby.exceptions.RaiseException: (TypeError) cannot convert instance of class org.jruby.RubyString to class java.lang.Long
    at java.lang.Thread.getStackTrace(java/lang/Thread.java:1552)
    at org.jruby.runtime.backtrace.TraceType$Gather.getBacktraceData(org/jruby/runtime/backtrace/TraceType.java:246)
    at org.jruby.runtime.backtrace.TraceType.getBacktrace(org/jruby/runtime/backtrace/TraceType.java:47)
    at org.jruby.RubyException.prepareBacktrace(org/jruby/RubyException.java:235)
    at org.jruby.exceptions.RaiseException.preRaise(org/jruby/exceptions/RaiseException.java:214)
    at org.jruby.exceptions.RaiseException.preRaise(org/jruby/exceptions/RaiseException.java:181)
    at org.jruby.exceptions.RaiseException.<init>(org/jruby/exceptions/RaiseException.java:111)
    at org.jruby.Ruby.newRaiseException(org/jruby/Ruby.java:4192)
    at org.jruby.Ruby.newTypeError(org/jruby/Ruby.java:3865)
    at org.jruby.RubyBasicObject.defaultToJava(org/jruby/RubyBasicObject.java:872)
    at org.jruby.RubyBasicObject.toJava(org/jruby/RubyBasicObject.java:843)
    at org.jruby.RubyString.toJava(org/jruby/RubyString.java:5541)
    at org.jruby.embed.variable.AbstractVariable.getJavaObject(org/jruby/embed/variable/AbstractVariable.java:140)
    at org.jruby.embed.variable.GlobalVariable.getJavaObject(org/jruby/embed/variable/GlobalVariable.java:46)
    at org.jruby.embed.internal.BiVariableMap.get(org/jruby/embed/internal/BiVariableMap.java:229)
    at org.jruby.embed.internal.BiVariableMap.get(org/jruby/embed/internal/BiVariableMap.java:208)
    at org.jruby.embed.jsr223.Utils.postEval(org/jruby/embed/jsr223/Utils.java:207)
    at org.jruby.embed.jsr223.JRubyEngine.eval(org/jruby/embed/jsr223/JRubyEngine.java:95)
    at org.jruby.embed.jsr223.JRubyEngine.eval(org/jruby/embed/jsr223/JRubyEngine.java:142)
    at Main.main(Main.java:10)
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 25, 2016

Member

Must be caching the last Java type used to coerce the value, or something like that.

Member

headius commented Sep 25, 2016

Must be caching the last Java type used to coerce the value, or something like that.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 26, 2016

Member

Yes, that appears to be it. For whatever reason, when assigning these variables from either Java or Ruby, we cache the first-seen Java type and try to forcibly coerce to that type from then on. This seems wrong to me.

The following patch removes this behavior from both forms of assignment:

diff --git a/core/src/main/java/org/jruby/embed/variable/AbstractVariable.java b/core/src/main/java/org/jruby/embed/variable/AbstractVariable.java
index 809f6f4..ace98e6 100644
--- a/core/src/main/java/org/jruby/embed/variable/AbstractVariable.java
+++ b/core/src/main/java/org/jruby/embed/variable/AbstractVariable.java
@@ -94,22 +94,22 @@ abstract class AbstractVariable implements BiVariable {
         return (RubyObject) receiver.getRuntime().getTopSelf();
     }

     protected void updateByJavaObject(final Ruby runtime, Object... values) {
         assert values != null;
         javaObject = values[0];
         if (javaObject == null) {
             javaType = null;
         } else if (values.length > 1) {
             javaType = (Class) values[1];
-        } else {
-            javaType = javaObject.getClass();
+//        } else {
+//            javaType = javaObject.getClass();
         }
         irubyObject = JavaEmbedUtils.javaToRuby(runtime, javaObject);
         fromRuby = false;
     }

     protected void updateRubyObject(final IRubyObject rubyObject) {
         if ( rubyObject == null ) return;
         this.irubyObject = rubyObject;
         // NOTE: quite weird - but won't pass tests otherwise !?!
         //this.javaObject = null;
@@ -134,23 +134,23 @@ abstract class AbstractVariable implements BiVariable {
     }

     public Object getJavaObject() {
         if (irubyObject == null) return javaObject;

         if (javaType != null) { // Java originated variables
             javaObject = javaType.cast( irubyObject.toJava(javaType) );
         }
         else { // Ruby originated variables
             javaObject = irubyObject.toJava(Object.class);
-            if (javaObject != null) {
-                javaType = javaObject.getClass();
-            }
+//            if (javaObject != null) {
+//                javaType = javaObject.getClass();
+//            }
         }
         return javaObject;
     }

     public void setJavaObject(final Ruby runtime, Object javaObject) {
         updateByJavaObject(runtime, javaObject);
     }

     public IRubyObject getRubyObject() {
         return irubyObject;

And with this patch in place, your original case runs (in Ruby form below):

ENV_JAVA["jruby.backtrace.style"] = "full"
engine = javax.script.ScriptEngineManager.new.getEngineByName("jruby")
puts(engine.eval("$x = 10"))
puts(engine.eval("$x = 'a'"))

@enebo Can you think of any reason why this type caching should be there? I assume it was done for perf reasons, but I don't think there really are any.

Member

headius commented Sep 26, 2016

Yes, that appears to be it. For whatever reason, when assigning these variables from either Java or Ruby, we cache the first-seen Java type and try to forcibly coerce to that type from then on. This seems wrong to me.

The following patch removes this behavior from both forms of assignment:

diff --git a/core/src/main/java/org/jruby/embed/variable/AbstractVariable.java b/core/src/main/java/org/jruby/embed/variable/AbstractVariable.java
index 809f6f4..ace98e6 100644
--- a/core/src/main/java/org/jruby/embed/variable/AbstractVariable.java
+++ b/core/src/main/java/org/jruby/embed/variable/AbstractVariable.java
@@ -94,22 +94,22 @@ abstract class AbstractVariable implements BiVariable {
         return (RubyObject) receiver.getRuntime().getTopSelf();
     }

     protected void updateByJavaObject(final Ruby runtime, Object... values) {
         assert values != null;
         javaObject = values[0];
         if (javaObject == null) {
             javaType = null;
         } else if (values.length > 1) {
             javaType = (Class) values[1];
-        } else {
-            javaType = javaObject.getClass();
+//        } else {
+//            javaType = javaObject.getClass();
         }
         irubyObject = JavaEmbedUtils.javaToRuby(runtime, javaObject);
         fromRuby = false;
     }

     protected void updateRubyObject(final IRubyObject rubyObject) {
         if ( rubyObject == null ) return;
         this.irubyObject = rubyObject;
         // NOTE: quite weird - but won't pass tests otherwise !?!
         //this.javaObject = null;
@@ -134,23 +134,23 @@ abstract class AbstractVariable implements BiVariable {
     }

     public Object getJavaObject() {
         if (irubyObject == null) return javaObject;

         if (javaType != null) { // Java originated variables
             javaObject = javaType.cast( irubyObject.toJava(javaType) );
         }
         else { // Ruby originated variables
             javaObject = irubyObject.toJava(Object.class);
-            if (javaObject != null) {
-                javaType = javaObject.getClass();
-            }
+//            if (javaObject != null) {
+//                javaType = javaObject.getClass();
+//            }
         }
         return javaObject;
     }

     public void setJavaObject(final Ruby runtime, Object javaObject) {
         updateByJavaObject(runtime, javaObject);
     }

     public IRubyObject getRubyObject() {
         return irubyObject;

And with this patch in place, your original case runs (in Ruby form below):

ENV_JAVA["jruby.backtrace.style"] = "full"
engine = javax.script.ScriptEngineManager.new.getEngineByName("jruby")
puts(engine.eval("$x = 10"))
puts(engine.eval("$x = 'a'"))

@enebo Can you think of any reason why this type caching should be there? I assume it was done for perf reasons, but I don't think there really are any.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 27, 2016

Member

I'm going to go ahead with the removal. I can't find anything in git logs as to why this caching of javaType was done. Maybe @yokolet remembers?

Member

headius commented Sep 27, 2016

I'm going to go ahead with the removal. I can't find anything in git logs as to why this caching of javaType was done. Maybe @yokolet remembers?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Sep 27, 2016

Member

@kares You've also touched this code from time to time...I'd be interested in your thoughts.

Member

headius commented Sep 27, 2016

@kares You've also touched this code from time to time...I'd be interested in your thoughts.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Sep 27, 2016

Member

looking good, although I wasn't changing much semantics here.

Member

kares commented Sep 27, 2016

looking good, although I wasn't changing much semantics here.

headius added a commit that referenced this issue Sep 28, 2016

headius added a commit that referenced this issue Sep 28, 2016

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