Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:116973
User-Agent: Cyrus-JMAP/3.5.0-alpha0-4585-ga9d9773056-fm-20220113.001-ga9d97730
Mime-Version: 1.0
Message-ID: <3ae1a3f5-b4ea-40a0-b03f-3fa69ed6b9d2@www.fastmail.com>
In-Reply-To: 
 <DM6PR14MB4155E8620E5907BDA2320B7EF9279@DM6PR14MB4155.namprd14.prod.outlook.com>
References: 
 <DM6PR07MB66185E23B803C5E5BF858B30F9DF9@DM6PR07MB6618.namprd07.prod.outlook.com>
 <DM6PR14MB41556131E6A9D7812854AA75F9269@DM6PR14MB4155.namprd14.prod.outlook.com>
 <1c9e79d5-fd79-45a8-9c77-4228d770062d@www.fastmail.com>
 <DM6PR14MB4155E8620E5907BDA2320B7EF9279@DM6PR14MB4155.namprd14.prod.outlook.com>
Date: Wed, 02 Feb 2022 10:43:28 -0600
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain
Subject: Re: [PHP-DEV] Re: Adding `final class Deque` to PHP
From: larry@garfieldtech.com ("Larry Garfield")

On Wed, Feb 2, 2022, at 8:59 AM, tyson andre wrote:
> Hi Larry Garfield,

>> Returning $this would resolve that, I think.  (Making it return a new, immutable copy of the Deque would be even nicer, but I realize that's probably not an argument I'm going to win at this point on this RFC.)
>
> Technically, you still can have single-expression usages in 
> readable/unreadable ways
>
> - `[$deque->shift('value'), $deque][1]`, or
> - `($deque->shift('value') ?: $deque)`, or
> - `my_userland_helper_shift_and_return($deque, 'value')`

... Those are all grotesque.

> My personal preference is against making this fluent.
> I'd rather expose an efficient datastructure that's consistent with the 
> rest of PHP's functionality to the extent possible,
> which userland can use to write their own fluent/non-fluent classes.
> There's drawbacks to returning `$this`, including:
>
> 1. Inconsistency with existing APIs making remembering what does what 
> harder. Barely anything in php that I remember returns $this.
>
>    https://www.php.net/manual/en/arrayobject.append.php returns void.
>
>    https://www.php.net/manual/en/function.array-push returns an int.

So it's already inconsistent, and frankly neither of those returns is useful.  If the existing convention is "inconsistently unhelpful", then let's go ahead and make it helpful since "it's lame but at least it's consistent" is not an argument here.  (Sometimes that is a valid argument, but not in this case.)

> 2. Inconsistency with possible new datastructures/methods
>
>    If a `Map`/`Set` function were to be added, then methods for 
> add/remove would return booleans (or the old value), not $this

Removal methods need to return the value being removed, sure.  That makes sense across the board.  But I don't see why Map::add($k, $v) or Set::add($v) shouldn't also return $this.  I would make the exact same ask there, for the exact same reason: To aid functional code, which (when the syntax is conducive to it) can be more readable in many cases than procedural approaches.

> 3. Slight additional performance overhead for functionality I assume 
> will be used relatively infrequently

I would assume the opposite, at least in my own code.  I'm writing a lot of recursive or reduction-based routines these days, so if I had reason to use a queue/stack in them in the first place, they'd almost certainly get used in this fashion.

>    (php has to increment reference counts and opcache can't eliminate 
> the opcode to decrease reference counts and possibly free the return 
> value of `$deque->shift()` with the return type info being an object)

I cannot speak to this one; how much of a difference is it in practice?  

> 4.  Returning $this makes code easier to write at some cost to 
> readability - Developers new to php or using `Collections\Deque` for 
> the first time would not immediately know what the code they're reading 
> is doing.
>    (less of a problem with a good IDE, typechecker, and a typed 
> codebase, but this isn't universal)
>    Having it return void, `return $deque->push()` would be less common 
> and this would force the meaning to be clear.

I disagree that it's less readable.  Compare it with the alternatives you showed at the start of your reply.  None of those would qualify as "readable", IMO.

>    Developers might have several guesses/assumptions based on their 
> experience with other methods in php/elsewhere
>
>    - It returns the new count (JavaScript Array.push, array_push)
>    - It returns $this (Ruby)
>    - It returns a lazy copy, like you'd wanted, not modifying the 
> original
>    - It's returning void and the code in question is shorthand for 
> `return null`.
>      (Python, C++ 
> https://www.cplusplus.com/reference/vector/vector/push_back/ , 
> offsetSet and spl push()/shift() methods)

Once again, the lack of a consistent pattern means we don't need to follow the pattern.  As with the first point, "it's silly but at least it's consistent" is a valid argument in many cases, and we've changed the language accordingly before.  (Ternary associativity order comes to mind.)  That's not the case here.  There is no broad-based ingrained training that people have that would lead them to expect a given return, so we can and should go for whichever one is most powerful/flexible/conducive to good code.  IMO, that's returning $this.  (Again, since a purely functional version is apparently off the table.)

--Larry Garfield