From 3b790606e1a135871403a7418025412844c137b8 Mon Sep 17 00:00:00 2001 From: Jaroslav Tulach Date: Tue, 19 Sep 2023 17:39:04 +0200 Subject: [PATCH] Avoid StackOverflow when comparing unknown foreign objects (#7780) Closes #7677 by eliminating the _stackoverflow execption_. In general it seems _too adventurous_ to walk members of random foreign objects. There can be anything including cycles. Rather than trying to be too smart in these cases, let's just rely on `InteropLibrary.isIdentical` message. # Important Notes Calling `sort` on the `numpy` array no longer yields an error, but the array isn't sorted - that needs a fix on the Python side: https://github.com/oracle/graalpython/issues/354 - once it is in, the elements will be treated as numbers and the sorting happens automatically (without any changes in Enso code). --- .../builtin/meta/EqualsComplexNode.java | 49 ++--------- .../src/Python_Examples_Spec.enso | 7 +- .../base_test_helpers/IntHolderEquals.java | 22 +++++ test/Tests/src/Data/Vector_Spec.enso | 4 +- test/Tests/src/Semantic/Equals_Spec.enso | 81 ++++++++++++++++++- 5 files changed, 114 insertions(+), 49 deletions(-) create mode 100644 test/Tests/polyglot-sources/enso-test-java-helpers/src/main/java/org/enso/base_test_helpers/IntHolderEquals.java diff --git a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsComplexNode.java b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsComplexNode.java index 71cb5b41f650..f8d5999d80a3 100644 --- a/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsComplexNode.java +++ b/engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsComplexNode.java @@ -370,50 +370,10 @@ boolean equalsInteropObjectWithMembers( @Shared("typesLib") @CachedLibrary(limit = "10") TypesLibrary typesLib, @Shared("equalsNode") @Cached EqualsNode equalsNode, @Shared("hostValueToEnsoNode") @Cached HostValueToEnsoNode valueToEnsoNode) { - try { - Object selfMembers = interop.getMembers(selfObject); - Object otherMembers = interop.getMembers(otherObject); - assert interop.getArraySize(selfMembers) < Integer.MAX_VALUE - : "Long array sizes not supported"; - int membersSize = (int) interop.getArraySize(selfMembers); - if (interop.getArraySize(otherMembers) != membersSize) { - return false; - } - - // Check member names - String[] memberNames = new String[membersSize]; - for (int i = 0; i < membersSize; i++) { - String selfMemberName = interop.asString(interop.readArrayElement(selfMembers, i)); - String otherMemberName = interop.asString(interop.readArrayElement(otherMembers, i)); - if (!equalsNode.execute(selfMemberName, otherMemberName)) { - return false; - } - memberNames[i] = selfMemberName; - } - - // Check member values - for (int i = 0; i < membersSize; i++) { - if (interop.isMemberReadable(selfObject, memberNames[i]) - && interop.isMemberReadable(otherObject, memberNames[i])) { - Object selfMember = - valueToEnsoNode.execute(interop.readMember(selfObject, memberNames[i])); - Object otherMember = - valueToEnsoNode.execute(interop.readMember(otherObject, memberNames[i])); - if (!equalsNode.execute(selfMember, otherMember)) { - return false; - } - } - } + if (interop.isIdentical(selfObject, otherObject, interop)) { return true; - } catch (UnsupportedMessageException - | InvalidArrayIndexException - | UnknownIdentifierException e) { - throw new IllegalStateException( - String.format( - "One of the interop objects has probably wrongly specified interop API " - + "for members. selfObject = %s ; otherObject = %s", - selfObject, otherObject), - e); + } else { + return false; } } @@ -549,6 +509,9 @@ boolean isObjectWithMembers(Object object, InteropLibrary interop) { if (interop.isTime(object)) { return false; } + if (interop.hasHashEntries(object)) { + return false; + } return interop.hasMembers(object); } diff --git a/test/Examples_Tests/src/Python_Examples_Spec.enso b/test/Examples_Tests/src/Python_Examples_Spec.enso index 2517de41c511..c5b9b7a80cba 100644 --- a/test/Examples_Tests/src/Python_Examples_Spec.enso +++ b/test/Examples_Tests/src/Python_Examples_Spec.enso @@ -49,11 +49,16 @@ spec = Test.group "Python Examples" <| main = dir/"src"/"Main.enso" main.exists . should_be_true code = """ + from Standard.Base import all + foreign python random_array s = """ import numpy return numpy.random.normal(size=s) - main = random_array 10 + main = + arr = random_array 10 + vec = arr.to_vector.sort + [ arr, vec ] code . write main on_existing_file=Existing_File_Behavior.Overwrite diff --git a/test/Tests/polyglot-sources/enso-test-java-helpers/src/main/java/org/enso/base_test_helpers/IntHolderEquals.java b/test/Tests/polyglot-sources/enso-test-java-helpers/src/main/java/org/enso/base_test_helpers/IntHolderEquals.java new file mode 100644 index 000000000000..5ba1ca4d2f56 --- /dev/null +++ b/test/Tests/polyglot-sources/enso-test-java-helpers/src/main/java/org/enso/base_test_helpers/IntHolderEquals.java @@ -0,0 +1,22 @@ +package org.enso.base_test_helpers; + +public final class IntHolderEquals { + public final int value; + public final Integer boxed; + + public IntHolderEquals(int value) { + this.value = value; + this.boxed = value; + } + + public int hashCode() { + return value; + } + + public boolean equals(Object obj) { + if (obj instanceof IntHolderEquals other) { + return value == other.value; + } + return false; + } +} diff --git a/test/Tests/src/Data/Vector_Spec.enso b/test/Tests/src/Data/Vector_Spec.enso index bb01775904ca..237d7d79e82c 100644 --- a/test/Tests/src/Data/Vector_Spec.enso +++ b/test/Tests/src/Data/Vector_Spec.enso @@ -760,13 +760,13 @@ type_spec name alter = Test.group name <| alter [T.Value 1 2, T.Value 3 3, T.Value 1 2] . distinct . should_equal [T.Value 1 2, T.Value 3 3] alter [T.Value 1 2, T.Value 3 3, T.Value 1 2, Nothing] . distinct . should_equal [T.Value 1 2, T.Value 3 3, Nothing] alter [Nothing, T.Value 1 2, T.Value 3 3, T.Value 1 2, Nothing] . distinct . should_equal [Nothing, T.Value 1 2, T.Value 3 3] - alter [T.Value 1 2, Date.new hours=3] . distinct . should_equal [T.Value 1 2, Date.new hours=3] + alter [T.Value 1 2, Date.new year=1973] . distinct . should_equal [T.Value 1 2, Date.new year=1973] Test.specify "should return a vector containing only unique elements up to some criteria" <| alter [Pair.new 1 "a", Pair.new 2 "b", Pair.new 1 "c"] . distinct (on = _.first) . should_equal [Pair.new 1 "a", Pair.new 2 "b"] Test.specify "should be able to sort a heterogenous vector" <| - arr = [ 1, 1.3, "hi", Date.today, Date_Time.now, [ 0 ] ] + arr = alter [ 1, 1.3, "hi", Date.today, Date_Time.now, [ 0 ] ] (arr.sort on=(.to_text) . map .to_text) . should_equal (arr.map .to_text . sort) (arr.sort on=(_.to_text) . map .to_text) . should_equal (arr.map .to_text . sort) (arr.sort on=(x-> x.to_text) . map .to_text) . should_equal (arr.map .to_text . sort) diff --git a/test/Tests/src/Semantic/Equals_Spec.enso b/test/Tests/src/Semantic/Equals_Spec.enso index 0ebc40913696..4ab805e897f2 100644 --- a/test/Tests/src/Semantic/Equals_Spec.enso +++ b/test/Tests/src/Semantic/Equals_Spec.enso @@ -3,8 +3,11 @@ from Standard.Base import all from Standard.Test import Test, Test_Suite import Standard.Test.Extensions -polyglot java import java.nio.file.Path as JavaPath +polyglot java import java.math.BigInteger as Java_Big_Integer +polyglot java import java.nio.file.Path as Java_Path polyglot java import java.util.Random as Java_Random +polyglot java import org.enso.base_test_helpers.IntHolder +polyglot java import org.enso.base_test_helpers.IntHolderEquals type CustomEqType C1 f1 @@ -110,6 +113,24 @@ foreign js js_true = """ foreign js js_text_foo = """ return 'foo' +foreign js js_json a b = """ + return JSON.parse(` + { + "a" : ${a}, + "b" : ${b} + }`); + +foreign js js_map a b = """ + var m = new Map() + m.set("a", a) + m.set("b", b); + return m + +foreign js js_obj a b = """ + var m = {}; + m.a = a; + m.b = b; + return m; spec = Test.group "Operator ==" <| @@ -158,8 +179,8 @@ spec = ((CustomEqType.C1 0) == (CustomEqType.C2 7 3)).should_be_false Test.specify "should dispatch to equals on host values" <| - path1 = JavaPath.of "home" "user" . resolve "file.txt" - path2 = JavaPath.of "home" "user" "file.txt" + path1 = Java_Path.of "home" "user" . resolve "file.txt" + path2 = Java_Path.of "home" "user" "file.txt" (path1 == path2).should_be_true path3 = path1.resolve "subfile.txt" (path3 == path2).should_be_false @@ -203,5 +224,59 @@ spec = dupl_tree = tree.deep_copy Test.with_clue "Seed sed to 42" (tree == dupl_tree).should_be_true + Test.specify "partially applied constructors aren't == " <| + f1 = CustomEqType.C2 10 + f2 = CustomEqType.C2 10 + f1==f2 . should_be_false + + Test.group "Polyglot Operator ==" <| + Test.specify "should not try to compare members" <| + x = IntHolder.new 5 + y = IntHolder.new 5 + z = IntHolder.new 3 + x==z . should_be_false + x==y . should_be_false + y==y . should_be_true + x==x . should_be_true + z==z . should_be_true + + Test.specify "should not try to compare members in JS object" <| + x = js_obj 5 3 + y = js_obj 5 3 + z = js_obj 3 5 + x==z . should_be_false + x==y . should_be_false + y==y . should_be_true + x==x . should_be_true + z==z . should_be_true + + Test.specify "should invoke equals" <| + x = IntHolderEquals.new 5 + y = IntHolderEquals.new 5 + z = IntHolderEquals.new 3 + x==z . should_be_false + x==y . should_be_true + y==y . should_be_true + x==x . should_be_true + z==z . should_be_true + + Test.specify "should compare big integer" <| + x = Java_Big_Integer.new "54024430107564432" + y = Java_Big_Integer.new "54024430107564432" + x==y . should_be_true + + Test.specify "JSON is found different" <| + x = js_json 10 5 + y = js_json 10 5 + x==y . should_be_false + + Test.specify "Identical JSON is found equal" <| + x = js_json 10 5 + x==x . should_be_true + + Test.specify "JavaScript Map is found same" <| + x = js_map 10 5 + y = js_map 10 5 + x==y . should_be_true main = Test_Suite.run_main spec