Commit 72a5d761 authored by Mark's avatar Mark

I've looked into this issue at length now and I think I have something worth

discussing and potentially trying[0] . The root issue here lies in how ABCL
converts its CL representation of integers to Java, as well as its conversion
of Java primitives to other Java primitives.

ABCL's common superclass for all objects (LispObject) includes two methods to
convert the lisp object to a Java representation: LispObject.javaInstance() and
LispObject.javaInstance(Class). LispObject.javaInstance(Class) presumably tries
to convert the internal representation to the type represented by the Class
passed in. In practice, this seems to be primarily used for Java primitive
types. The current implementation of these methods in ABCL's internal Bignum,
DoubleFloat, SingleFloat, Fixnum, LispCharacter, and JavaObject, do not seem to
have all the required permutations necessary to support all of the possible Java
primitive converting operations.

Even for instances where a conversion /is/ implemented (such as
Fixnum.javaInstance(Short) where a Short would be returned), the method lookup
strategy in Java.findMethod (which boils down to what Java.isAssignable does)
doesn't find the method with the Short parameter. Instead it uses normal rules
in the Java Language spec and only looks for methods that potentially /widen/
primitives, and disallows any narrowing without an explicit narrowing cast (and
in Brad's example, the literal 16 or 32 is represented by Fixnum, which converts
to java.lang.Integer). I think the original intention is to have such methods be
called using jcoerce, such as:

(jcoerce 32 (jclass "short"))
or
(jcoerce 32 (jclass "java.lang.Short"))

This fails in the current implementation, I believe due to a bug in
JavaObject(Object, Class) constructor. This only does one type of conversion, to
java.lang.Byte, if the intended class is Byte and the Object implements Number.

For jcoerce to work with all primitives, all possible primitive operations need
to be covered in JavaObject(Object, Class).

I have attached a patch that adds these conversions to JavaObject(Object,
Class). However, I've also changed other places where these primitive
conversions are done for consistency. My rule of thumb has been that the Java
Language Specification rules[1] should be followed, and the /only/ time a
/narrowing/ conversion is allowed where information can potentially be lost, is
through the JavaObject(Object, Class) constructor (in Lisp land, called using
the (java:jcoerce) function).

As such, the patch also changes the implementation classes Bignum, DoubleFloat,
SingleFloat, Fixnum, Character, and JavaObject, to support all possible
primitive /widening/ conversions. Their implementations of
LispObject.javaInstance(Class) continue to signal type errors for conversions
that cannot be performend, but also now for all narrowing operations. Previously
/some/ of these classes allowed /some/ narrowing conversions to happen. I think
a case could be made for each of these LispObject implementations to allow any
primitve conversion. The disadvantage is not knowing an information reducing
operation potentially occurred (and is counter to the behavior of not finding
narrowing methods such as Brad's original example) -- at the very least if all
primitive narrowing operations /were/ allowed from Fixnum.javaInstance(Class)
for example, a compiler warning should be emitted (though I'm not sure how that
would be implemented).

The following patch may break code that relies on the one conversion that has
worked in the past (to byte, in the JavaObject(Object, Class) constructor. In
fact, this patch breaks swank. To fix your swank, modify your swank's abcl.lisp
to include the now required jcoerce call (in the octets-to-jbytes function): the
jstatic call should now look like:
... (around line 191)
         do (java:jstatic (java:jmethod "java.lang.reflect.Array"  "setByte"
                           "java.lang.Object" "int" "byte")
                          "java.lang.reflect.Array"
                          bytes i (java:jcoerce byte "byte")))

What are your thoughts? I dislike backwards incompatible changes like this, but
I think the impact is probably low due to the fact that method resolution used
rules to only find widening conversions, and the fact that short and byte are
not as commonly used in method calls.

Previously, (jfield) and (jproperty) uses would also not signal a type error
unless LispObject.javaInstance(Class) did not support the conversion (and like
mentioned previously, some classes did some primitive narrowing operations).
Uses of these fields will now (consistently) with method calls, error on
narrowing conversions unless coerced.

Try the patch (but make sure to modify your swank!)...

Now my old example works like the following:
(jstatic "reverseBytes" "java.lang.Short" (jcoerce #x7f "short"))

Note also that the patch didn't break any new tests.  I ran tests via:
(require 'asdf)
(load "~/quicklisp/setup.lisp")
(asdf:initialize-source-registry
 `(:source-registry (:directory ,*default-pathname-defaults*) :inherit-configuration))
(asdf:test-system :abcl)


-Mark

[0] See patch attached (against latest master of https://github.com/armedbear/abcl)
   (3a54c91f)

[1] Widening Primitve Conversion
   https://docs.oracle.com/javase/specs/jls/se8/html/jls-5.html#jls-5.1.2
parent c1f6070a
......@@ -85,15 +85,11 @@ public final class Bignum extends LispInteger
@Override
public Object javaInstance(Class c) {
if (c == Byte.class || c == byte.class)
return Byte.valueOf((byte)value.intValue());
if (c == Short.class || c == short.class)
return Short.valueOf((short)value.intValue());
if (c == Integer.class || c == int.class)
return Integer.valueOf(value.intValue());
if (c == Long.class || c == long.class)
return Long.valueOf((long)value.longValue());
return javaInstance();
if (BigInteger.class.equals(c)) {
return value;
}
return error(new TypeError("Cannot convert Bignum to " + c.getName()));
}
......
......@@ -236,9 +236,11 @@ public final class DoubleFloat extends LispObject
@Override
public Object javaInstance(Class c)
{
if (c == Float.class || c == float.class)
return Float.valueOf((float)value);
return javaInstance();
if (Double.class.equals(c) || double.class.equals(c)) {
return Double.valueOf(value);
}
return error(new TypeError("Cannot convert DoubleFloat to " + c.getName()));
}
@Override
......
......@@ -76,13 +76,12 @@ public final class Fixnum extends LispInteger
@Override
public Object javaInstance(Class c)
{
if (c == Byte.class || c == byte.class)
return Byte.valueOf((byte)value);
if (c == Short.class || c == short.class)
return Short.valueOf((short)value);
if (c == Long.class || c == long.class)
if (Long.class.equals(c) || long.class.equals(c))
return Long.valueOf((long)value);
return javaInstance();
if (Integer.class.equals(c) || int.class.equals(c))
return Integer.valueOf(value);
return error(new TypeError("Cannot convert Fixnum to " + c.getName()));
}
@Override
......
......@@ -57,7 +57,9 @@ public final class JavaObject extends LispObject {
/**
* Constructs a Java Object with the given intended class, used to access
* the object reflectively. If the class represents a primitive type,
* the corresponding wrapper type is used instead.
* the corresponding wrapper type is used instead (and a narrowing conversion
* done if necessary).
*
* @throws ClassCastException if the object is not an instance of the
* intended class.
*/
......@@ -65,18 +67,47 @@ public final class JavaObject extends LispObject {
if(obj != null && intendedClass == null) {
intendedClass = obj.getClass();
}
if(intendedClass != null) {
if(obj != null && !intendedClass.equals(obj.getClass())) {
intendedClass = Java.maybeBoxClass(intendedClass);
if(!intendedClass.isInstance(obj)) {
if (intendedClass.equals(java.lang.Byte.class)
&& obj instanceof java.lang.Number) {
// Maps any number to two's complement 8bit byte representation
// ??? Is this a reasonable thing?
this.obj = ((java.lang.Number)obj).byteValue();
this.intendedClass = intendedClass;
return;
if (obj instanceof Number) {
if (intendedClass.equals(Byte.class)) {
obj = ((Number) obj).byteValue();
} else if (intendedClass.equals(Short.class)) {
obj = ((Number) obj).shortValue();
} else if (intendedClass.equals(Integer.class)) {
obj = ((Number) obj).intValue();
} else if (intendedClass.equals(Long.class)) {
obj = ((Number) obj).longValue();
} else if (intendedClass.equals(Character.class)) {
obj = Character.valueOf((char) ((Number) obj).intValue());
} else if (intendedClass.equals(Float.class)) {
obj = ((Number) obj).floatValue();
} else if (intendedClass.equals(Double.class)) {
obj = ((Number) obj).doubleValue();
} else {
throw new ClassCastException(obj + " can not be cast to " + intendedClass);
}
} else if (obj instanceof Character) {
char c = ((Character) obj).charValue();
if (intendedClass.equals(Byte.class)) {
obj = (byte) c;
} else if (intendedClass.equals(Short.class)) {
obj = (short) c;
} else if (intendedClass.equals(Integer.class)) {
obj = (int) c;
} else if (intendedClass.equals(Long.class)) {
obj = (long) c;
} else if (intendedClass.equals(Float.class)) {
obj = (float) c;
} else if (intendedClass.equals(Double.class)) {
obj = (double) c;
} else {
throw new ClassCastException(obj + " can not be cast to " + intendedClass);
}
} else {
throw new ClassCastException(obj + " can not be cast to " + intendedClass);
}
throw new ClassCastException(obj + " can not be cast to " + intendedClass);
}
}
this.obj = obj;
......@@ -278,6 +309,9 @@ public final class JavaObject extends LispObject {
} else {
c = Java.maybeBoxClass(c);
if (c.isAssignableFrom(intendedClass) || c.isInstance(obj)) {
// By using isAssignableFrom() we are assured that we don't do any narrowing
// here (use (jcoerce) for that, or from java .getInstance(obj, class))
// XXX In the case that c.isInstance(obj) should we then
// "fix" the intendedClass field with the (presumably)
// narrower type of 'obj'?
......@@ -289,6 +323,61 @@ public final class JavaObject extends LispObject {
// representations.
return obj;
} else {
// Still need to do widening conversions here...
if (obj instanceof Byte) {
if (c.equals(Short.class)) {
return ((Byte) obj).shortValue();
} else if (c.equals(Integer.class)) {
return ((Byte) obj).intValue();
} else if (c.equals(Long.class)) {
return ((Byte) obj).longValue();
} else if (c.equals(Float.class)) {
return ((Byte) obj).floatValue();
} else if (c.equals(Double.class)) {
return ((Byte) obj).doubleValue();
}
} else if (obj instanceof Short) {
if (c.equals(Integer.class)) {
return ((Short) obj).intValue();
} else if (c.equals(Long.class)) {
return ((Short) obj).longValue();
} else if (c.equals(Float.class)) {
return ((Short) obj).floatValue();
} else if (c.equals(Double.class)) {
return ((Short) obj).doubleValue();
}
} else if (obj instanceof Integer) {
if (c.equals(Long.class)) {
return ((Integer) obj).longValue();
} else if (c.equals(Float.class)) {
return ((Integer) obj).floatValue();
} else if (c.equals(Double.class)) {
return ((Integer) obj).doubleValue();
}
} else if (obj instanceof Long) {
if (c.equals(Float.class)) {
return ((Long) obj).floatValue();
} else if (c.equals(Double.class)) {
return ((Long) obj).doubleValue();
}
} else if (obj instanceof Float) {
if (c.equals(Double.class)) {
return ((Float) obj).doubleValue();
}
// nothing that doesn't potentially narrow for double
// } else if (obj instanceof Double) {
} else if (obj instanceof Character) {
if (c.equals(Integer.class)) {
return (int) ((Character) obj).charValue();
} else if (c.equals(Long.class)) {
return (long) ((Character) obj).charValue();
} else if (c.equals(Float.class)) {
return (float) ((Character) obj).charValue();
} else if (c.equals(Double.class)) {
return (double) ((Character) obj).charValue();
}
}
return error(new TypeError(intendedClass.getName() + " is not assignable to " + c.getName()));
}
}
......
......@@ -200,7 +200,14 @@ public final class LispCharacter extends LispObject
@Override
public Object javaInstance(Class c)
{
return javaInstance();
if (Integer.class.equals(c) || int.class.equals(c))
return (int) Character.valueOf(value);
if (Long.class.equals(c) || long.class.equals(c))
return (long) Character.valueOf(value);
if (Character.class.equals(c) || char.class.equals(c))
return Character.valueOf(value);
return error(new TypeError("Cannot convert LispCharacter to " + c.getName()));
}
@Override
......
......@@ -241,9 +241,12 @@ public final class SingleFloat extends LispObject
@Override
public Object javaInstance(Class c)
{
if (c == Float.class || c == float.class)
if (Float.class.equals(c) || float.class.equals(c))
return Float.valueOf(value);
return javaInstance();
if (Double.class.equals(c) || double.class.equals(c))
return Double.valueOf(value);
return error(new TypeError("Cannot convert SingleFloat to " + c.getName()));
}
@Override
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment