Merge pull request #315 from layus/fix-unify-rollback

Fix error message on failed unification

Author: layus

platform-test/CMakeLists.txt

@@ -13,6 +13,7 @@ set(BASE_FUNCTORS
     #"finalize.oz" "gc.oz"
     "state.oz" "thread.oz"
     #"vm.oz" #FIXME vm tests are buggy, see #313.
+    "unify.oz"
     "reflection.oz" "serializer.oz"
 )
 file(MAKE_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/base")

platform-test/base/unify.oz

@@ -0,0 +1,28 @@
+functor
+import
+   VM
+export
+   Return
+define
+   Return = unify([vmlist(proc {$}
+			     try
+				{VM.list} = unit
+				fail
+			     catch failure(debug:d(info:[eq(unit _)] ...) ...) then
+				skip
+			     end
+			  end
+			  keys:[unify])
+                   order(proc {$}
+			    fun {Const X} [1 2 X] end
+			 in
+			    try
+			       {Const 1} = {Const 2}
+			       fail
+			    catch failure(debug:d(info:[eq(1 2)] ...) ...) then
+			       skip
+			    end
+			 end
+			 keys:[unify])
+		  ])
+end

vm/vm/main/store.hh

@@ -97,6 +97,7 @@ void UnstableNode::copy(VM vm, StableNode& from) {
     make<Reference>(vm, &from);
 }
 
+// WARNING This code is highly sensitive for StructuralDualWalk::rebind in unify.cc
 void UnstableNode::copy(VM vm, UnstableNode& from) {
   if (from.isCopyable()) {
     set(from);
@@ -207,6 +208,7 @@ void RichNode::ensureStable(VM vm) {
   }
 }
 
+// WARNING This code is highly sensitive for StructuralDualWalk::rebind in unify.cc
 void RichNode::reinit(VM vm, StableNode& from) {
   if (node() == &from) {
     // do nothing
@@ -217,6 +219,7 @@ void RichNode::reinit(VM vm, StableNode& from) {
   }
 }
 
+// WARNING This code is highly sensitive for StructuralDualWalk::rebind in unify.cc
 void RichNode::reinit(VM vm, UnstableNode& from) {
   if (node() == &from) {
     // do nothing
@@ -231,6 +234,7 @@ void RichNode::reinit(VM vm, UnstableNode&& from) {
   node()->set(from);
 }
 
+// WARNING This code is highly sensitive for StructuralDualWalk::rebind in unify.cc
 void RichNode::reinit(VM vm, RichNode from) {
   if (from.isStable())
     reinit(vm, from.asStable());

vm/vm/main/unify.cc

@@ -465,8 +465,31 @@ bool StructuralDualWalk::processPair(VM vm, RichNode left, RichNode right) {
 }
 
 void StructuralDualWalk::rebind(VM vm, RichNode left, RichNode right) {
-  rebindTrail.push_back(vm, left.makeBackup());
-  left.reinit(vm, right);
+  // When given a Stable and an Unstable node, reinit will always alias the
+  // unstable node to the stable one, even if it is called on the stable one.
+  // In particular, it means that it can modify its argument, and not this.
+  // In that case, we need to backup the variable that will be modified.
+  // See #312 and #315 for details.
+  //
+  // In particular, StableNode.reinit(vm, UnstableNode) must never be called as
+  // it updates both `left` and `right` and breaks our backup system.
+  // To avoid that, we always call reinit on the less stable node we have.
+  if (right.isStable()) {
+      // `right` is stable, backup `left` because it will be aliased.
+      rebindTrail.push_back(vm, left.makeBackup());
+      // If both are stable, `left` must be modified (because that's the one we
+      // have a backup for).  That is why we call `left`'s reinit.
+      left.reinit(vm, right);
+  } else {
+      // `right` is unstable, backup it as it will be aliased.
+      rebindTrail.push_back(vm, right.makeBackup());
+      // If both are unstable, they are both aliased to a stable node
+      // initialised with reinit's second arg (`left` in this case).
+      // By calling `right`'s reinit, we ensure that `left` is still valid
+      // after a rollback. This leaves around an unnecessary indirection from
+      // `left` to the new stable node, but it is the price to pay for eagerness.
+      right.reinit(vm, left);
+  }
 }
 
 void StructuralDualWalk::cleanupOnFailure(VM vm) {