New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typed Properties #3313

Open
wants to merge 384 commits into
base: master
from

Conversation

@nikic
Member

nikic commented Jun 20, 2018

This is the implementation for the new typed properties RFC.

The implementation is not quite ready for review yet, there are still a number of open issues and some behavior edge-cases regarding references may still change.

krakjoe and others added some commits Apr 7, 2016

Merge branch 'master' of https://github.com/php/php-src into typed-pr…
…operties

Conflicts:
	Zend/zend_execute.c
	Zend/zend_vm_execute.h
Merge branch 'master' into typed-properties
* master:
  Fixed white-spaces
Fixed Zend/tests/type_declarations/typed_properties_044.phpt (error m…
…essage should be the same independently of called function)
Removed "safety" code for non standard zend objects.
If non-standard objects implement typed properties, they should care about type compatibility checks their seves.
@nikic

This comment was marked as resolved.

Show comment
Hide comment
@nikic

nikic Sep 28, 2018

Member

There is currently an issue with traits + static properties + self:

trait Test {
    public static self $prop;
}

try {
    Test::$prop = new stdClass; // No error if this line is commented
} catch (Error $e) {
    echo $e->getMessage(), "\n";
}

class Foo {
    use Test;
}

Foo::$prop = new Foo;
// Uncaught TypeError: Typed property Foo::$prop must be an instance of Test, Foo used

The problem is that we end up resolving the self type on the trait before the trait is imported in the class.

Member

nikic commented Sep 28, 2018

There is currently an issue with traits + static properties + self:

trait Test {
    public static self $prop;
}

try {
    Test::$prop = new stdClass; // No error if this line is commented
} catch (Error $e) {
    echo $e->getMessage(), "\n";
}

class Foo {
    use Test;
}

Foo::$prop = new Foo;
// Uncaught TypeError: Typed property Foo::$prop must be an instance of Test, Foo used

The problem is that we end up resolving the self type on the trait before the trait is imported in the class.

@@ -43,9 +43,6 @@ var_dump( lchown( 'foobar_lchown.txt', $uid ) );
var_dump( lchown( new StdClass(), $uid ) );
var_dump( lchown( array(), $uid ) );
// Bad user
var_dump( lchown( $filename, -5 ) );

This comment has been minimized.

@nikic

nikic Sep 28, 2018

Member

What's the reason for this test change? Just an unrelated fix?

Edit: Apparently yes 0cc95b8.

@nikic

nikic Sep 28, 2018

Member

What's the reason for this test change? Just an unrelated fix?

Edit: Apparently yes 0cc95b8.

This comment has been minimized.

@bwoebi

bwoebi Sep 29, 2018

Contributor

Yeah, one should just commit that to master ... No idea why I pushed it here.

@bwoebi

bwoebi Sep 29, 2018

Contributor

Yeah, one should just commit that to master ... No idea why I pushed it here.

Show outdated Hide outdated ext/standard/var.c Outdated
(*var_hash)->refs = emalloc(sizeof(HashTable));
zend_hash_init((*var_hash)->refs, 8, NULL, ZVAL_PTR_DTOR, 0);
}
data = zend_hash_next_index_insert((*var_hash)->refs, &d);

This comment has been minimized.

@nikic

nikic Sep 28, 2018

Member

This is probably a leftover from the previous proposal which did not permit references. We should be allowing refs here but verify their types.

@nikic

nikic Sep 28, 2018

Member

This is probably a leftover from the previous proposal which did not permit references. We should be allowing refs here but verify their types.

Show outdated Hide outdated ext/reflection/php_reflection.c Outdated
Show outdated Hide outdated ext/reflection/php_reflection.c Outdated
@nikic

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic Sep 29, 2018

Member

It's possible to circumvent the property type check using the implicit null to object conversion:

class Test {
    public ?Test $prop;
}

$test = new Test;
$test->prop->wat = 123;
var_dump($test);
Warning: Creating default object from empty value in /home/nikic/php-src-refs/t060.php on line 8
object(Test)#1 (1) {
  ["prop"]=>
  object(stdClass)#2 (1) {
    ["wat"]=>
    int(123)
  }
}
Member

nikic commented Sep 29, 2018

It's possible to circumvent the property type check using the implicit null to object conversion:

class Test {
    public ?Test $prop;
}

$test = new Test;
$test->prop->wat = 123;
var_dump($test);
Warning: Creating default object from empty value in /home/nikic/php-src-refs/t060.php on line 8
object(Test)#1 (1) {
  ["prop"]=>
  object(stdClass)#2 (1) {
    ["wat"]=>
    int(123)
  }
}
Show outdated Hide outdated Zend/zend_object_handlers.c Outdated
@nikic

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic Sep 30, 2018

Member

A case with missing unmangling:

class A {
    private int $prop = 42;

    public function test() {
        $x =& $this->prop;
        $x = "foo";
        var_dump($x);
    }
}

$a = new A;
$a->test();

Currently prints "Cannot assign string to reference held by property A::$ of type int" (missing property name).

Member

nikic commented Sep 30, 2018

A case with missing unmangling:

class A {
    private int $prop = 42;

    public function test() {
        $x =& $this->prop;
        $x = "foo";
        var_dump($x);
    }
}

$a = new A;
$a->test();

Currently prints "Cannot assign string to reference held by property A::$ of type int" (missing property name).

Show outdated Hide outdated Zend/zend_object_handlers.c Outdated
Show outdated Hide outdated Zend/zend_object_handlers.c Outdated
Show resolved Hide resolved Zend/zend_object_handlers.c
Show outdated Hide outdated Zend/zend_object_handlers.c Outdated
if (UNEXPECTED(Z_ISREF_P(p))) {
zend_property_info *prop_info;
ZEND_REF_FOREACH_TYPE_SOURCES(Z_REF_P(p), prop_info) {
if (prop_info->ce == object->ce && p == OBJ_PROP(object, prop_info->offset)) {

This comment has been minimized.

@nikic

nikic Sep 30, 2018

Member

The CE check here is not right if the property is inherited. The following code generates a TypeError, even though it shouldn't.

class A {
    public int $prop = 42;
}
class B extends A {
}

$b = new B;
$r =& $b->prop;
unset($b);
$r = "foo";
@nikic

nikic Sep 30, 2018

Member

The CE check here is not right if the property is inherited. The following code generates a TypeError, even though it shouldn't.

class A {
    public int $prop = 42;
}
class B extends A {
}

$b = new B;
$r =& $b->prop;
unset($b);
$r = "foo";

This comment has been minimized.

@nikic

nikic Sep 30, 2018

Member

Another test-case involving shadowing:

class A {
    private int $prop = 42;

    public function &getRef() {
        return $this->prop;
    }
}
class B extends A {
    public $prop;
}

$b = new B;
$r =& $b->getRef();
unset($b);
$r = "foo";

I think that just not checking the CE should be safe. Acquiring an OOB pointer is technically UB but it should not cause miscompiles here, as we're in struct hack land anyway.

@nikic

nikic Sep 30, 2018

Member

Another test-case involving shadowing:

class A {
    private int $prop = 42;

    public function &getRef() {
        return $this->prop;
    }
}
class B extends A {
    public $prop;
}

$b = new B;
$r =& $b->getRef();
unset($b);
$r = "foo";

I think that just not checking the CE should be safe. Acquiring an OOB pointer is technically UB but it should not cause miscompiles here, as we're in struct hack land anyway.

This comment has been minimized.

@nikic

nikic Sep 30, 2018

Member

On a related note, I think it would be useful if zend_reference_destroy() checked if there are any sources left in debug mode and print a message. That should make it much easier to discover cases where we are not properly deleting references.

@nikic

nikic Sep 30, 2018

Member

On a related note, I think it would be useful if zend_reference_destroy() checked if there are any sources left in debug mode and print a message. That should make it much easier to discover cases where we are not properly deleting references.

This comment has been minimized.

@bwoebi

bwoebi Sep 30, 2018

Contributor

OBJ_PROP(object, prop_info->offset) may simply point into unallocated memory (or that location be accidentally equal to p), and then it will blow up. Thus checking is necessary in any case.

So, looks like it's going to need an instanceof check

@bwoebi

bwoebi Sep 30, 2018

Contributor

OBJ_PROP(object, prop_info->offset) may simply point into unallocated memory (or that location be accidentally equal to p), and then it will blow up. Thus checking is necessary in any case.

So, looks like it's going to need an instanceof check

This comment has been minimized.

@nikic

nikic Sep 30, 2018

Member

Hm... you are right that it could accidentally equal p. I'm not sure if a simple instanceof will cut it though. In particular I'm thinking about the case where a property is redeclared in a child class, which IIRC shares the offset. So there might be to prop_infos with the same offset coming from two different classes where one is a child of the other. Not totally sure though.

@nikic

nikic Sep 30, 2018

Member

Hm... you are right that it could accidentally equal p. I'm not sure if a simple instanceof will cut it though. In particular I'm thinking about the case where a property is redeclared in a child class, which IIRC shares the offset. So there might be to prop_infos with the same offset coming from two different classes where one is a child of the other. Not totally sure though.

This comment has been minimized.

@bwoebi

bwoebi Oct 1, 2018

Contributor

Yeah, just checked, properties which are protected in parent and public in child will generate two different properties with the same offset.

It actually is indistinguishable at this point whether the particular reference here has been created by the child or the parent.

Consider:

class A {
    protected int $prop;
    public setRef(&$ref) {
        $this->prop = &$ref;
    }
}
class B extends A {
    public int $prop;
}
$a = new A;
$b = new B;
$bb = new B;
$ref = 1;
$a->setRef($ref);
$b->setRef($ref);
$bb->prop = &$ref;

Now we have twice A::$prop and once B::$prop in the reference set. Without additional information, there is no way to know which one to remove when destroying $b or $bb...
The lacking piece of information would be the actual zend_object * pointer for non-static properties. Then we could easily compare the object pointers and we then know that it's safe to remove.

@bwoebi

bwoebi Oct 1, 2018

Contributor

Yeah, just checked, properties which are protected in parent and public in child will generate two different properties with the same offset.

It actually is indistinguishable at this point whether the particular reference here has been created by the child or the parent.

Consider:

class A {
    protected int $prop;
    public setRef(&$ref) {
        $this->prop = &$ref;
    }
}
class B extends A {
    public int $prop;
}
$a = new A;
$b = new B;
$bb = new B;
$ref = 1;
$a->setRef($ref);
$b->setRef($ref);
$bb->prop = &$ref;

Now we have twice A::$prop and once B::$prop in the reference set. Without additional information, there is no way to know which one to remove when destroying $b or $bb...
The lacking piece of information would be the actual zend_object * pointer for non-static properties. Then we could easily compare the object pointers and we then know that it's safe to remove.

This comment has been minimized.

@nikic

nikic Oct 1, 2018

Member

Now we have twice A::$prop and once B::$prop in the reference set. Without additional information, there is no way to know which one to remove when destroying $b or $bb...
The lacking piece of information would be the actual zend_object * pointer for non-static properties. Then we could easily compare the object pointers and we then know that it's safe to remove.

I don't think that's actually true. Even if the property is accessed from inside A, it should still use the prop_info from B (if that's the property actually accessed). Checked with this code:

class A {
    protected int $prop;
    public function setRef(&$ref) {
        $this->prop = &$ref;
    }
}
class B extends A {
    public int $prop;
}

$ref = 42;
$b = new B;
$b->setRef($ref);
$ref = "not an int";
// Cannot assign string to reference held by property B::$prop of type int

Note that it says B::$prop, not A::$prop. So I think we should be able to find the right one based on property_info and class of the object alone.

@nikic

nikic Oct 1, 2018

Member

Now we have twice A::$prop and once B::$prop in the reference set. Without additional information, there is no way to know which one to remove when destroying $b or $bb...
The lacking piece of information would be the actual zend_object * pointer for non-static properties. Then we could easily compare the object pointers and we then know that it's safe to remove.

I don't think that's actually true. Even if the property is accessed from inside A, it should still use the prop_info from B (if that's the property actually accessed). Checked with this code:

class A {
    protected int $prop;
    public function setRef(&$ref) {
        $this->prop = &$ref;
    }
}
class B extends A {
    public int $prop;
}

$ref = 42;
$b = new B;
$b->setRef($ref);
$ref = "not an int";
// Cannot assign string to reference held by property B::$prop of type int

Note that it says B::$prop, not A::$prop. So I think we should be able to find the right one based on property_info and class of the object alone.

This comment has been minimized.

@bwoebi

bwoebi Oct 1, 2018

Contributor

Ah. Okay, at least we have that :-D

This is annoying though, we'll have to iterate over all the property infos ... And check whose ce is the closest to the object ce.

I'm not sure what the nicest way to implement that is...

@bwoebi

bwoebi Oct 1, 2018

Contributor

Ah. Okay, at least we have that :-D

This is annoying though, we'll have to iterate over all the property infos ... And check whose ce is the closest to the object ce.

I'm not sure what the nicest way to implement that is...

if (UNEXPECTED(Z_ISREF_P(p))) {
zend_property_info *prop_info;
ZEND_REF_FOREACH_TYPE_SOURCES(Z_REF_P(p), prop_info) {
if (prop_info->ce == ce && p - ce->default_static_members_table == prop_info->offset) {

This comment has been minimized.

@nikic

nikic Sep 30, 2018

Member

Same issue with CE checks.

@nikic

nikic Sep 30, 2018

Member

Same issue with CE checks.

This comment has been minimized.

@bwoebi

bwoebi Oct 1, 2018

Contributor

Is there really an issue with statics? If I read the code correctly, the static variable is always stored alongside the actual defining class and you are not allowed to redeclare statics either (visibility change not possible), thus this should be fine.

@bwoebi

bwoebi Oct 1, 2018

Contributor

Is there really an issue with statics? If I read the code correctly, the static variable is always stored alongside the actual defining class and you are not allowed to redeclare statics either (visibility change not possible), thus this should be fine.

This comment has been minimized.

@nikic

nikic Oct 1, 2018

Member

I think you are basically right, but there might still be an issue if a static property is inherited from an internal class into a user class, in which case we will duplicate the property info.

@nikic

nikic Oct 1, 2018

Member

I think you are basically right, but there might still be an issue if a static property is inherited from an internal class into a user class, in which case we will duplicate the property info.

This comment has been minimized.

@bwoebi

bwoebi Oct 1, 2018

Contributor

This is correct, though we don't have internal typed static properties yet…
I think the solution would be to rather not duplicate static property type info. I see no reason why we are even doing that (probably because we need it for non-statics and it did no harm on statics).

@bwoebi

bwoebi Oct 1, 2018

Contributor

This is correct, though we don't have internal typed static properties yet…
I think the solution would be to rather not duplicate static property type info. I see no reason why we are even doing that (probably because we need it for non-statics and it did no harm on statics).

This comment has been minimized.

@bwoebi

bwoebi Oct 1, 2018

Contributor

@nikic Please check out master...bwoebi:prevent_property_info_duplication - this should keep this simple and it will also make the other check much simpler as we can simply assume there will be at most two different prop_infos pointing to the same property offset. And if there is a second one, it will be protected and the other one public.
It would simplify the whole loop to:

zend_property_info *prop_info, *protected_prop_info;
ZEND_REF_FOREACH_TYPE_SOURCES(Z_REF_P(p), prop_info) {
	if (p == OBJ_PROP(object, prop_info->offset) && instanceof_function(object->ce, prop_info->ce)) {
		if (prop_info->flags & (ZEND_ACC_PUBLIC | ZEND_ACC_PRIVATE)) {
			goto found;
		} else {
			protected_prop_info = prop_info;
		}
	}
} ZEND_REF_FOREACH_TYPE_SOURCES_END();
prop_info = protected_prop_info;
found:
ZEND_REF_DEL_TYPE_SOURCE(Z_REF_P(p), prop_info);
@bwoebi

bwoebi Oct 1, 2018

Contributor

@nikic Please check out master...bwoebi:prevent_property_info_duplication - this should keep this simple and it will also make the other check much simpler as we can simply assume there will be at most two different prop_infos pointing to the same property offset. And if there is a second one, it will be protected and the other one public.
It would simplify the whole loop to:

zend_property_info *prop_info, *protected_prop_info;
ZEND_REF_FOREACH_TYPE_SOURCES(Z_REF_P(p), prop_info) {
	if (p == OBJ_PROP(object, prop_info->offset) && instanceof_function(object->ce, prop_info->ce)) {
		if (prop_info->flags & (ZEND_ACC_PUBLIC | ZEND_ACC_PRIVATE)) {
			goto found;
		} else {
			protected_prop_info = prop_info;
		}
	}
} ZEND_REF_FOREACH_TYPE_SOURCES_END();
prop_info = protected_prop_info;
found:
ZEND_REF_DEL_TYPE_SOURCE(Z_REF_P(p), prop_info);

This comment has been minimized.

@nikic

nikic Oct 1, 2018

Member

@bwoebi I don't think I understand the purpose of that change. Is it only about the static property case?

At least right now (and that patch does not seem to change this), if you override a public property with another public property that will also result in a new property_info, it's not necessarily due to a visibility change.

@nikic

nikic Oct 1, 2018

Member

@bwoebi I don't think I understand the purpose of that change. Is it only about the static property case?

At least right now (and that patch does not seem to change this), if you override a public property with another public property that will also result in a new property_info, it's not necessarily due to a visibility change.

This comment has been minimized.

@bwoebi

bwoebi Oct 1, 2018

Contributor

Eih, I forgot about that. And we cannot just overwrite it because, well, the attached doc comment...
But still, it's applicable to the static property case.

@bwoebi

bwoebi Oct 1, 2018

Contributor

Eih, I forgot about that. And we cannot just overwrite it because, well, the attached doc comment...
But still, it's applicable to the static property case.

@nikic

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic Sep 30, 2018

Member

It could also have been made a reference in the meantime. We should probably just goto found here.

Member

nikic commented on Zend/zend_object_handlers.c in 1d555b7 Sep 30, 2018

It could also have been made a reference in the meantime. We should probably just goto found here.

This comment has been minimized.

Show comment
Hide comment
@bwoebi

bwoebi Sep 30, 2018

Contributor

I think it should overwrite the potential reference here, as it happens after resolving the actual locations to be assigned to.

Contributor

bwoebi replied Sep 30, 2018

I think it should overwrite the potential reference here, as it happens after resolving the actual locations to be assigned to.

This comment has been minimized.

Show comment
Hide comment
@nikic

nikic Sep 30, 2018

Member

That's true-ish. On the other hand destroying the reference is going to leave behind a prop_info source.

Member

nikic replied Sep 30, 2018

That's true-ish. On the other hand destroying the reference is going to leave behind a prop_info source.

This comment has been minimized.

Show comment
Hide comment
@bwoebi

bwoebi Sep 30, 2018

Contributor

Arghs. You're right...
Going with goto found; then

Contributor

bwoebi replied Sep 30, 2018

Arghs. You're right...
Going with goto found; then

bwoebi and others added some commits Sep 30, 2018

Merge branch 'master' into typed_ref_properties
* master:
  Rename ZEND_ACC_NO_RT_ARENA into ZEND_ACC_HEAP_RT_CACHE and use it for pseudo-main op_arrays.
  Allocate only necessary space for static properties of internal classes in ZTS mode.
  Turn accel_activate into module callback.
  Bump phpdbg version to PHP_VERSION
  Get rid of accel_deactivate() calback
  Fix the deplister rule to not ignore the .c file (Anatol)
  Update .gitignore to include the Windows deplister program (win32/build/deplister.c)
  Bug > Feature Request
  NEWS and UPGRADING
  Fixed bug #75479
  Fix test
  Remove dead code (only IS_ARRAY may relive zendi_convert_scalar_to_number()), and micro-optimization.
  Removing last unused
  Last few changes : 	. force the nls_date_format 	. add the scale to the return of the function 	. add tests on some function return 	. removing unused variables
  cs
  Changes : . Add the distinction between NUMBER and FLOAT types . Changing BFLOAT text to be BINARY_FLOAT . Changing BDOUBLE text to be BINARY_DOUBLE . Add the data types names for NCHAR, NVARCHAR and NCLOB . Few changes in the tests
  fixing comments
  Update oci_statement.c
  Add the PDOStatement::getColumnMeta() function to the pdo_oci driver
Merge branch 'master' into typed_ref_properties
* master:
  Make ZEND_ACC_IMMUTABLE and ZEND_ACC_HAS_TYPE_HINTS to be common (for functions and classes)

bwoebi and others added some commits Oct 4, 2018

return 0;
}
zend_string_release(ZEND_TYPE_NAME(info->type));
info->type = ZEND_TYPE_ENCODE_CE(ce, ZEND_TYPE_ALLOW_NULL(info->type));

This comment has been minimized.

@nikic

nikic Oct 17, 2018

Member

Now that immutable classes have landed, we can no longer do this. We either have to drop this caching, or switch it to use MAP.

@nikic

nikic Oct 17, 2018

Member

Now that immutable classes have landed, we can no longer do this. We either have to drop this caching, or switch it to use MAP.

This comment has been minimized.

@dstogov

dstogov Oct 17, 2018

Contributor

@nikic You may simple prevent classes to be immutable, if they have unresolved typed properties (in the same way as with unresolved constants).

@dstogov

dstogov Oct 17, 2018

Contributor

@nikic You may simple prevent classes to be immutable, if they have unresolved typed properties (in the same way as with unresolved constants).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment