Re: Typed properties patch

Hi Jow, First of all, I appreciate the amount of effort you already invested into this idea. Anyway, I still don't agree with the following terms of RFC and corresponding implementation: 1) While parameters allow null to be accepted as the default value, null is never a valid value for a typed property. I think we must have a way to make properties explicitly nullable. Otherwise we won't be able to define even simple binary tree... class Node { public Node $left = null; public Node $right = null; } 2) The implementation will raise an exception where a typed property is accessed before being initialized:<https://3v4l.org/cVkcj/rfc#tabs> This makes unnecessary complication, and opens thousands possibilities to cheat the engine. class A { public int $x; } $a = new A; var_dump($a->x++); // prints NULL (according to RFC, this should raise Exception) I propose to initialize typed properties with the value of corresponding type (if default value is omitted). boolean => false int => 0 double => 0.0 string => "" array => [] object, resource, callable => null Actually, I propose implicit default values if they are not specified. This will always guarantee the type and eliminate run-time checks on read. 3) It is possible to unset typed properties, and return them to the same state as a property that was never set. This mean that properties types are not guaranteed, and Optimizer/Compilers/JIT won't be able to generate the best code performing type check on every read. I would prefer to disable unset() of typed properties. 4) Checking the type of a zval is an extremely cheap operation, there may be some very minor performance impact only when typed properties are used. The patch makes some slowdown even if typed properties are not used (0.5% more instruction retired on 100 requests to Wordpress according to callgrind) 5) In principle, knowing the type of a variable in advance allows you to make optimizations, and in the long term, there is good reason to think that typed properties will allow us to improve performance. This is an optimistic assumption [&#X1f60a] For now typed-parameters make slowdown except of expected speedup, properties are not going to change this. Actually the type of property may make some gain only if we know the type of object and its definition at compile-time, but in most cases we do't know the class definition, because definition and usage are in different PHP scripts. Especially with (2) and (3), even if we know type of object and property, we'll still have to check if property is initialised on each usage. Thanks. Dmitry. ________________________________ From: Dmitry Stogov Sent: Wednesday, April 6, 2016 10:32 To: Joe Watkins Subject: Typed properties patch Ho Joe, I see some tests failures (SIGSEGV) > Test typed properties float widen at runtime > [Zend/tests/type_declarations/typed_properties_027.phpt] > Test typed properties respect strict types (off) > [Zend/tests/type_declarations/typed_properties_028.phpt] > Test typed properties unset __get magical magic > [Zend/tests/type_declarations/typed_properties_030.phpt] valgrind shows problems on every 4-rd test at Zend/tests (454 of 1668 tests), but it seems like a single problem $ cat ../Zend/tests/add_002.mem ==26053== Conditional jump or move depends on uninitialised value(s) ==26053== at 0x8627895: _zend_object_has_type_hints (zend_execute.h:367) ==26053== by 0x8629098: zend_std_write_property (zend_object_handlers.c:674) ==26053== by 0x85FA1B5: zend_update_property (zend_API.c:3894) ==26053== by 0x85FA38E: zend_update_property_string (zend_API.c:3952) ==26053== by 0x860F8DB: zend_default_exception_new_ex (zend_exceptions.c:222) ==26053== by 0x860F96F: zend_default_exception_new (zend_exceptions.c:236) ==26053== by 0x85F347E: _object_and_properties_init (zend_API.c:1303) ==26053== by 0x85F34B5: _object_init_ex (zend_API.c:1311) ==26053== by 0x86124F4: zend_throw_exception (zend_exceptions.c:881) ==26053== by 0x85EF256: zend_throw_error (zend.c:1316) ==26053== by 0x85E4622: add_function (zend_operators.c:969) ==26053== by 0x868824E: ZEND_ADD_SPEC_CV_CV_HANDLER (zend_vm_execute.h:44246) Thanks. Dmitry.