Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119780
Message-ID: <d6ec36ca-28e0-8689-6e18-2bf5a3735f9c@gmail.com>
Date: Wed, 29 Mar 2023 19:43:19 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
To: "G. P. B." <george.banyard@gmail.com>,
 Christian Schneider <cschneid@cschneid.com>
Cc: PHP internals <internals@lists.php.net>
References: <CAFPFaMJ1opg7=SZ2ovFiAqjO_z1avpcNPFWTOoOqjhptoxSvcg@mail.gmail.com>
 <9318094F-139C-4392-8307-1C42B327CDD6@cschneid.com>
 <CAFPFaMLnmkfM7Y5t-E-Sz0y1ZLxDCeubGd79ngqa0e26JdZeAA@mail.gmail.com>
Content-Language: en-US
In-Reply-To: <CAFPFaMLnmkfM7Y5t-E-Sz0y1ZLxDCeubGd79ngqa0e26JdZeAA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [PHP-DEV] [RFC] Define proper semantics for range() function
From: dossche.niels@gmail.com (Niels Dossche)

Hi

On 28/03/2023 14:42, G. P. B. wrote:
> On Tue, 28 Mar 2023 at 08:19, Christian Schneider <cschneid@cschneid.com>
> wrote:
> 
>> Am 28.03.2023 um 00:36 schrieb G. P. B. <george.banyard@gmail.com>:
>>> I therefore propose the "Define proper semantics for range() function"
>> RFC
>>> to address the unintuitive behaviour that sees no usage and/or hide bugs:
>>> https://wiki.php.net/rfc/proper-range-semantics
>>>
>>> The change propose to throw TypeErrors and ValueErrors for case where I
>>> couldn't find occurrences in the wild and hide bugs, and emit some
>>> E_WARNINGs for cases that are hard to detect via static analysis.
>>
>> I think it makes sense to clean up the range() function, thanks!
>>
>> There are two cases I would handle differently:
>> - I'm not sure why a negative step for $start > $end is considered wrong,
>> I consider range(10, 0, -2) at least as logical/readable as using a
>> positive decrement of 2. Not requiring a sign for steps seems weirder to me
>> but that's something we cannot change. *BUT* if it is the result of a
>> calculation it seems wrong to require an abs() around it. I do see the
>> reason for a warning/error when $start < $end and $step < 0.
>>
> 
> Considering the only other programming language that I know of that has a
> range() function that accepts a step argument is Python, and its behaviour
> is IMHO worse.
> For increasing ranges it requires a positive step, and if not just
> generates an empty range.
> For decreasing ranges it requires a negative step, and if not just
> generates an empty range (this applies even if using the default step value
> of 1 which is bonkers).
> 
> Making it a requirement to pass a negative step is definitely out of the
> question.
> Making it okay to use negative steps *only* for decreasing ranges could be
> sensible, but we check for the step parameter way before we look into the
> boundary values because those are different for int, float and string
> boundaries.
> Moreover, I personally find it weirder to require a sign for negative steps
> as for me a step is something that *must* be positive, and at least Kotlin
> seems to somewhat agree with me looking around at
> https://kotlinlang.org/docs/ranges.html#progression and playing with the
> source code
> Namely:
> for (i in 4 downTo 1 step 2) print(i)
> 
>> 42
> 
> for (i in 4 downTo 1 step -2) print(i)
> 
>> Exception in thread "main" java.lang.IllegalArgumentException: Step must
> be positive, was: -2.
> 
> 
>> - Values of '' or null in integer context (e.g. range(null, 10, 2)) should
>> IMHO emit a warning first, not directly be changed to a TypeError. The
>> usual BC / migration concern :-)
>>
> 
> When null is used, no TypeError is emitted, just the "usual" null to scalar
> deprecation that happens since
> https://wiki.php.net/rfc/deprecate_null_to_scalar_internal_arg got accepted.
> But I'll add an example to the RFC and a test case in the PR.
> 
> Trying to figure out if an empty string was used with another string
> boundary is tedious, as this information needs to somehow get carried
> around.
> A previous iteration of the PR used to convert empty strings to 0 with a
> warning, but considering the analysis I decide to just make this a
> ValueError as it doesn't seem that empty strings are actually used in
> practice.
> But this is an easy revert, and I'm not really bound to this decision.
> 
> Best regards,
> 
> George P. Banyard
> 

I like the RFC in general, just this negative $step parameter warning stood out to me.

While I agree that requiring a negative $step for a decreasing range is a bit silly,
I've always found it intuitive that a negative $step should be used for a decreasing range.
I'm not saying that it should be required, I'm just concerned about the BC break of emitting E_WARNING and breaking people's intuition.

The reasoning behind this is that range($start, $end, $step) (for me at least) has always been symmetric and consistent with a for loop:
  $array = [];
  for ($i = $start; $i < $end (or >); $i += $step) { $array[] = $i; }
So by allowing a negative step the behaviour is consistent with how for loops behave, and avoids a BC break.

That Kotlin doesn't require it makes sense for me because the words "down to" combined with a negative step indeed don't make sense.
The negativity is in a sense already embedded by explicitly writing "down to". It's less explicit for PHP's range function.

I looked at the source code of some of my projects and I do see some occurrences of range($high, $low, -1 (or another negative value)).

Just wanted to chime in quickly to state my thoughts on this.
Thanks.

Kind regards
Niels