Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123511
Message-ID: <ab83af79-0669-47dd-a3cb-ab72327ae174@bastelstu.be>
Date: Tue, 4 Jun 2024 21:16:30 +0200
Precedence: bulk
MIME-Version: 1.0
Subject: Re: [PHP-DEV] [RFC] Lazy Objects
To: Nicolas Grekas <nicolas.grekas+php@gmail.com>,
 PHP Internals List <internals@lists.php.net>,
 Arnaud Le Blanc <arnaud.lb@gmail.com>
References: <CAOWwgpmbq5VRrZQvUXDsKiNK4r6+bFA4VxnjQ_U=h8T9r0o3DA@mail.gmail.com>
Content-Language: en-US
In-Reply-To: <CAOWwgpmbq5VRrZQvUXDsKiNK4r6+bFA4VxnjQ_U=h8T9r0o3DA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=C3=BCsterhus?=)

Hi

On 6/4/24 14:28, Nicolas Grekas wrote:
> Please find all the details here:
> https://wiki.php.net/rfc/lazy-objects
> 
> We look forward to your thoughts and feedback.

I've gave the RFC three or four passes and I'm not quite sure if I 
follow everything, here's a list of some questions / remarks that came 
to mind, roughly ordered by the order of things appearing in the RFC.

- "been tested successfully on the Doctrine and on the Symfony projects"

Is there a PoC patch showcasing how the code would change / be 
simplified for those pre-existing codebases?

- int $options = 0

Not a fan of flag parameters that take a bitset, those provide for a 
terrible DX due to magic numbers. Perhaps make this a regular (named) 
parameter, or an list of enum LazyObjectOptions { case 
SkipInitOnSerialize; }?

- skipProperty()

Not a fan of the method name, because it doesn't really say what it 
does, without consulting the docs. Perhaps `skipInitializationFor()` or 
similar?

- setProperty()

Not a fan of the method name, because it is not a direct counterpart to 
`getProperty()`. Unfortunately I don't have a better suggestion.

- The examples should be expanded and clarified, especially the one for 
makeLazyProxy():

My understanding is that the $object that is passed to the first 
parameter of makeLazyProxy() is completely replaced. Is this 
understanding correct? What does that mean for spl_object_hash(), 
spl_object_id()? What does this mean for WeakMap and WeakReference? What 
does this mean for objects that are only referenced from within $object?

Consider this example:

     class Foo {
       public function __destruct() { echo __METHOD__; }
     }

     class Bar {
       public string $s;
       public ?Foo $foo;

      public function __destruct() { echo __METHOD__; }
     }

     $bar = new Bar();
     $bar->foo = new Foo();

     ReflectionLazyObject::makeLazyProxy($bar, function (Bar $bar) {
       $result = new Bar();
       $result->foo = null;
       $result->s = 'init';
       return $result;
     });

     var_dump($bar->s);

My understanding is that this will dump `string(4) "init"`. Will the 
destructor of Foo be called? Will the destructor of Bar be called?

- What happens if I make an object lazy that already has all properties 
initialized? Will that be a noop? Will that throw? Will that create a 
lazy object that will never automatically be initialized?

- Cloning, unless __clone() is implemented and accesses a property.

The semantics of cloning a lazy object should be explicitly spelled out 
in the RFC, ideally with an example of the various edge cases (should 
any exist).

- Before calling the initializer, properties that were not initialized 
with ReflectionLazyObject::skipProperty(), 
ReflectionLazyObject::setProperty(), 
ReflectionLazyObject::setRawProperty() are initialized to their default 
value.

Should skipProperty() also skip the initialization to the default value? 
My understanding is that it allows skipping the initialization on 
access, but when initialization actually happens it should probably be 
set to a well-defined value, no?

Am I also correct in my understanding that this should read "initialized 
to their default value (if any)", meaning that properties without a 
default value are left uninitialized?

- If an exception is thrown while calling the initializer, the object is 
reverted to its pre-initialization state and is still considered lazy.

Does this mean that the initializer will be called once again when 
accessing another property? Will the "revert to its pre-initialization" 
work properly when you have nested lazy objects? An example would 
probably help.

- The initializer is called with the object as first parameter.

What is the behavior of accessing the object properties, while the 
initializer is active? Based on the examples, I assume it will not be 
recursively called, similarly to how the hooks work?

- The object is marked as non-lazy and the initializer is released.

What does it mean for the initializer to be released? Consider the 
following example:

     ReflectionLazyObject::makeLazyGhost($o, $init = function ($o) use 
(&$init) {
       $o->init = $init;
     });

- The return value of the initializer has to be an instance of a parent 
or a child class of the lazy-object and it must have the same properties.

Would returning a parent class not violate the LSP? Consider the 
following example:

     class A { public string $s; }
     class B extends A { public function foo() { } }

     $o = new B();
     ReflectionLazyObject::makeLazyProxy($o, function (B $o) {
       return new A();
     });

     $o->foo(); // works
     $o->s = 'init';
     $o->foo(); // breaks

- The destructor of lazy non-initialized objects is not called.

That sounds unsafe. Consider the following example:

     class Mutex {
       public string $s;
       public function __construct() {
         // take lock
       }

       public function __destruct() {
         // release lock
       }
     }

     $m = new Mutex();
     ReflectionLazyObject::makeLazyGhost($m, function ($m) {
     });
     unset($m); // will not release the lock.

- Using the Manager::createManager() factory is not compatible with 
ghost objects because their initializer requires initializing the ghost 
object in place,

I don't understand that example, because it doesn't actually use lazy 
objects and thus I don't understand if Manager, Dispatcher, or both are 
intended to be lazily initialized. It would help to rewrite the example 
to use `makeLazyGhost()` and indicate with a comment in which cases the 
problem would arise.

- Backward Incompatible Changes

There a technicality: The `ReflectionLazyObject` class name will no 
longer be available to userland.

Best regards
Tim Düsterhus