Skip to content

Commit

Permalink
Avoid StackOverflow when comparing unknown foreign objects (#7780)
Browse files Browse the repository at this point in the history
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: oracle/graalpython#354 - once it is in, the elements will be treated as numbers and the sorting happens automatically (without any changes in Enso code).
  • Loading branch information
JaroslavTulach authored Sep 19, 2023
1 parent 5c19814 commit 3b79060
Show file tree
Hide file tree
Showing 5 changed files with 114 additions and 49 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -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);
}

Expand Down
7 changes: 6 additions & 1 deletion test/Examples_Tests/src/Python_Examples_Spec.enso
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
@@ -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;
}
}
4 changes: 2 additions & 2 deletions test/Tests/src/Data/Vector_Spec.enso
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
81 changes: 78 additions & 3 deletions test/Tests/src/Semantic/Equals_Spec.enso
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 ==" <|
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

0 comments on commit 3b79060

Please sign in to comment.