Skip to content

Commit

Permalink
Fixed issue with recursive equality test on maps/lists.
Browse files Browse the repository at this point in the history
(Would go into infinite loop under certain circumstances.)
Now also bails out much faster in the case where two items are reference equal.
  • Loading branch information
JoeStrout committed Oct 27, 2023
1 parent d17ddeb commit 8e55362
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 1 deletion.
4 changes: 3 additions & 1 deletion MiniScript-cpp/src/MiniScript/MiniscriptTypes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ namespace MiniScript {
ValuePair(const Value& inA, const Value& inB) : a(inA), b(inB) {}
ValuePair() {}
bool operator==(const ValuePair& rhs) {
// Careful: we muste use RefEqual here to detect reference loops
// Careful: we must use RefEqual here to detect reference loops
// below; if we used ==, which does a deep comparison, it could
// just send us into an infinite recursion right here.
return Value::RefEqual(a, rhs.a) && Value::RefEqual(b, rhs.b);
Expand All @@ -718,6 +718,7 @@ namespace MiniScript {
long aCount = listA.Count();
ValueList listB((ListStorage<Value>*)pair.b.data.ref);
if (listB.Count() != aCount) return false;
if (Value::RefEqual(pair.a, pair.b)) continue;
for (int i=0; i < aCount; i++) {
ValuePair newPair(listA[i], listB[i]);
if (!visited.Contains(newPair)) toDo.push_back(newPair);
Expand All @@ -728,6 +729,7 @@ namespace MiniScript {
long countA = dictA.Count();
ValueDict dictB((DictionaryStorage<Value, Value>*)pair.b.data.ref);
if (dictB.Count() != countA) return false;
if (Value::RefEqual(pair.a, pair.b)) continue;
ValueList keys = dictA.Keys();
for (int i=0; i<countA; i++) {
Value key = keys[i];
Expand Down
15 changes: 15 additions & 0 deletions MiniScript-cs/MiniscriptTypes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -190,10 +190,12 @@ protected bool RecursiveEqual(Value rhs) {
toDo.Push(new ValuePair() { a = this, b = rhs });
while (toDo.Count > 0) {
var pair = toDo.Pop();

visited.Add(pair);
if (pair.a is ValList listA) {
var listB = pair.b as ValList;
if (listB == null) return false;
if (Object.ReferenceEquals(listA, listB)) continue;
int aCount = listA.values.Count;
if (aCount != listB.values.Count) return false;
for (int i = 0; i < aCount; i++) {
Expand All @@ -203,6 +205,7 @@ protected bool RecursiveEqual(Value rhs) {
} else if (pair.a is ValMap mapA) {
var mapB = pair.b as ValMap;
if (mapB == null) return false;
if (Object.ReferenceEquals(mapA, mapB)) continue;
if (mapA.map.Count != mapB.map.Count) return false;
foreach (KeyValuePair<Value, Value> kv in mapA.map) {
Value valFromB;
Expand Down Expand Up @@ -265,6 +268,18 @@ protected int RecursiveHash()
struct ValuePair {
public Value a;
public Value b;
public override bool Equals(object obj) {
if (obj is ValuePair other) {
return ReferenceEquals(a, other.a) && ReferenceEquals(b, other.b);
}
return false;
}

public override int GetHashCode() {
unchecked {
return ((a != null ? a.GetHashCode() : 0) * 397) ^ (b != null ? b.GetHashCode() : 0);
}
}
}

public class ValueSorter : IComparer<Value>
Expand Down

0 comments on commit 8e55362

Please sign in to comment.