Fix Heap comparator interference

README.md

@@ -24,6 +24,8 @@ Documentation is available on [php.net](https://www.php.net/manual/en/book.ds.ph
 Heap iterators retain a snapshot of the values present when created; `rewind()` restarts that snapshot even if the original heap has changed.
 Calling `next()` or `rewind()` on the same iterator from its comparator throws a `RuntimeException`.
 
+Each `Heap` uses its own comparator, including during nested heap operations and interleaved iteration.
+
 You should also include the [polyfill](https://github.com/php-ds/polyfill) in your project for IDE integration.
 
 ## Installation

php_ds.c

@@ -132,7 +132,7 @@ PHP_FUNCTION(heap)
 
     if (values && Z_TYPE_P(values) != IS_NULL) {
         ds_heap_compare_func_t compare = php_ds_heap_get_compare(obj);
-        ds_heap_push_all(heap, values, compare);
+        ds_heap_push_all(heap, values, compare, obj);
     }
 
     ZVAL_OBJ(return_value, obj_zend);

src/ds/ds_heap.c

@@ -28,7 +28,7 @@ static inline void ds_heap_compact(ds_heap_t *heap)
     }
 }
 
-int ds_heap_default_compare(zval *a, zval *b)
+int ds_heap_default_compare(zval *a, zval *b, void *context)
 {
     zval retval;
     if (compare_function(&retval, a, b) == SUCCESS) {
@@ -104,7 +104,7 @@ zval *ds_heap_peek(ds_heap_t *heap)
     return &heap->nodes[0];
 }
 
-void ds_heap_push(ds_heap_t *heap, zval *value, ds_heap_compare_func_t compare)
+void ds_heap_push(ds_heap_t *heap, zval *value, ds_heap_compare_func_t compare, void *context)
 {
     uint32_t index;
     uint32_t parent;
@@ -117,7 +117,7 @@ void ds_heap_push(ds_heap_t *heap, zval *value, ds_heap_compare_func_t compare)
     for (index = heap->size; index > 0; index = parent) {
         parent = PARENT(index);
 
-        if (compare(value, &heap->nodes[parent]) <= 0) {
+        if (compare(value, &heap->nodes[parent], context) <= 0) {
             break;
         }
 
@@ -129,7 +129,7 @@ void ds_heap_push(ds_heap_t *heap, zval *value, ds_heap_compare_func_t compare)
     heap->size++;
 }
 
-void ds_heap_pop(ds_heap_t *heap, zval *return_value, ds_heap_compare_func_t compare)
+void ds_heap_pop(ds_heap_t *heap, zval *return_value, ds_heap_compare_func_t compare, void *context)
 {
     uint32_t index;
     uint32_t swap;
@@ -165,12 +165,12 @@ void ds_heap_pop(ds_heap_t *heap, zval *return_value, ds_heap_compare_func_t com
         swap = LEFT(index);
 
         // Pick the larger child.
-        if (swap + 1 < heap->size && compare(&heap->nodes[swap], &heap->nodes[swap + 1]) < 0) {
+        if (swap + 1 < heap->size && compare(&heap->nodes[swap], &heap->nodes[swap + 1], context) < 0) {
             swap++;
         }
 
         // If the bottom value is >= the larger child, we're done.
-        if (compare(&bottom, &heap->nodes[swap]) >= 0) {
+        if (compare(&bottom, &heap->nodes[swap], context) >= 0) {
             break;
         }
 
@@ -184,16 +184,17 @@ void ds_heap_pop(ds_heap_t *heap, zval *return_value, ds_heap_compare_func_t com
 typedef struct _ds_heap_push_all_data {
     ds_heap_t               *heap;
     ds_heap_compare_func_t   compare;
+    void                   *context;
 } ds_heap_push_all_data;
 
 static int ds_heap_iterator_add(zend_object_iterator *iterator, void *puser)
 {
     ds_heap_push_all_data *data = (ds_heap_push_all_data *) puser;
-    ds_heap_push(data->heap, iterator->funcs->get_current_data(iterator), data->compare);
+    ds_heap_push(data->heap, iterator->funcs->get_current_data(iterator), data->compare, data->context);
     return ZEND_HASH_APPLY_KEEP;
 }
 
-void ds_heap_push_all(ds_heap_t *heap, zval *values, ds_heap_compare_func_t compare)
+void ds_heap_push_all(ds_heap_t *heap, zval *values, ds_heap_compare_func_t compare, void *context)
 {
     if (!values) {
         return;
@@ -202,22 +203,22 @@ void ds_heap_push_all(ds_heap_t *heap, zval *values, ds_heap_compare_func_t comp
     if (ds_is_array(values)) {
         zval *value;
         ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(values), value) {
-            ds_heap_push(heap, value, compare);
+            ds_heap_push(heap, value, compare, context);
         }
         ZEND_HASH_FOREACH_END();
         return;
     }
 
     if (ds_is_traversable(values)) {
-        ds_heap_push_all_data data = { heap, compare };
+        ds_heap_push_all_data data = { heap, compare, context };
         spl_iterator_apply(values, ds_heap_iterator_add, &data);
         return;
     }
 
     ARRAY_OR_TRAVERSABLE_REQUIRED();
 }
 
-void ds_heap_to_array(ds_heap_t *heap, zval *array, ds_heap_compare_func_t compare)
+void ds_heap_to_array(ds_heap_t *heap, zval *array, ds_heap_compare_func_t compare, void *context)
 {
     if (heap->size == 0) {
         array_init(array);
@@ -231,7 +232,7 @@ void ds_heap_to_array(ds_heap_t *heap, zval *array, ds_heap_compare_func_t compa
 
     while (clone->size > 0) {
         zval value;
-        ds_heap_pop(clone, &value, compare);
+        ds_heap_pop(clone, &value, compare, context);
         add_next_index_zval(array, &value);
     }
 

src/ds/ds_heap.h

@@ -8,7 +8,7 @@
 #define DS_HEAP_SIZE(h)      ((h)->size)
 #define DS_HEAP_IS_EMPTY(h)  ((h)->size == 0)
 
-typedef int (*ds_heap_compare_func_t)(zval *a, zval *b);
+typedef int (*ds_heap_compare_func_t)(zval *a, zval *b, void *context);
 
 typedef struct _ds_heap_t {
     zval                    *nodes;
@@ -23,19 +23,19 @@ ds_heap_t *ds_heap_clone(ds_heap_t *heap);
 void ds_heap_separate(ds_heap_t **heap);
 void ds_heap_release(ds_heap_t *heap);
 
-void ds_heap_push(ds_heap_t *heap, zval *value, ds_heap_compare_func_t compare);
-void ds_heap_push_all(ds_heap_t *heap, zval *values, ds_heap_compare_func_t compare);
-void ds_heap_pop(ds_heap_t *heap, zval *return_value, ds_heap_compare_func_t compare);
+void ds_heap_push(ds_heap_t *heap, zval *value, ds_heap_compare_func_t compare, void *context);
+void ds_heap_push_all(ds_heap_t *heap, zval *values, ds_heap_compare_func_t compare, void *context);
+void ds_heap_pop(ds_heap_t *heap, zval *return_value, ds_heap_compare_func_t compare, void *context);
 zval *ds_heap_peek(ds_heap_t *heap);
 
 void ds_heap_allocate(ds_heap_t *heap, uint32_t capacity);
 uint32_t ds_heap_capacity(ds_heap_t *heap);
 
-void ds_heap_to_array(ds_heap_t *heap, zval *array, ds_heap_compare_func_t compare);
+void ds_heap_to_array(ds_heap_t *heap, zval *array, ds_heap_compare_func_t compare, void *context);
 
 void ds_heap_clear(ds_heap_t *heap);
 void ds_heap_free(ds_heap_t *heap);
 
-int ds_heap_default_compare(zval *a, zval *b);
+int ds_heap_default_compare(zval *a, zval *b, void *context);
 
 #endif

src/php/classes/php_heap_ce.c

@@ -40,7 +40,7 @@ METHOD(__construct)
     if (values && Z_TYPE_P(values) != IS_NULL) {
         SEPARATE();
         ds_heap_compare_func_t compare = php_ds_heap_get_compare(obj);
-        ds_heap_push_all(obj->heap, values, compare);
+        ds_heap_push_all(obj->heap, values, compare, obj);
     }
 }
 
@@ -70,7 +70,7 @@ METHOD(push)
     SEPARATE();
 
     for (int i = 0; i < argc; i++) {
-        ds_heap_push(obj->heap, &argv[i], compare);
+        ds_heap_push(obj->heap, &argv[i], compare, obj);
     }
 }
 
@@ -79,7 +79,7 @@ METHOD(pop)
     php_ds_heap_t *obj = THIS_DS_HEAP();
     PARSE_NONE;
     SEPARATE();
-    ds_heap_pop(obj->heap, return_value, php_ds_heap_get_compare(obj));
+    ds_heap_pop(obj->heap, return_value, php_ds_heap_get_compare(obj), obj);
 }
 
 METHOD(peek)
@@ -117,14 +117,14 @@ METHOD(toArray)
 {
     php_ds_heap_t *obj = THIS_DS_HEAP();
     PARSE_NONE;
-    ds_heap_to_array(obj->heap, return_value, php_ds_heap_get_compare(obj));
+    ds_heap_to_array(obj->heap, return_value, php_ds_heap_get_compare(obj), obj);
 }
 
 METHOD(jsonSerialize)
 {
     php_ds_heap_t *obj = THIS_DS_HEAP();
     PARSE_NONE;
-    ds_heap_to_array(obj->heap, return_value, php_ds_heap_get_compare(obj));
+    ds_heap_to_array(obj->heap, return_value, php_ds_heap_get_compare(obj), obj);
 }
 
 METHOD(__serialize)
@@ -141,7 +141,7 @@ METHOD(__serialize)
 
     while (clone->size > 0) {
         zval value;
-        ds_heap_pop(clone, &value, compare);
+        ds_heap_pop(clone, &value, compare, obj);
         add_next_index_zval(return_value, &value);
     }
 
@@ -157,7 +157,7 @@ METHOD(__unserialize)
 
     zval *value;
     ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(data), value) {
-        ds_heap_push(obj->heap, value, compare);
+        ds_heap_push(obj->heap, value, compare, obj);
     } ZEND_HASH_FOREACH_END();
 }
 

src/php/handlers/php_heap_handlers.c

@@ -35,7 +35,7 @@ static HashTable *php_ds_heap_get_debug_info(zend_object *obj, int *is_temp)
     ds_heap_t *heap = heap_obj->heap;
     zval arr;
     *is_temp = 1;
-    ds_heap_to_array(heap, &arr, php_ds_heap_get_compare(heap_obj));
+    ds_heap_to_array(heap, &arr, php_ds_heap_get_compare(heap_obj), heap_obj);
     return Z_ARRVAL(arr);
 }
 

src/php/iterators/php_heap_iterator.c

@@ -40,7 +40,8 @@ static void php_ds_heap_iterator_set_current(php_ds_heap_iterator_t *iterator)
     DTOR_AND_UNDEF(&iterator->intern.data);
 
     if (!DS_HEAP_IS_EMPTY(iterator->heap)) {
-        ds_heap_pop(iterator->heap, &iterator->intern.data, iterator->compare);
+        ds_heap_pop(iterator->heap, &iterator->intern.data, iterator->compare,
+            php_ds_heap_fetch_object(iterator->object));
     }
 }
 

src/php/objects/php_heap.c

@@ -3,21 +3,23 @@
 
 #include "php_heap.h"
 
-int ds_heap_user_compare(zval *a, zval *b)
+int ds_heap_user_compare(zval *a, zval *b, void *context)
 {
+    php_ds_heap_t *obj = context;
+    // Keep callback state local so nested heap operations cannot overwrite it.
+    zend_fcall_info fci = obj->fci;
+    zend_fcall_info_cache fci_cache = obj->fci_cache;
     zval params[2];
     zval retval;
 
     ZVAL_COPY_VALUE(&params[0], a);
     ZVAL_COPY_VALUE(&params[1], b);
 
-    DSG(user_compare_fci).param_count = 2;
-    DSG(user_compare_fci).params      = params;
-    DSG(user_compare_fci).retval      = &retval;
+    fci.param_count = 2;
+    fci.params      = params;
+    fci.retval      = &retval;
 
-    if (zend_call_function(
-            &DSG(user_compare_fci),
-            &DSG(user_compare_fci_cache)) == SUCCESS) {
+    if (zend_call_function(&fci, &fci_cache) == SUCCESS) {
         int result = (int) zval_get_long(&retval);
         zval_ptr_dtor(&retval);
         return result;
@@ -29,8 +31,6 @@ int ds_heap_user_compare(zval *a, zval *b)
 ds_heap_compare_func_t php_ds_heap_get_compare(php_ds_heap_t *obj)
 {
     if (obj->has_comparator) {
-        DSG(user_compare_fci)       = obj->fci;
-        DSG(user_compare_fci_cache) = obj->fci_cache;
         return ds_heap_user_compare;
     }
     return ds_heap_default_compare;
@@ -119,7 +119,7 @@ int php_ds_heap_unserialize(zval *object, zend_class_entry *ce, const unsigned c
             goto error;
         }
 
-        ds_heap_push(heap, value, ds_heap_default_compare);
+        ds_heap_push(heap, value, ds_heap_default_compare, NULL);
     }
 
     ZVAL_OBJ(object, php_ds_heap_create_object_ex(heap));

src/php/objects/php_heap.h

@@ -32,15 +32,14 @@ do {                                                                \
 
 /**
  * Returns the compare function for this heap.
- * If the heap has a custom comparator, sets up the module globals
- * and returns the user compare function. Otherwise returns the default.
+ * The heap object must be passed as context when invoking the comparator.
  */
 ds_heap_compare_func_t php_ds_heap_get_compare(php_ds_heap_t *obj);
 
 /**
- * User comparator function that reads from module globals.
+ * User comparator function that reads the callable from the heap object.
  */
-int ds_heap_user_compare(zval *a, zval *b);
+int ds_heap_user_compare(zval *a, zval *b, void *context);
 
 zend_object *php_ds_heap_create_object_ex(ds_heap_t *heap);
 zend_object *php_ds_heap_create_object(zend_class_entry *ce);

tests/Heap/comparator_nested.phpt

@@ -0,0 +1,78 @@
+--TEST--
+Heap: nested heap operations preserve each caller's comparator
+--FILE--
+<?php
+$inner = new Ds\Heap([1, 2, 3, 4, 5], fn($a, $b) => $a <=> $b);
+$compare = function ($a, $b) use ($inner) {
+    $inner->toArray();
+    return $b <=> $a;
+};
+$heap = new Ds\Heap([3, 5, 1, 4, 2], $compare);
+echo json_encode($heap->toArray()), "\n";
+
+// A temporary heap's callback must not be retained by the surrounding operation.
+$heap = new Ds\Heap([3, 5, 1, 4, 2], function ($a, $b) {
+    $temporary = new Ds\Heap([10, 20], fn($x, $y) => $x <=> $y);
+    $temporary->toArray();
+    return $b <=> $a;
+});
+echo json_encode($heap->toArray()), "\n";
+
+// An exception caught inside the comparator must not replace its callback state.
+$throw = false;
+$throwing = new Ds\Heap([1, 2, 3], function ($a, $b) use (&$throw) {
+    if ($throw) {
+        throw new RuntimeException('nested comparison');
+    }
+    return $a <=> $b;
+});
+$throw = true;
+$heap = new Ds\Heap([3, 5, 1, 4, 2], function ($a, $b) use ($throwing) {
+    try {
+        $throwing->toArray();
+    } catch (RuntimeException $e) {
+    }
+    return $b <=> $a;
+});
+echo json_encode($heap->toArray()), "\n";
+$heap->push(7, 0, 6);
+echo json_encode($heap), "\n";
+echo json_encode($heap->__serialize()), "\n";
+$result = [];
+while (!$heap->isEmpty()) {
+    $result[] = $heap->pop();
+}
+echo json_encode($result), "\n";
+
+$heap = new Ds\Heap([], $compare);
+$heap->__unserialize([3, 5, 1, 4, 2]);
+echo json_encode($heap->toArray()), "\n";
+echo json_encode(Ds\heap([3, 5, 1, 4, 2], $compare)->toArray()), "\n";
+
+// Heap operations must also leave the comparator of a surrounding sort intact.
+$sequence = new Ds\Seq([3, 5, 1, 4, 2]);
+$sequence->sort(function ($a, $b) use ($inner) {
+    $inner->toArray();
+    return $b <=> $a;
+});
+echo json_encode($sequence->toArray()), "\n";
+
+// Sorts inside a heap comparator must not affect the heap's ordering either.
+$heap = new Ds\Heap([3, 5, 1, 4, 2], function ($a, $b) {
+    $sequence = new Ds\Seq([3, 1, 2]);
+    $sequence->sort(fn($x, $y) => $x <=> $y);
+    return $b <=> $a;
+});
+echo json_encode($heap->toArray()), "\n";
+?>
+--EXPECT--
+[1,2,3,4,5]
+[1,2,3,4,5]
+[1,2,3,4,5]
+[0,1,2,3,4,5,6,7]
+[0,1,2,3,4,5,6,7]
+[0,1,2,3,4,5,6,7]
+[1,2,3,4,5]
+[1,2,3,4,5]
+[5,4,3,2,1]
+[1,2,3,4,5]