Skip to content
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

Implement typed properties #3734

Closed
wants to merge 56 commits into from
Closed

Implement typed properties #3734

wants to merge 56 commits into from

Conversation

@nikic
Copy link
Member

nikic commented Jan 7, 2019

RFC: https://wiki.php.net/rfc/typed_properties_v2

This is a squashed version of #3313 to make final review easier. The previous thread has become unwieldy.

@nikic nikic added the RFC label Jan 7, 2019
Copy link
Member

carusogabriel left a comment

@nikic From the tests part, just a couple of tests, for example, Zend/tests/type_declarations/typed_properties_019.phpt, that have a fell extra lines at the end of the lines. I'll run this implementation in some OSS projects, see if I can found some missing tests 👍

@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Jan 7, 2019

@carusogabriel I've added some basic property type support to https://github.com/nikic/TypeUtil for this purpose. For now I've only tested that PHP-Parser works after automatically annotating it with property types (needs some manual fixup to fix inaccurate @var annotations and different initialization semantics of typed properties). Would be great to do these kinds of tests on more projects.

@nikic nikic mentioned this pull request Jan 7, 2019
@nikic

This comment was marked as resolved.

Copy link
Member Author

nikic commented Jan 8, 2019

Another fun case:

<?php
  
class Test {
    public ?int $prop = null;
}

$test = new Test;
try {
    $test->prop->foo = 123;
} catch (TypeError $e) {
    echo $e->getMessage(), "\n"; // Cannot auto-initialize an stdClass inside property Test::$prop of type ?int
}
var_dump($test->prop); // $test->prop is uninitialized here :/

At the point where we generate the invalid auto-initialization error we don't know anymore whether the property was originally undefined or not. Right now we're always undefing it, but that's not right. The same issue also exists for array auto-initialization.

@nikic

This comment was marked as resolved.

Copy link
Member Author

nikic commented Jan 9, 2019

This code currently doesn't throw an error:

<?php
  
class Test {
    private int $prop;

    public function __get($name) {
        return "foobar";
    }
}

$test = new Test;
var_dump($test->prop);

Not totally sure whether or not it should. We do throw an error if the property is visible but undefined.

@bwoebi

This comment was marked as resolved.

Copy link
Contributor

bwoebi commented Jan 9, 2019

The following code does not error currently:

<?php

class Test {
    private $prop;

    public function __construct() {
        unset($this->prop);
    }

    public function __get($name) {
        return "foobar";
    }
}

$test = new Test;
var_dump($test->prop);

And as uninitialized typed properties mirror the behavior of unset() untyped properties, this should not error either.

@nikic

This comment was marked as resolved.

Copy link
Member Author

nikic commented Jan 9, 2019

@bwoebi What I had in mind here is that this does error:

<?php
  
class Test {
    private int $prop;

    public function __get($name) {
        return "foobar";
    }
}

$test = new Test;
var_dump($test->prop);
nikic and others added 22 commits Jan 7, 2019
RFC: https://wiki.php.net/rfc/typed_properties_v2

Co-authored-by: Bob Weinand <bobwei9@hotmail.com>
Co-authored-by: Joe Watkins <krakjoe@php.net>
Co-authored-by: Dmitry Stogov <dmitry@zend.com>
Add two more macros ZEND_TRY_ASSIGN_VALUE and ZEND_TRY_ASSIGN_COPY
to assign zvals and replace all remaining uses of the internal
API.

Switch the API to not use a temporary zval anymore, as all users
of the API take care to do this themselves.
I found the 1+count here much more confusing than helpful.
And add tests for typed properties + references + undefined classes.
I think the behavior here is not correct and we should treat this the
same way we do elsewhere (if the class can't be loaded, conclude that
it can't be an instance of the class, but don't issue an error).

Going back to this behavior for now to avoid an assertion failure...
This is probably going to need changes in a few more places that
currently implicitly assume only CE types can occur.
If the value_type is not known at compile-time, don't perform checks
for whether the copy is needed or not, always do it. Also make sure
we free the copy in the error case...
If we're in a class that has type hints, actually seeing a type is
not unexpected.
This makes it consistent with the function for ref types and avoids
having to handle these differently.
And remove some code handling future possibilities where a property
could be both int and double. I don't like having untestable code
around. Let's deal with this fully once we have union types.
And remove some redundant checks.
nikic added 10 commits Jan 9, 2019
We don't want to create dependencies between zend_execute and
zend_inheritance...
Made a mistake here when refactoring the code, causing a failure
in serialization test.
Based on the descision that we do not want to enforce __get() return
types for inaccessible propreties.
It's not relevant in any other case and might be wrong wrt cache
state I think.
We have the proprerty offset here and know it's valid, so we can
use our property helper table to fetch it, rather than doing a
full property info lookup.
By switching to the slot API, which doesn't need unmangled keys
and is faster anyway...

Also moving those APIs to a place that has a complete prop_info
def.
I originall thought this wasn't possible, but now that it turned out
that object_handlers also always has slots available, we can do this.
Otherwise we can probably run into the same issues here as we could
with destruction.
@nikic nikic force-pushed the nikic:typed_props branch from 7ae0a3e to 1005f92 Jan 10, 2019
nikic added 15 commits Jan 10, 2019
Used in conjunction with try_array_init, can't deref beforehand.
I'm explicitly passing the object to process_nested_data now.
When I tried to use Z_OBJ_P(rval) things started failing in odd
ways and I have no idea why. The variable doesn't seem to be
overwritten...
It's rather unlikely that these will be available as CEs at this
point, we need to look them up.
I don't think it can happen right now, but will once there is full
preloading support for this.
php-pulls pushed a commit that referenced this pull request Jan 11, 2019
RFC: https://wiki.php.net/rfc/typed_properties_v2

This is a squash of PR #3734, which is a squash of PR #3313.

Co-authored-by: Bob Weinand <bobwei9@hotmail.com>
Co-authored-by: Joe Watkins <krakjoe@php.net>
Co-authored-by: Dmitry Stogov <dmitry@zend.com>
@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Jan 11, 2019

Merged via e219ec1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.