Repository: jpype-project/jpype (language: python)

Issue:
Unresolvable ambiguous overload for booleans
When writing wrappers for calling raphtory from python (see https://github.com/Raphtory/Raphtory/pull/704) we ran into an issue with ambiguous overloads for our methods that I was unable to resolve. 

I can reproduce the issue using three simple java classes:

```java
public class TestType {}
```

```java
public class TestTypeDerived extends TestType {}
```

```java
public class OverloadTest {
    public static void test(boolean a, TestType b) {
        String astr = Boolean.valueOf(a).toString();
        System.out.println("boolean called: " + astr);
    }

    public static  void test(Object a, TestType b) {
        System.out.println("object called: " + a.toString());
    }

    public static void main(String args[])
    {

        test(false, new TestTypeDerived());
        test("that", new TestTypeDerived());
    }
}
```

Calling this from java works as expected and outputs
```
boolean called: false
object called: that
```

However, if I try to call the `test` method from python using JPype the boolean case raises an ambiguous overload error when called with the derived type. Here is the python script I used to test this:

```python
import jpype.imports
from jpype import JClass, JBoolean, JObject
from pathlib import Path

path = [str(Path(__file__).parents[1] / "out/production/JPypeOverloadIssue")]  # set appropriately


if __name__ == "__main__":
    jpype.startJVM(classpath=path)
    from java.lang import Boolean, Object
    test = JClass("OverloadTest")
    test_type = JClass("TestType")()
    test_type_derived = JClass("TestTypeDerived")()
    test.test(True, test_type)  # works
    test.test("that", test_type)  # works
    test.test("that", test_type_derived)  # works
    test.test(JObject(True, Object), test_type_derived)  # works and calls object overload
    test.test(JObject(True, Boolean), test_type_derived)  # ambiguous overload
    test.test(True, test_type_derived)  # ambiguous overload
    test.test(JBoolean(True), test_type_derived)  # ambiguous overload
```

I would be willing to spend some time to try to fix this issue myself but I would need some pointers as to where to start.


Discussion / hints:
The issue is in the method resolution.   Current JPype uses a simple scheme for resolution when there is no governing rules provided by the Java method overloading.   There is a 4 level resolution for each match (exact, implicit, explicit, and none).  It goes through each overload and gets the matching of each.   Each match is given the lowest overload match.   If two are at the same match level then it decides that is ambiguous. 

In this case the "test_type_derived" is forcing the match to implicit so therefore it can't decide if bool or object match regardless of whether the cast is applied or not.  It is true that (exact, implicit) should be better than (implicit, implicit) but we don't have that level of sophistication.   The underlaying issue is that derived should be like exact, but isn't so that we can resolve the cases where multiple inherited objects are used for the overload.  

Oddly if you test Java directly the case ``test.test(new java.lang.Boolean(false), test_type_derived)`` will go to ``object``.

The current resolution would be to cast the second argument which ups its match to exact.
```
     TestType = jpype.JClass("TestType")
     test.test(True, TestType @ test_type_derived)  # ambiguous overload, resolved by cast
     test.test(JBoolean(True), TestType @ test_type_derived)  # ambiguous overload, resolved by cast
```

There are two methods that we use to resolve this but they are rather crude.  One is written in C and the other is in Java.   Then it probes for a method the first time in Java there is code that generates the list of which overloads mask others.   That is supposed to resolve this by saying ``test.test(bool, TestType)`` masks ``test.test(Object,TestType)``.  But it were to do so then ``test.test(java.lang.Boolean, TestType)`` would go to ``test.test(bool, TestType)`` which doesn't match the Java language.  The C method is the 4 types we discussed earlier.   I have debated for a long type whether there should be a 5th level of derived which is better than implicit.

So lets see if adding a 5th level resolves this problem....

``test.test(JBoolean(True), test_type_derived)`` 
 -  matches ``test.test(bool, TestType)`` with (exact,derived) => derived    
 -  matches ``test.test(Object, TestType)`` with (implicit,derived) => implicit

So it is unambiguously better to go to the first.    

Next.

``test.test(java.lang.Boolean(True), test_type_derived)`` 
 -  matches ``test.test(bool, TestType)`` with (implicit,derived) => implicit    
 -  matches ``test.test(Object, TestType)`` with (derived,derived) => derived
So it is unambiguously better to go to the second.    

Conclusion, it is fixable with a change to the C API.   

technical details
------------------
The enum types are located in ``common/native/include/jp_match.h``  so we can add a new level between ``_implicit`` and ``_exact```.

The conversion rules are located in ``common/native/jp_classhints.cpp`` in ``JPConversionObject``

```
        virtual JPMatch::Type matches(JPClass *cls, JPMatch &match) override
        {
                JP_TRACE_IN("JPConversionObject::matches");
                JPValue *value = match.getJavaSlot();
                if (value == NULL || match.frame == NULL)
                        return match.type = JPMatch::_none;
                match.conversion = this;
                JPClass *oc = value->getClass();
                if (oc == NULL)
                        return match.type = JPMatch::_none;
                if (oc == cls)
                {
                        // hey, this is me! :)
                        return match.type = JPMatch::_exact;
                }
                bool assignable = match.frame->IsAssignableFrom(oc->getJavaClass(), cls->getJavaClass()) != 0;
                JP_TRACE("assignable", assignable, oc->getCanonicalName(), cls->getCanonicalName());
                match.type = (assignable ? JPMatch::_implicit : JPMatch::_none);  // <== here would need to use the new level

                // This is the one except to the conversion rule patterns.
                // If it is a Java value then we must prevent it from proceeding
                // through the conversion rules even if it was not a match.
                // Thus the return result and the match type differ here.
                return JPMatch::_implicit;
                JP_TRACE_OUT;
        }
```

Next up is the audit information which is supposed to be able to report the quality of the match to a user which would need to know about the new level.  That would be ``PyJPClass_canConvertToJava`` in ``native/python/pyjp_class.cpp``.

And we would need to place a check in ``native/common/jp_methoddispatch.cpp`` in the method ``JPMethodDispatch::findOverload`` so that it can see if the best match is better than implicit to avoid the ambiguity on line  123.

And last would be the documentation section under ``Java conversions`` is ``doc/userguide.rst``.

We would not need to touch the Java overload list.   For reference it is in ``native/java/org/jpype/manager/MethodOverload.java``.

@marscher It would be good to get your input on this one.   I can fix it with about a ~5 line change by adding a new level to the enum table, changing the object conversion to use, and then updating the auditing logic so that it reports the new level of matching, but I am somewhat worried that this would be a major change in the resolution policy.   It should in principle never make something ambiguous that wasn't before but it is hard for me to see all of the possible interplay that it could have.   The whole logic of method resolution predates me (though I rewrote it to be easier to maintain), I haven't ever changed its rules.  The current rules are as I see it "exact always wins", "use Java overloading list otherwise", else "report ambiguity".  This would add a new dimension to the third case.   It is also unclear if it should be before or after the second rule.


thanks a lot for explaining this in detail @Thrameos. I also cannot tell which implications this would have. From your line of arguments it seems sane to add this new dimension to the "ambiguity" case. If we would have a downstream CI job, we could check if this breaks at least something in dependent projects.
We could also directly ask people to test this.

> It should in principle never make something ambiguous that wasn't before` this would be a mission cruicial goal, but ...

I really have to think how one could proof that.
My guess is that if the new rule is the last rule before reporting an ambiguity then it can only resolve an ambiguity and never change an existing resolution as the only outcome that it avoids is error reporting.   If we place it any higher on the list then it could change which method is resolved which we must avoid.  The test cases here can be added to the test bench, but it is hard to think of test cases that would actually prove that every resolution for each number of arguments will work.   To properly test it we would have to hit every combination of [primitives(including all promotions), boxed, string, specific object instance, derived object instance] for 1 to 3 arguments plus variadic methods.   That would be an very large matrix. 



Fix patch:
diff --git a/.bumpversion.cfg b/.bumpversion.cfg
index fb8776540..83750a368 100644
--- a/.bumpversion.cfg
+++ b/.bumpversion.cfg
@@ -1,5 +1,5 @@
 [bumpversion]
-current_version = 1.4.2_dev0
+current_version = 1.5.0_dev0
 commit = True
 tag = False
 parse = (?P<major>\d+)\.(?P<minor>\d+)\.(?P<patch>\d+)(\_(?P<release>[a-z]+)(?P<build>\d+))?
diff --git a/doc/CHANGELOG.rst b/doc/CHANGELOG.rst
index 123970f5c..4c5684a58 100644
--- a/doc/CHANGELOG.rst
+++ b/doc/CHANGELOG.rst
@@ -4,14 +4,20 @@ Changelog
 This changelog *only* contains changes from the *first* pypi release (0.5.4.3) onwards.
 
 Latest Changes:
+- **1.5.0_dev0 - 2023-04-03**
 - **1.4.2_dev0 - 2022-10-26**
 
   - Fixed crash when calling subscript on JArray.
 
   - Fixed direct byte buffers not reporting nbytes correctly when cast to memoryview.
-  - Expand the defintion for Functional interface to include classes without 
+
+  - Expand the defintion for Functional interface to include classes without
     FunctionInterface annotation.
 
+  - Add additional matching level for derived types to resolve ambiguities when
+    a derived type is used in place of base class when determining the method
+    overload.  This will resolve some previous ambiguities between methods.  
+
 - **1.4.1 - 2022-10-26**
   
   - Fixed issue with startJVM changing locale settings.
diff --git a/doc/userguide.rst b/doc/userguide.rst
index f7ba9a060..3fd9bb467 100644
--- a/doc/userguide.rst
+++ b/doc/userguide.rst
@@ -708,8 +708,8 @@ Java conversions
 ----------------
 
 A conversion is a permitted change from an object of one type to another.
-Conversions have three different degrees.  These are: exact, implicit, and
-explicit.
+Conversions have three different degrees.  These are: exact, derived, implicit,
+and explicit.
 
 Exact conversions are those in which the type of an object is identical.  In
 Java each class has only one definition thus there is no need for an exact
@@ -718,21 +718,30 @@ identical for which exact conversion rules apply.  For example, a Java string
 and a Python string both bind equally well to a method which requires a string,
 thus this is an exact conversion for the purposes of bind types.
 
+The next level of conversion is derived.  A derived class is one which is a
+descends from a required type.  It is better that implicit but worse than
+exact.  If all of the types in a method match are exact or derived then it will
+override a method in which one argument is implicit.
+
 The next level of conversion is implicit.  An implicit conversion is one that
-Java would perform automatically.  For example converting a derived class to is
-base class when setting a field would be an implicit conversion.  Java defines
-a number of other conversions such as converting a primitive to a boxed type
-or from a boxed type back to a primitive as implicit conversions..
+Java would perform automatically.  Java defines a number of other conversions
+such as converting a primitive to a boxed type or from a boxed type back to a
+primitive as implicit conversions.  Python conversions defined by the user are
+also considered to be implicit.
 
 Of course not every cast is safe to perform.  For example, converting an object
 whose type is currently viewed as a base type to a derived type is not
 performed automatically nor is converting from one boxed type to another.  For
 those operations the conversion must be explicitly requested, hence these are
-explicit conversions.  To request an explicit conversion an object must be
-"cast" using a cast operator.  In Java, a cast is requested by placing the type
-name in parentheses in front of the object to be cast.  Unfortunately, the same
-syntax is not allowed in Python.  Not every conversion is possible between Java
-types.  Types that cannot be converted are considerer to be conversion type "none".
+explicit conversions.   In Java, a cast is requested by placing the type name
+in parentheses in front of the object to be cast.  Python does not directly
+support Java casting syntax. To request an explicit conversion an object must
+be "cast" using a cast operator @.   Overloaded methods with an explicit
+argument will not be matched.  After applying an explicit cast, the match
+quality can improve to exact or derived depending on the cast type. 
+
+Not every conversion is possible between Java types.  Types that cannot be
+converted are considerer to be conversion type "none".
 
 Details on the standard conversions provided by JPype are given in the section
 `Type Matching`_.
diff --git a/jpype/__init__.py b/jpype/__init__.py
index ed4b9b402..9922a6635 100644
--- a/jpype/__init__.py
+++ b/jpype/__init__.py
@@ -51,7 +51,7 @@
 __all__.extend(_jcustomizer.__all__)
 __all__.extend(_gui.__all__)
 
-__version__ = "1.4.2_dev0"
+__version__ = "1.5.0_dev0"
 __version_info__ = __version__.split('.')
 
 
@@ -65,5 +65,5 @@
 JField = _jpype._JField
 
 if hasattr(_jpype, 'bootstrap'):
-   _jpype.bootstrap()
-   _core.initializeResources()
+    _jpype.bootstrap()
+    _core.initializeResources()
diff --git a/native/common/include/jp_match.h b/native/common/include/jp_match.h
index fa7ea5292..9c7ae8c71 100644
--- a/native/common/include/jp_match.h
+++ b/native/common/include/jp_match.h
@@ -27,7 +27,8 @@ class JPMatch
 		_none = 0,
 		_explicit = 1,
 		_implicit = 2,
-		_exact = 3
+		_derived = 3,
+		_exact = 4
 	} ;
 
 public:
@@ -91,4 +92,4 @@ class JPMethodMatch : public JPMethodCache
 	char m_Skip;
 } ;
 
-#endif /* JP_MATCH_H */
\ No newline at end of file
+#endif /* JP_MATCH_H */
diff --git a/native/common/jp_classhints.cpp b/native/common/jp_classhints.cpp
index 26494102f..89ee9a47c 100644
--- a/native/common/jp_classhints.cpp
+++ b/native/common/jp_classhints.cpp
@@ -651,7 +651,7 @@ class JPConversionObject : public JPConversion
 		}
 		bool assignable = match.frame->IsAssignableFrom(oc->getJavaClass(), cls->getJavaClass()) != 0;
 		JP_TRACE("assignable", assignable, oc->getCanonicalName(), cls->getCanonicalName());
-		match.type = (assignable ? JPMatch::_implicit : JPMatch::_none);
+		match.type = (assignable ? JPMatch::_derived : JPMatch::_none);
 
 		// This is the one except to the conversion rule patterns.
 		// If it is a Java value then we must prevent it from proceeding
@@ -865,7 +865,12 @@ class JPConversionJavaObjectAny : public JPConversionBox
 		if (value == NULL || match.frame == NULL || value->getClass() == NULL)
 			return match.type = JPMatch::_none;
 		match.conversion = this;
-		match.type = (value->getClass() == cls) ? JPMatch::_exact : JPMatch::_implicit;
+		if (value->getClass()->isPrimitive())
+			match.type = JPMatch::_implicit;
+		else if (value->getClass() == cls)
+			match.type = JPMatch::_exact;
+		else
+			match.type = JPMatch::_derived;
 		return match.type;
 		JP_TRACE_OUT;
 	}
diff --git a/native/common/jp_methoddispatch.cpp b/native/common/jp_methoddispatch.cpp
index d1c5af5e8..2744af404 100644
--- a/native/common/jp_methoddispatch.cpp
+++ b/native/common/jp_methoddispatch.cpp
@@ -118,7 +118,23 @@ bool JPMethodDispatch::findOverload(JPJavaFrame& frame, JPMethodMatch &bestMatch
 				}
 			}
 
+			// If the best match is worse than current, switch
+			if (bestMatch.m_Type < match.m_Type)
+			{
+				bestMatch = match;
+				// Remove any prior ambiguity
+				ambiguous.clear();
+				continue;
+			}
+
+			// If the best match is still better than current, keep it
+			if (bestMatch.m_Type > match.m_Type)
+			{
+				continue;
+			}
+
 			JP_TRACE("Adding to ambiguous list");
+
 			// Keep trace of ambiguous overloads for the error report.
 			ambiguous.push_back(*it);
 		}
diff --git a/native/common/jp_objecttype.cpp b/native/common/jp_objecttype.cpp
index 1d5657bec..52237cccf 100644
--- a/native/common/jp_objecttype.cpp
+++ b/native/common/jp_objecttype.cpp
@@ -65,4 +65,4 @@ void JPObjectType::getConversionInfo(JPConversionInfo &info)
 	hintsConversion->getInfo(this, info);
 	PyList_Append(info.ret, PyJPClass_create(frame, this).get());
 	JP_TRACE_OUT;
-}
\ No newline at end of file
+}
diff --git a/native/java/org/jpype/JPypeContext.java b/native/java/org/jpype/JPypeContext.java
index c34e2b113..59396d1e9 100644
--- a/native/java/org/jpype/JPypeContext.java
+++ b/native/java/org/jpype/JPypeContext.java
@@ -73,7 +73,7 @@
 public class JPypeContext
 {
 
-  public final String VERSION = "1.4.2_dev0";
+  public final String VERSION = "1.5.0_dev0";
 
   private static JPypeContext INSTANCE = new JPypeContext();
   // This is the C++ portion of the context.
diff --git a/native/python/pyjp_class.cpp b/native/python/pyjp_class.cpp
index a04150dda..0fb283959 100644
--- a/native/python/pyjp_class.cpp
+++ b/native/python/pyjp_class.cpp
@@ -687,6 +687,8 @@ static PyObject *PyJPClass_canConvertToJava(PyJPClass *self, PyObject *other)
 		return JPPyString::fromStringUTF8("explicit").keep();
 	if (match.type == JPMatch::_implicit)
 		return JPPyString::fromStringUTF8("implicit").keep();
+	if (match.type == JPMatch::_derived)
+		return JPPyString::fromStringUTF8("derived").keep();
 	if (match.type == JPMatch::_exact)
 		return JPPyString::fromStringUTF8("exact").keep();
 
diff --git a/native/python/pyjp_module.cpp b/native/python/pyjp_module.cpp
index a86bbe144..9dcfdfdf5 100644
--- a/native/python/pyjp_module.cpp
+++ b/native/python/pyjp_module.cpp
@@ -719,7 +719,7 @@ PyMODINIT_FUNC PyInit__jpype()
 	// PyJPModule = module;
 	Py_INCREF(module);
 	PyJPModule = module;
-	PyModule_AddStringConstant(module, "__version__", "1.4.2_dev0");
+	PyModule_AddStringConstant(module, "__version__", "1.5.0_dev0");
 	
 	// Our module will be used for PyFrame object and it is a requirement that
 	// we have a builtins in our dictionary.
diff --git a/setup.py b/setup.py
index 44cbdd7da..709fe5f25 100644
--- a/setup.py
+++ b/setup.py
@@ -50,7 +50,7 @@
 
 setup(
     name='JPype1',
-    version='1.4.2_dev0',
+    version='1.5.0_dev0',
     description='A Python to Java bridge.',
     long_description=open('README.rst').read(),
     license='License :: OSI Approved :: Apache Software License',


Test patch:
diff --git a/test/harness/jpype/overloads/DerivedTest.java b/test/harness/jpype/overloads/DerivedTest.java
new file mode 100644
index 000000000..73d992dd4
--- /dev/null
+++ b/test/harness/jpype/overloads/DerivedTest.java
@@ -0,0 +1,44 @@
+/* ****************************************************************************
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+
+  See NOTICE file for details.
+**************************************************************************** */
+package jpype.overloads;
+
+/** Test to see that using a derived type rather than a base class will 
+ * not alter the method resolution process.
+ */
+public class DerivedTest
+{
+	public static class Base {}
+        public static class Derived extends Base {}
+
+	public static int testStatic(boolean a, Base b) {
+		return 1;
+	}
+
+	public static int testStatic(Object a, Base b) {
+		return 2;
+	}
+
+	public int testMember(boolean a, Base b) {
+		return 3;
+	}
+
+	public int testMember(Object a, Base b) {
+		return 4;
+	}
+
+	public static class Sub extends DerivedTest {}
+
+}
diff --git a/test/jpypetest/test_conversion.py b/test/jpypetest/test_conversion.py
index dce5cbd52..a169691fe 100644
--- a/test/jpypetest/test_conversion.py
+++ b/test/jpypetest/test_conversion.py
@@ -144,3 +144,9 @@ def testCharBytes(self):
         with self.assertRaises(TypeError):
             ja[0:1] = [object()]
         ja[0:1] = ["\u265E"]
+
+    def testDerived(self):
+        # Test Java Object any conversion
+        self.assertEqual(jpype.JClass("java.lang.Object")._canConvertToJava(jpype.JClass("java.lang.String")("a")), "derived")
+        # Test Java Object specific conversion
+        self.assertEqual(jpype.JClass("java.util.List")._canConvertToJava(jpype.JClass("java.util.ArrayList")()), "derived")
diff --git a/test/jpypetest/test_overloads.py b/test/jpypetest/test_overloads.py
index 7604ac7ba..bde5948ec 100644
--- a/test/jpypetest/test_overloads.py
+++ b/test/jpypetest/test_overloads.py
@@ -283,3 +283,50 @@ def testRunnable(self):
             cb = Runnable @ fun4
         with self.assertRaises(TypeError):
             cb = Runnable @ mc.fun4
+
+    def testDerivedStatic(self):
+        Boolean = jpype.JClass("java.lang.Boolean")
+        Object = jpype.JClass("java.lang.Object")
+        DerivedTest = JClass("jpype.overloads.DerivedTest")
+        Base = DerivedTest.Base()
+        Derived = DerivedTest.Derived()
+        self.assertEqual(DerivedTest.testStatic(True, Base), 1)
+        self.assertEqual(DerivedTest.testStatic("that", Base), 2)
+        self.assertEqual(DerivedTest.testStatic(True, Derived), 1)
+        self.assertEqual(DerivedTest.testStatic("that", Derived), 2)
+        self.assertEqual(DerivedTest.testStatic(jpype.JBoolean(True), Base), 1)
+        self.assertEqual(DerivedTest.testStatic(jpype.JBoolean(True), Derived), 1)
+        self.assertEqual(DerivedTest.testStatic(jpype.JObject(True, Boolean), Derived), 2)
+        self.assertEqual(DerivedTest.testStatic(jpype.JObject(True, Object), Derived), 2)
+
+    def testDerivedMember(self):
+        Boolean = jpype.JClass("java.lang.Boolean")
+        Object = jpype.JClass("java.lang.Object")
+        DerivedTest = JClass("jpype.overloads.DerivedTest")
+        Base = DerivedTest.Base()
+        Derived = DerivedTest.Derived()
+        obj = DerivedTest()
+        self.assertEqual(obj.testMember(True, Base), 3)
+        self.assertEqual(obj.testMember("that", Base), 4)
+        self.assertEqual(obj.testMember(True, Derived), 3)
+        self.assertEqual(obj.testMember("that", Derived), 4)
+        self.assertEqual(obj.testMember(jpype.JBoolean(True), Base), 3)
+        self.assertEqual(obj.testMember(jpype.JBoolean(True), Derived), 3)
+        self.assertEqual(obj.testMember(jpype.JObject(True, Boolean), Derived), 4)
+        self.assertEqual(obj.testMember(jpype.JObject(True, Object), Derived), 4)
+
+    def testDerivedSub(self):
+        Boolean = jpype.JClass("java.lang.Boolean")
+        Object = jpype.JClass("java.lang.Object")
+        DerivedTest = JClass("jpype.overloads.DerivedTest")
+        Base = DerivedTest.Base()
+        Derived = DerivedTest.Derived()
+        obj = DerivedTest.Sub()
+        self.assertEqual(obj.testMember(True, Base), 3)
+        self.assertEqual(obj.testMember("that", Base), 4)
+        self.assertEqual(obj.testMember(True, Derived), 3)
+        self.assertEqual(obj.testMember("that", Derived), 4)
+        self.assertEqual(obj.testMember(jpype.JBoolean(True), Base), 3)
+        self.assertEqual(obj.testMember(jpype.JBoolean(True), Derived), 3)
+        self.assertEqual(obj.testMember(jpype.JObject(True, Boolean), Derived), 4)
+        self.assertEqual(obj.testMember(jpype.JObject(True, Object), Derived), 4)