Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:123515
Precedence: bulk
MIME-Version: 1.0
References: <CAOWwgpmbq5VRrZQvUXDsKiNK4r6+bFA4VxnjQ_U=h8T9r0o3DA@mail.gmail.com>
 <ab83af79-0669-47dd-a3cb-ab72327ae174@bastelstu.be>
In-Reply-To: <ab83af79-0669-47dd-a3cb-ab72327ae174@bastelstu.be>
Date: Wed, 5 Jun 2024 14:33:49 +0200
Message-ID: <CAPzBOBNi4XGi6S3PNUZ14=NftTamYpbo_NOLBex6bDoYSG-X+w@mail.gmail.com>
Subject: Re: [PHP-DEV] [RFC] Lazy Objects
To: =?UTF-8?Q?Tim_D=C3=BCsterhus?= <tim@bastelstu.be>
Cc: Nicolas Grekas <nicolas.grekas+php@gmail.com>, 
	PHP Internals List <internals@lists.php.net>, Arnaud Le Blanc <arnaud.lb@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
From: landers.robert@gmail.com (Robert Landers)

On Tue, Jun 4, 2024 at 10:23=E2=80=AFPM Tim D=C3=BCsterhus <tim@bastelstu.b=
e> wrote:
>
> 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 =3D 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 =3D new Bar();
>      $bar->foo =3D new Foo();
>
>      ReflectionLazyObject::makeLazyProxy($bar, function (Bar $bar) {
>        $result =3D new Bar();
>        $result->foo =3D null;
>        $result->s =3D '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 =3D function ($o) use
> (&$init) {
>        $o->init =3D $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 =3D new B();
>      ReflectionLazyObject::makeLazyProxy($o, function (B $o) {
>        return new A();
>      });
>
>      $o->foo(); // works
>      $o->s =3D '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 =3D 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=C3=BCsterhus

As someone who has had to maintain these proxies/ghosts before, this
looks quite useful and powerful. I feel it has rather wonky syntax,
but it is clearly better than the alternative of implementing it
yourself. I'm also a huge fan that the syntax allows for
usage/creation far and away from the definition/class itself.

Good luck, and I hope it passes.

Robert Landers
Software Engineer
Utrecht NL