Emit warning for RW fetch if offset does not exist

Girgias · Girgias · commit 0ef62d201ece · 2024-07-12T03:14:00.000+01:00
The implementation is the best I could come up after this was noticed
diff --git a/Zend/tests/offsets/ArrayAccess_container_offset_behaviour.phpt b/Zend/tests/offsets/ArrayAccess_container_offset_behaviour.phpt
@@ -69,6 +69,8 @@ VAR_DUMP_OF_OFFSET
 Notice: Indirect modification of overloaded element of CLASS_NAME has no effect in %s on line 81
 Cannot use a scalar value as an array
 Nested Read-Write:
+string(15) "CLASS_NAME::offsetExists"
+VAR_DUMP_OF_OFFSET
 string(12) "CLASS_NAME::offsetGet"
 VAR_DUMP_OF_OFFSET
 
diff --git a/Zend/tests/offsets/ArrayObject_container_offset_behaviour.phpt b/Zend/tests/offsets/ArrayObject_container_offset_behaviour.phpt
@@ -110,6 +110,8 @@ Nested Read-Write:
 
 Deprecated: Implicit conversion from float %F to int loses precision in %s on line %d
 
+Deprecated: Implicit conversion from float %F to int loses precision in %s on line %d
+
 Deprecated: Implicit conversion from float %F to int loses precision in %s on line %d
 Nested isset():
 
@@ -249,6 +251,8 @@ Nested Read-Write:
 
 Warning: Resource ID#3 used as offset, casting to integer (3) in %s on line 88
 
+Warning: Resource ID#3 used as offset, casting to integer (3) in %s on line 88
+
 Warning: Resource ID#3 used as offset, casting to integer (3) in %s on line 88
 Nested isset():
 
diff --git a/Zend/tests/offsets/DimensionFetchable_container_offset_behaviour.phpt b/Zend/tests/offsets/DimensionFetchable_container_offset_behaviour.phpt
@@ -59,6 +59,8 @@ string(27) "DimensionFetch::offsetFetch"
 VAR_DUMP_OF_OFFSET
 Cannot write to offset of object of type DimensionFetch
 Nested Read-Write:
+string(27) "DimensionRead::offsetExists"
+VAR_DUMP_OF_OFFSET
 string(27) "DimensionFetch::offsetFetch"
 VAR_DUMP_OF_OFFSET
 Cannot read-write offset of object of type DimensionFetch
diff --git a/Zend/tests/offsets/increment_decrement_object_offset.phpt b/Zend/tests/offsets/increment_decrement_object_offset.phpt
@@ -0,0 +1,92 @@
+--TEST--
+increment/decrement on proper container objects
+--FILE--
+<?php
+
+require_once __DIR__ . DIRECTORY_SEPARATOR . 'test_offset_helpers.inc';
+
+class Legacy implements ArrayAccess {
+    private int $i = 15;
+    public function offsetSet($offset, $value): void {
+        var_dump(__METHOD__);
+        $this->i = $value;
+    }
+    public function offsetGet($offset): mixed {
+        var_dump(__METHOD__);
+        return $this->i;
+    }
+    public function offsetUnset($offset): void {
+        var_dump(__METHOD__);
+    }
+    public function offsetExists($offset): bool {
+        var_dump(__METHOD__);
+        return true;
+    }
+}
+
+class Container implements DimensionFetchable, DimensionWritable {
+    private int $i = 15;
+
+    public function offsetExists($offset): bool {
+        var_dump(__METHOD__);
+        return true;
+    }
+    public function offsetGet($offset): mixed {
+        var_dump(__METHOD__);
+        return $this->i;
+    }
+    public function offsetSet($offset, $value): void {
+        var_dump(__METHOD__);
+        $this->i = $value;
+    }
+    public function &offsetFetch($offset): mixed {
+        var_dump(__METHOD__);
+        return $this->i;
+    }
+}
+
+foreach ([new Legacy(), new Container()] as $container) {
+    echo zend_test_var_export($container), '[0]', "\n";
+
+    try {
+        var_dump($container[0]++);
+        var_dump($container[0]);
+    } catch (Throwable $e) {
+        echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+    }
+    try {
+        var_dump($container[0]--);
+        var_dump($container[0]);
+    } catch (Throwable $e) {
+        echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+    }
+}
+
+?>
+--EXPECTF--
+new Legacy()[0]
+string(20) "Legacy::offsetExists"
+string(17) "Legacy::offsetGet"
+
+Notice: Indirect modification of overloaded element of Legacy has no effect in %s on line %d
+int(15)
+string(17) "Legacy::offsetGet"
+int(15)
+string(20) "Legacy::offsetExists"
+string(17) "Legacy::offsetGet"
+
+Notice: Indirect modification of overloaded element of Legacy has no effect in %s on line %d
+int(15)
+string(17) "Legacy::offsetGet"
+int(15)
+new Container()[0]
+string(23) "Container::offsetExists"
+string(22) "Container::offsetFetch"
+int(15)
+string(20) "Container::offsetGet"
+int(16)
+string(23) "Container::offsetExists"
+string(22) "Container::offsetFetch"
+int(16)
+string(20) "Container::offsetGet"
+int(15)
diff --git a/Zend/tests/offsets/increment_decrement_operation_order.phpt b/Zend/tests/offsets/increment_decrement_operation_order.phpt
@@ -64,29 +64,43 @@ new stdClass()[0]
 Error: Cannot use object of type stdClass as array
 Error: Cannot use object of type stdClass as array
 new ArrayObject()[0]
+
+Warning: Undefined offset in %s on line %d
 NULL
 int(1)
 new A()[0]
+string(15) "A::offsetExists"
+int(0)
 string(12) "A::offsetGet"
 int(0)
 
 Notice: Indirect modification of overloaded element of A has no effect in %s on line %d
 int(5)
+string(15) "A::offsetExists"
+int(0)
 string(12) "A::offsetGet"
 int(0)
 
 Notice: Indirect modification of overloaded element of A has no effect in %s on line %d
 int(5)
 new B()[0]
+string(15) "B::offsetExists"
+int(0)
 NULL
+string(15) "B::offsetExists"
+int(0)
 int(1)
 new DimensionRead()[0]
 Error: Cannot fetch offset of object of type DimensionRead
 Error: Cannot fetch offset of object of type DimensionRead
 new DimensionFetch()[0]
+string(27) "DimensionRead::offsetExists"
+int(0)
 string(27) "DimensionFetch::offsetFetch"
 int(0)
 TypeError: Cannot increment DimensionFetch
+string(27) "DimensionRead::offsetExists"
+int(0)
 string(27) "DimensionFetch::offsetFetch"
 int(0)
 TypeError: Cannot decrement DimensionFetch
diff --git a/Zend/tests/offsets/internal_handlers.phpt b/Zend/tests/offsets/internal_handlers.phpt
@@ -245,7 +245,7 @@ DimensionHandlersNoArrayAccess, read: false, write: true, has: false, unset: fal
 nested write: appending then write
 DimensionHandlersNoArrayAccess, read: false, write: true, has: false, unset: false, readType: uninitialized, hasOffset: true, checkEmpty: uninitialized, offset: 'bar'
 nested read-write
-DimensionHandlersNoArrayAccess, read: true, write: true, has: false, unset: false, readType: uninitialized, hasOffset: true, checkEmpty: uninitialized, offset: 'bar'
+DimensionHandlersNoArrayAccess, read: true, write: true, has: true, unset: false, readType: uninitialized, hasOffset: true, checkEmpty: uninitialized, offset: 'bar'
 nested read-write: appending then write
 DimensionHandlersNoArrayAccess, read: true, write: true, has: false, unset: false, readType: uninitialized, hasOffset: true, checkEmpty: uninitialized, offset: 'bar'
 nested isset
diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c
@@ -2996,19 +2996,31 @@ static zend_never_inline void zend_fetch_object_dimension_address(zval *result,
 			ZEND_ASSERT(zend_check_dimension_interfaces_implemented(obj, /* has_offset */ true, BP_VAR_FETCH));
 
 			ZVAL_DEREF(offset);
+
+			bool offset_exists = false;
 			/* For null coalesce we first check if the offset exist,
 			 * if it does we call the fetch_dimension() handler,
 			 * otherwise just return an undef result */
 			if (UNEXPECTED(type == BP_VAR_IS)) {
-				ZEND_ASSERT(offset && "BP_VAR_IS with append operation is compile time failure");
 				ZEND_ASSERT(obj->ce->dimension_handlers->has_dimension);
 				/* The key does not exist */
 				if (!obj->ce->dimension_handlers->has_dimension(obj, offset)) {
 					ZVAL_UNDEF(result);
 					goto clean_up;
 				}
+			} else if (UNEXPECTED(type == BP_VAR_RW)) {
+				/* RW semantics dictate that the offset must actually exists, otherwise a warning is emitted */
+				offset_exists = obj->ce->dimension_handlers->has_dimension(obj, offset);
 			}
 			retval = obj->ce->dimension_handlers->fetch_dimension(obj, offset, result);
+			if (UNEXPECTED(type == BP_VAR_RW && !offset_exists && !EG(exception))) {
+				// TODO Better warning?
+				zend_error(E_WARNING, "Undefined offset");
+				if (UNEXPECTED(EG(exception))) {
+					ZVAL_UNDEF(result);
+					goto clean_up;
+				}
+			}
 		} else if (!offset && obj->ce->dimension_handlers->fetch_append) {
 			ZEND_ASSERT(zend_check_dimension_interfaces_implemented(obj, /* has_offset */ false, BP_VAR_FETCH));
 			ZEND_ASSERT(type != BP_VAR_IS);
diff --git a/Zend/zend_operators.c b/Zend/zend_operators.c
@@ -2683,6 +2683,11 @@ ZEND_API zend_result ZEND_FASTCALL increment_function(zval *op1) /* {{{ */
 		case IS_REFERENCE:
 			op1 = Z_REFVAL_P(op1);
 			goto try_again;
+		case IS_INDIRECT:
+			op1 = Z_INDIRECT_P(op1);
+			ZEND_ASSERT(Z_TYPE_P(op1) != IS_INDIRECT && "I am still indirect");
+			return FAILURE;
+			goto try_again;
 		case IS_OBJECT: {
 			if (Z_OBJ_HANDLER_P(op1, do_operation)) {
 				zval op2;
diff --git a/ext/spl/tests/bug70852.phpt b/ext/spl/tests/bug70852.phpt
@@ -10,4 +10,6 @@ var_dump($y[NULL]++);
 --EXPECTF--
 Warning: Undefined array key "" in %s on line %d
 NULL
+
+Warning: Undefined offset in %s on line %d
 NULL
diff --git a/tests/classes/array_access_003.phpt b/tests/classes/array_access_003.phpt
@@ -50,6 +50,7 @@ ObjectOne::offsetGet(1)
 string(6) "fooBar"
 ObjectOne::offsetGet(2)
 int(1)
+ObjectOne::offsetExists(2)
 ObjectOne::offsetGet(2)
 
 Notice: Indirect modification of overloaded element of ObjectOne has no effect in %sarray_access_003.php on line 39
diff --git a/tests/classes/array_access_004.phpt b/tests/classes/array_access_004.phpt
@@ -48,6 +48,7 @@ ObjectOne::offsetGet(1)
 string(6) "fooBar"
 ObjectOne::offsetGet(2)
 int(1)
+ObjectOne::offsetExists(2)
 ObjectOne::offsetGet(2)
 
 Notice: Indirect modification of overloaded element of ObjectOne has no effect in %sarray_access_004.php on line 39
diff --git a/tests/classes/array_access_008.phpt b/tests/classes/array_access_008.phpt
@@ -11,7 +11,7 @@ class Peoples implements ArrayAccess {
     }
 
     function offsetExists($index): bool {
-        return array_key_exists($this->person, $index);
+        return array_key_exists($index, $this->person);
     }
 
     function offsetGet($index): mixed {

Original file line number	Diff line number	Diff line change
`@@ -11,7 +11,7 @@ class Peoples implements ArrayAccess {`
`11`	`11`	`}`
`12`	`12`
`13`	`13`	`function offsetExists($index): bool {`
`14`		`- return array_key_exists($this->person, $index);`
	`14`	`+ return array_key_exists($index, $this->person);`
`15`	`15`	`}`
`16`	`16`
`17`	`17`	`function offsetGet($index): mixed {`