extend documentation of Reflect.compareMethods and add more tests (#5792)

* extend documentation of Reflect.compareMethods and add more tests (some of them where already in TestReflect)

* fix Reflect.compareMethods for closures on Array and Strings

* use weakrefs for closures, don't create closures for field access (length)

* [python] create better closures for array and string methods, fix Reflect.compareMethods, see #5792

* [python] make Boot.field more verbose, but faster

src/generators/genpy.ml

@@ -846,13 +846,6 @@ module Transformer = struct
 		| (is_value, TBinop(OpAssignOp op,{eexpr = TField(e1,FDynamic s); etype = t},e2)) ->
 			let e = dynamic_field_read_write ae.a_next_id e1 s op e2 t in
 			transform_expr ~is_value:is_value e
-		(*
-		| (is_value, TField(e1, FClosure(Some ({cl_path = [],("str")},_),cf))) ->
-
-		| (is_value, TField(e1, FClosure(Some ({cl_path = [],("list")},_),cf))) ->
-			let e = dynamic_field_read e1 cf.cf_name ae.a_expr.etype in
-			transform_expr ~is_value:is_value e
-		*)
 		| (is_value, TBinop(OpAssign, left, right))->
 			(let left = trans true [] left in
 			let right = trans true [] right in
@@ -1429,14 +1422,14 @@ module Printer = struct
 				Printf.sprintf "HxString.fromCharCode"
 			| FStatic({cl_path = ["python";"internal"],"UBuiltins"},{cf_name = s}) ->
 				s
-			| FClosure (Some(c,cf),_)  when call_override(name) && ((is_type "" "list")(TClassDecl c)) ->
-				Printf.sprintf "_hx_partial(python_internal_ArrayImpl.%s, %s)" name obj
-			| FInstance (c,_,cf) when call_override(name) && ((is_type "" "list")(TClassDecl c)) ->
-				Printf.sprintf "_hx_partial(python_internal_ArrayImpl.%s, %s)" name obj
-			| FClosure (Some(c,cf),_)  when call_override(name) && ((is_type "" "str")(TClassDecl c)) ->
-				Printf.sprintf "_hx_partial(HxString.%s, %s)" name obj
-			| FInstance (c,_,cf) when call_override(name) && ((is_type "" "str")(TClassDecl c)) ->
-				Printf.sprintf "_hx_partial(HxString.%s, %s)" name obj
+			| FClosure (Some(c,cf),_) when ((is_type "" "list")(TClassDecl c)) ->
+				Printf.sprintf "python_Boot.createClosure(%s, python_internal_ArrayImpl.%s)" obj name
+			| FClosure (Some(c,cf),_) when ((is_type "" "str")(TClassDecl c)) ->
+				Printf.sprintf "python_Boot.createClosure(%s, HxString.%s)" obj name
+			| FInstance (c,_,cf) when ((is_type "" "list")(TClassDecl c)) ->
+				Printf.sprintf "python_Boot.createClosure(%s, python_internal_ArrayImpl.%s)" obj name
+			| FInstance (c,_,cf) when ((is_type "" "str")(TClassDecl c)) ->
+				Printf.sprintf "python_Boot.createClosure(%s, HxString.%s)" obj name
 			| FInstance _ | FStatic _ ->
 				do_default ()
 			| FAnon cf when is_assign && call_override(name) ->

std/Reflect.hx

@@ -135,11 +135,15 @@ extern class Reflect {
 
 	/**
 		Compares the functions `f1` and `f2`.
-
+		
+		If `f1` or `f2` are null, the result is false.
 		If `f1` or `f2` are not functions, the result is unspecified.
 
 		Otherwise the result is true if `f1` and the `f2` are physically equal,
 		false otherwise.
+
+		If `f1` or `f2` are member method closures, the result is true if they
+		are closures of the same method on the same object value, false otherwise.
 	**/
 	public static function compareMethods( f1 : Dynamic, f2 : Dynamic ) : Bool;
 

std/python/Boot.hx

@@ -20,7 +20,7 @@
  * DEALINGS IN THE SOFTWARE.
  */
 package python;
-
+import python.internal.MethodClosure;
 import python.internal.ArrayImpl;
 import python.internal.Internal;
 import python.internal.StringImpl;
@@ -276,6 +276,8 @@ class Boot {
 		return UBuiltins.isinstance(o, UBuiltins.list);
 	}
 
+
+
 	static function simpleField( o : Dynamic, field : String ) : Dynamic {
 		if (field == null) return null;
 
@@ -283,49 +285,81 @@ class Boot {
 		return if (UBuiltins.hasattr(o, field)) UBuiltins.getattr(o, field) else null;
 	}
 
+	@:ifFeature("closure_Array", "closure_String")
+	static inline function createClosure (obj:Dynamic, func:Dynamic):Dynamic {
+		return new MethodClosure(obj, func);
+	}
+
 	static function field( o : Dynamic, field : String ) : Dynamic {
 		if (field == null) return null;
 
-		switch (field) {
-			case "length" if (isString(o)): return StringImpl.get_length(o);
-			case "toLowerCase" if (isString(o)): return StringImpl.toLowerCase.bind(o);
-			case "toUpperCase" if (isString(o)): return StringImpl.toUpperCase.bind(o);
-			case "charAt" if (isString(o)): return StringImpl.charAt.bind(o);
-			case "charCodeAt" if (isString(o)): return StringImpl.charCodeAt.bind(o);
-			case "indexOf" if (isString(o)): return StringImpl.indexOf.bind(o);
-			case "lastIndexOf" if (isString(o)): return StringImpl.lastIndexOf.bind(o);
-			case "split" if (isString(o)): return StringImpl.split.bind(o);
-			case "substr" if (isString(o)): return StringImpl.substr.bind(o);
-			case "substring" if (isString(o)): return StringImpl.substring.bind(o);
-			case "toString" if (isString(o)): return StringImpl.toString.bind(o);
-			case "length" if (isArray(o)): return ArrayImpl.get_length(o);
-			case "map" if (isArray(o)): return ArrayImpl.map.bind(o);
-			case "filter" if (isArray(o)): return ArrayImpl.filter.bind(o);
-			case "concat" if (isArray(o)): return ArrayImpl.concat.bind(o);
-			case "copy" if (isArray(o)): return function () return ArrayImpl.copy(o);
-			case "iterator" if (isArray(o)): return ArrayImpl.iterator.bind(o);
-			case "insert" if (isArray(o)): return ArrayImpl.insert.bind(o);
-			case "join" if (isArray(o)): return function (sep) return ArrayImpl.join(o, sep);
-			case "toString" if (isArray(o)): return ArrayImpl.toString.bind(o);
-			case "pop" if (isArray(o)): return ArrayImpl.pop.bind(o);
-			case "push" if (isArray(o)): return ArrayImpl.push.bind(o);
-			case "unshift" if (isArray(o)): return ArrayImpl.unshift.bind(o);
-			case "indexOf" if (isArray(o)): return ArrayImpl.indexOf.bind(o);
-			case "lastIndexOf" if (isArray(o)): return ArrayImpl.lastIndexOf.bind(o);
-			case "remove" if (isArray(o)): return ArrayImpl.remove.bind(o);
-			case "reverse" if (isArray(o)): return ArrayImpl.reverse.bind(o);
-			case "shift" if (isArray(o)): return ArrayImpl.shift.bind(o);
-			case "slice" if (isArray(o)): return ArrayImpl.slice.bind(o);
-			case "sort" if (isArray(o)): return ArrayImpl.sort.bind(o);
-			case "splice" if (isArray(o)): return ArrayImpl.splice.bind(o);
+		return switch (field) {
+			case "toLowerCase" if (isString(o)):
+				createClosure(o, StringImpl.toLowerCase);
+			case "toUpperCase" if (isString(o)):
+				createClosure(o, StringImpl.toUpperCase);
+			case "charAt" if (isString(o)):
+				createClosure(o, StringImpl.charAt);
+			case "charCodeAt" if (isString(o)):
+				createClosure(o, StringImpl.charCodeAt);
+			case "indexOf" if (isString(o)):
+				createClosure(o, StringImpl.indexOf);
+			case "lastIndexOf" if (isString(o)):
+				createClosure(o, StringImpl.lastIndexOf);
+			case "split" if (isString(o)):
+				createClosure(o, StringImpl.split);
+			case "substr" if (isString(o)):
+				createClosure(o, StringImpl.substr);
+			case "substring" if (isString(o)):
+				createClosure(o, StringImpl.substring);
+			case "toString" if (isString(o)):
+				createClosure(o, StringImpl.toString);
+			case "length" if (isArray(o)):
+				ArrayImpl.get_length(o);
+			case "map" if (isArray(o)):
+				createClosure(o, ArrayImpl.map);
+			case "filter" if (isArray(o)):
+				createClosure(o, ArrayImpl.filter);
+			case "concat" if (isArray(o)):
+				createClosure(o, ArrayImpl.concat);
+			case "copy" if (isArray(o)):
+				createClosure(o, ArrayImpl.copy);
+			case "iterator" if (isArray(o)):
+				createClosure(o, ArrayImpl.iterator);
+			case "insert" if (isArray(o)):
+				createClosure(o, ArrayImpl.insert);
+			case "join" if (isArray(o)):
+				createClosure(o, ArrayImpl.join);
+			case "toString" if (isArray(o)):
+				createClosure(o, ArrayImpl.toString);
+			case "pop" if (isArray(o)):
+				createClosure(o, ArrayImpl.pop);
+			case "push" if (isArray(o)):
+				createClosure(o, ArrayImpl.push);
+			case "unshift" if (isArray(o)):
+				createClosure(o, ArrayImpl.unshift);
+			case "indexOf" if (isArray(o)):
+				createClosure(o, ArrayImpl.indexOf);
+			case "lastIndexOf" if (isArray(o)):
+				createClosure(o, ArrayImpl.lastIndexOf);
+			case "remove" if (isArray(o)):
+				createClosure(o, ArrayImpl.remove);
+			case "reverse" if (isArray(o)):
+				createClosure(o, ArrayImpl.reverse);
+			case "shift" if (isArray(o)):
+				createClosure(o, ArrayImpl.shift);
+			case "slice" if (isArray(o)):
+				createClosure(o, ArrayImpl.slice);
+			case "sort" if (isArray(o)):
+				createClosure(o, ArrayImpl.sort);
+			case "splice" if (isArray(o)):
+				createClosure(o, ArrayImpl.splice);
+			default:
+				var field = handleKeywords(field);
+				if (UBuiltins.hasattr(o, field)) UBuiltins.getattr(o, field) else null;
 		}
-
-
-		var field = handleKeywords(field);
-		return if (UBuiltins.hasattr(o, field)) UBuiltins.getattr(o, field) else null;
 	}
 
-
 	static function getInstanceFields( c : Class<Dynamic> ) : Array<String> {
 		var f = if (Internal.hasFields(c)) (Internal.fieldFields(c) : Array<String>).copy() else [];
 		if (Internal.hasMethods(c))

std/python/_std/Reflect.hx

@@ -24,6 +24,7 @@ import python.internal.AnonObject;
 import python.internal.StringImpl;
 import python.internal.ArrayImpl;
 import python.internal.UBuiltins;
+import python.internal.MethodClosure;
 
 import python.lib.Inspect;
 import python.Syntax;
@@ -93,9 +94,19 @@ class Reflect {
 			else ( a == b ) ? 0 : (((cast a) > (cast b)) ? 1 : -1);
 	}
 
+	static inline function isClosure (v:Dynamic):Bool {
+		return UBuiltins.isinstance(v, MethodClosure);
+	}
+
 	public static function compareMethods( f1 : Dynamic, f2 : Dynamic ) : Bool {
 		if( f1 == f2 )
 			return true;
+		if (isClosure(f1) && isClosure(f2)) {
+			var m1 = (f1:MethodClosure);
+			var m2 = (f2:MethodClosure);
+			return m1.obj == m2.obj && m1.func == m2.func;
+
+		}
 		if( !isFunction(f1) || !isFunction(f2) )
 			return false;
 

std/python/internal/MethodClosure.hx

@@ -0,0 +1,14 @@
+package python.internal;
+
+
+class MethodClosure {
+	@:allow(Reflect) var obj:Dynamic;
+	@:allow(Reflect) var func:haxe.Constraints.Function;
+	public function new (obj:Dynamic, func:haxe.Constraints.Function) {
+		this.obj = obj;
+		this.func = func;
+	}
+	@:keep public function __call__ (args:VarArgs<Dynamic>) {
+		return this.func(this.obj, args);
+	}
+}

std/python/internal/UBuiltins.hx

@@ -26,6 +26,7 @@ package python.internal;
     Fields listed here must be synchronized with genpy's KeywordHandler.kwds2 list to be properly escaped.
 **/
 extern class UBuiltins {
+    static function id(x:Dynamic):String;
     static function len(x:Dynamic):Int;
     static function isinstance(o:Dynamic, c:Dynamic):Bool;
     static function str(o:Dynamic):String;

tests/unit/src/unitstd/Reflect.unit.hx

@@ -102,9 +102,19 @@ Reflect.compareMethods(y,z) == false;
 Reflect.compareMethods(x,x) == true;
 Reflect.compareMethods(y,y) == true;
 Reflect.compareMethods(z,z) == true;
-//Reflect.compareMethods(x,null) == false;
-//Reflect.compareMethods(null,x) == false;
-//Reflect.compareMethods(null,null) == false; // varies
+
+Reflect.compareMethods(x,null) == false;
+Reflect.compareMethods(null,x) == false;
+
+// compareMethods with closures
+var a = [1];
+var b = [2];
+var v : Dynamic = a.push;
+Reflect.compareMethods(a.push, a.push) == true;
+Reflect.compareMethods(a.push, a.pop) == false;
+Reflect.compareMethods(a.push, b.push) == false;
+Reflect.compareMethods(a.push, v) == true;
+Reflect.compareMethods(b.push, v) == false;
 
 // isObject
 Reflect.isObject({}) == true;