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 is_initialized #7029

Closed
wants to merge 1 commit into from
Closed

Conversation

krakjoe
Copy link
Member

@krakjoe krakjoe commented May 22, 2021

Determine the state of a typed property without using reflection

Fixes #78480

@krakjoe krakjoe force-pushed the is_initialized branch 4 times, most recently from 6d8c539 to ef7bede Compare May 22, 2021 07:33
  Determine the state of a property without using reflection

  Fixes #78480
@@ -1393,6 +1393,8 @@ function boolval(mixed $value): bool {}

function strval(mixed $value): string {}

function is_initialized(object|string $scope, string $property) : bool {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe property_is_initialized( object|string $object_or_class , string $property ) would make more sense as a name

is_initialized may be confused with other functions/expressions on variables such as is_null, isset, etc. property_exists ( object|string $object_or_class , string $property ) : bool

Also, my personal preference would be to have this work for untyped properties as well.
For untyped properties, it would be a bit shorter than https://www.php.net/manual/en/reflectionproperty.isinitialized.php to check if a property was initialized, and also work with dynamic properties.

  • (e.g. allowing this to work for untyped properties would make distinguishing between $property->dynamicProperty being null and unset shorter)
  • ReflectionProperty->isInitialized seems to permit dynamic and untyped properties
php > class TestClass { public $declared; public int $typed; }
php > $o = new TestClass();
php > unset($o->declared);
php > $o->dynamic = 'dynamic value';
php > var_export((new ReflectionProperty($o, 'dynamic'))->isInitialized($o));
true
php > var_export((new ReflectionProperty($o, 'declared'))->isInitialized($o));
false

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hesitate to widen the scope so far for two reasons: First it means necessarily using handlers, which are going to be slower - not much better than just using the existing mechanism, in my opinion. Second, other than typed properties won't raise exceptions when you try to determine their state by normal means.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it work to only use slow handlers if the property wasn't defined (i.e. instead of throwing, check the handlers first)?

I would have thought the fast path could be used for declared properties, and the slow path for dynamic properties, but I'm not 100% sure if there aren't any extra edge cases (e.g. public dynamic properties overriding a declared private property)

Second, other than typed properties won't raise exceptions when you try to determine their state by normal means.

non-typed declared instance properties would emit notices when you were trying to differentiate between null and undefined by normal means - property_exists would return true for all declared properties even after unsetting them

Copy link
Member Author

@krakjoe krakjoe May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have thought the fast path could be used for declared properties, and the slow path for dynamic properties, but I'm not 100% sure if there aren't any extra edge cases (e.g. public dynamic properties overriding a declared private property)

Indeed it would introduce edges, which I didn't consider when I said perf would be the reason I didn't want to use them ...

I didn't use them to keep the scope of the function narrow ...

The reflector works the way it does because it has no choice, precisely because it's using handlers. I think using handlers and so having to widen scope confuses the purpose of the function, the purpose of the function is very narrowly to check if a typed property is initialized in order to avoid exception in normal code. It also makes it less valuable because you already have reflection to do exactly this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware of a possible problem with avoiding handlers ... an internal class may declare typed properties and handlers that do not use pre-allocated property slots.

I think it's acceptable, because any object doing that is not behaving correctly (and would seem to be wasting memory), and if/when found we probably should consider that a bug in the handlers.

If it turns out we must use handlers, I'd rather not have the function, because you already have reflection and there doesn't seem to be a win ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm aware of a possible problem with avoiding handlers ... an internal class may declare typed properties and handlers that do not use pre-allocated property slots.

I think it's acceptable, because any object doing that is not behaving correctly (and would seem to be wasting memory), and if/when found we probably should consider that a bug in the handlers.

We've actually started doing this a lot recently, so that these properties end up in reflection.

@krakjoe krakjoe requested a review from nikic May 24, 2021 08:10
@nikic
Copy link
Member

nikic commented May 25, 2021

I think it would be good to start an internals discussion for this one. I'm not completely convinced that this is something we want to add (and thus endorse). I do see some use-cases, but I'd also rather avoid the "always use array_key_exists instead of isset to "correctly" handle null" stupidity that this may lead people down to.

@krakjoe
Copy link
Member Author

krakjoe commented May 25, 2021

When we designed the behaviour for uninitialized typed properties I didn't see it leading here, I have this niggling feeling that normal code is probably logically inconsistent if it's attempting to access uninitialized properties. Having said that, allowing unsetting means it may be difficult to model precisely the state of a property in code, especially where a property may be public (yuk, but we support it).

So yeah, wider feedback seems to be the thing to do ... thanks for input ...

@kapitancho
Copy link

This is what I can say as an author of the ticket:

The major scenarios where one would use a function like is_initialized is for lazy loading or caching.
It is clear that isset works perfectly well for typed properties that cannot not contain NULL but if the type is like ?int or string|float|null or simply mixed then it gets complicated.
Using reflection for this simple case is simply ugly. As we know we have property_exists, get_class_methods and many other functions so it is not always necessary to go through reflection.

@oprypkhantc
Copy link

@kapitancho It doesn't make sense to add even more magic into PHP (which reflection and this are) for use-cases that should be handled in other ways. For example, Kotlin solves lazy loading/caching with it's Lazy interface and adds a tiny bit of syntax sugar for properties so it doesn't need to be unwrapped. That's a much better solution than putting is_initialized all over your codebase. For now, you could just use Lazy object without any additional syntax and manually unwrap it as you go. I don't see how this:

private ?int $someProp;

private function someProp(): int
{
    if (!is_initialized($this, 'someProp')) {
        $this->someProp = random_int(0, 123);
    }
    
    return $this->someProp;
}

is any better than what you can do right now:

/** @var Lazy<int> */
private Lazy $someProp;

public function __construct()
{
    $this->someProp = lazy(fn () => random_int(0, 123));
}

which becomes even more concise with "new in initializers":

/** @var Lazy<int> */
private Lazy $someProp = new CallableLazy(fn () => random_int(0, 123));

@nikic
Copy link
Member

nikic commented May 27, 2021

Mailing list thread: https://externals.io/message/114607

@nikic nikic added the RFC label May 27, 2021
@nikic
Copy link
Member

nikic commented May 27, 2021

As the ML reception has been fairly negative, I think we can safely say that this would require an RFC to be added.

@krakjoe
Copy link
Member Author

krakjoe commented May 27, 2021

I'll go one further, an RFC wouldn't really be appropriate because the reception was so negative. That consensus is negative is enough for me to close the PR and FR as won't fix.

If anyone else wants to pursue an RFC, they are free to do so with this or any other implementation, but for now I'm satisfied that all questions have been answered and we are not going down this path.

@krakjoe krakjoe closed this May 27, 2021
@kapitancho
Copy link

@kapitancho It doesn't make sense to add even more magic into PHP (which reflection and this are) for use-cases that should be handled in other ways.

It would have been magic if it were possible to call is_initialized($obj->prop) without getting a type error. is_initialized($obj, 'prop') is not magic at all. So in fact the suggested solution brings no real value.

@autaut03
Copy link

@kapitancho It doesn't make sense to add even more magic into PHP (which reflection and this are) for use-cases that should be handled in other ways.

It would have been magic if it were possible to call is_initialized($obj->prop) without getting a type error. is_initialized($obj, 'prop') is not magic at all. So in fact the suggested solution brings no real value.

Acessing internal property state by it's name isn't good, regardless of what you call it. Aside from specific use cases, you shouldn't have property name as a string anywhere in your codebase. This isn't such use case.

@kapitancho
Copy link

@kapitancho It doesn't make sense to add even more magic into PHP (which reflection and this are) for use-cases that should be handled in other ways.

It would have been magic if it were possible to call is_initialized($obj->prop) without getting a type error. is_initialized($obj, 'prop') is not magic at all. So in fact the suggested solution brings no real value.

Acessing internal property state by it's name isn't good, regardless of what you call it. Aside from specific use cases, you shouldn't have property name as a string anywhere in your codebase. This isn't such use case.

Well, I clearly state in the bugs.php.net ticket that I am talking about $obj->prop.
is_initialized($obj, 'prop') is what we have in this pull request and I agree that it is not a good practice to work with string property names. So this pull request is just an interpretation of what I mean in this feature request.

hemberger added a commit to hemberger/smr that referenced this pull request Jan 26, 2023
This is an optimization fix. No functionality changes.

When we first call `Player::getPlottedCourse`, we cache the result of
the database query, even if the Player has no plotted course, so that
we don't need to perform the query again on the next call. To check if
we've set the cache, we check `isset($this->plottedCourse)`; however,
this returns false for both uninitialized _and_ null (a deficiency in
the PHP language, see php/php-src#7029).

Therefore, we must use `false` as the "already queried but no value"
state (barring the addition of lazy-loading/caching infrastructure).

This reverts b9ad390, which was causing the database query to be
repeated many times in, e.g., displaying the Local/Galaxy map.

We add a PHPStan assertion to `Player::hasPlottedCourse` to narrow the
return type of `Player::getPlottedCourse`.
@realjjaveweb
Copy link

I need to put my 2 cents here...

I have absolutely no problem with is_initialized not existing, but only if it is forbidden to create objects with uninitialized properties (with no default value).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants