Follow-up to STH user experience, this time with actual testing

Thank you so much for taking the time to do this. Your explanations of a
more "real-world" example are extremely valuable and provide a very strong
hint at the effects that these RFC implementations may have, both in the
short and long term.

Seriously, very appreciated.

On Thu, Feb 26, 2015 at 7:30 PM Matthew Weier O'Phinney matthew@zend.com
wrote:

I've taken some time the last couple days to compile both the Scalare Type
Hints
v0.5 (heretofor STHv5) and Coercive Scalar Type Hints (heretofore
STHcoerce)
patches and test some code against them.

In each case, I modified the Phly\Http\Response class from my phly/http
package to add scalar type hints, remove previous argument validation, and
then
ran the unit tests. Here's my details of the experience, and analysis.

STHv5

With STHv5, my tests failed out of the box. First, I needed to add an error
handler that would convert the E_RECOVERABLE_ERROR to an
InvalidArgumentException so that I would get useful error messages from
PHPUnit.
Next, once I had done that, I had tests that were throwing invalid input at
methods, and validating that the invalid arguments were caught. This
worked in
all but one specific case: passing a float value to an argument expecting
an
integer. In this particular case, the value was coerced to an integer,
albeit
lossily, and no error was raised.

When I changed my test case to operate under strict_types mode, the test
executed as I had originally expected, and succeeded.

Analysis

I did not expect the float value to be coerced, particularly as it had a
fractional part. Yes, I understand that this is how casting currently
works in
PHP, and that the patch uses the current casting rules. However, I'd
expect that
any float with a fractional value would not be cast to an integer; doing
so is
lossy, and can lead to unexpected results.

The strict_types mode operated how I would expect it, but meant I was
forced to
add the declaration to the top of the file. Which leads to this:

My tests operated differently in strict vs normal mode. That means my code
does,
too. Testing both modes would be difficult to do in an automated fashion;
essentially, you're left needing to choose to support one mode or the
other.
This goes back to the point I was making earlier this week: I worry that
having
two modes will lead to a schism in userland libraries: those that support
strict, and those that do not; there will be little possibility of testing
both.

STHcoerce

With STHcoerce, my tests also failed out of the box, but not for the same
reason. Instead, I had a bunch of errors reported based on code that
PHPUnit was
executing! In this case, I discovered that:

• PHPUnit passes a boolean false to debug_backtrace()... which is
  documented
  as expecting an integer! (There are actually several constant values it
  accepts, all of which are integer values.) In this case, PHPUnit is
  relying
  on the fact that the engine casts booleans to the integers 0 and 1.
  (Zeev has
  written to the list already indicating that this coercion path will be
  supported in the patch.)
• PHPUnit is passing the results of $reflector->getDocComment() blindly to
  substr() and preg_match*(). getDocComment() is documented as returning
  EITHER
  a string OR boolean false. Again, PHPUnit is relying on PHP to cast
  boolean
  false to an empty string. (Zeev has also indicated this coercion path
  may be
  re-introduced.)

I was able to fix these in a matter of minutes, and then my tests ran, and
passed without any changes.

Analysis

STHcoerce worked about how I expect STHv5 will work if every file
were
declared in strict_types mode. In other words, it uncovered errors in
calling
both userland and internal functions. Userland typehints worked as I
expected,
with floats using fractional values not casting to integers.

Wrap Up

STHcoerce was actually far more strict than I found strict_types mode to
be in
STHv5. The reason is that it's a single, non-optional mode. This poses a
potential challenge to migration, as noted in my problems using PHPUnit
(Yes, I
WILL be sending patches their way soon!): method calls and arguments that
previously worked because of how PHP juggles types often do not work,
particularly when going from boolean to other scalar values. However, the
patch
does what I'd expect in terms of preventing operations that would result in
either data loss or data additions, all of which can have unexpected side
effects if you don't understand the casting rules currently.

The flip side to this is that you do not need to make any changes to your
code
in order to locate and fix errors. What this means from a library/framework
author perspective is that, if the STHcoerce patch is merged, I can test
against
PHP 7, make fixes, and my code is not only forward-compatible, but also
backwards to the various PHP 5 versions I might already be supporting —
and that
code now benefits from being more correct.

Because STHv5 is opt-in only, the only way to see similar type errors is to
somehow automate the addition of the strict_types declaration to all files
in
your project. While it can be done with tools like sed and awk,
realistically it
means that existing libraries cannot and/or will not benefit easily from
the
changes until they support PHP 7 as a minimum version. Additionally, it
poses
potential problems in terms of different testing results based on which
mode you
run in, leading to ambiguity for users. The only way to address the latter
as a
library author is to specify which mode your code is known to run in.

Ironically, I'm seeing STHcoerce as more strict than STHv5 in terms of type
handling, due to the fact that every call, userland or internal, is
affected.
The fact that it's always on makes it simpler for me to test against it
now, and
makes it easier to make my existing code both forward-compatible and more
correct — even if I do not add type hints until I update my minimum
supported
version to PHP 7.

I am slightly worried about the amount of work needed to make code run
from v5
to v7 if STHcoerce is adopted. That said, the issues I found in PHPUnit
were
corrected in minutes (though a more thorough patch that allows early
return from
methods if doc comments are not strings would be better), and my own
library's
tests ran without problems once the PHPUnit issues were corrected (both
code
with and without typehints). Strict_types is definitely interesting, but I
found
that strict_types mode was more or less how I wanted code to operate, and
to
opt-in to that means changes to every file in my library — which is a much
larger migration problem.

In the end: kudos to everyone who is working on these patches and RFCs. I'm
excited about scalar type hints in PHP 7!

--
Matthew Weier O'Phinney
Principal Engineer
Project Lead, Zend Framework and Apigility
matthew@zend.com
http://framework.zend.com
http://apigility.org
PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc

On Thu, Feb 26, 2015 at 4:29 PM, Matthew Weier O'Phinney
matthew@zend.com wrote:

I've taken some time the last couple days to compile both the Scalare Type Hints
v0.5 (heretofor STHv5) and Coercive Scalar Type Hints (heretofore STHcoerce)
patches and test some code against them.

Thanks for this detailed report, much appreciated!

In each case, I modified the Phly\Http\Response class from my phly/http
package to add scalar type hints, remove previous argument validation, and then
ran the unit tests. Here's my details of the experience, and analysis.

STHv5

With STHv5, my tests failed out of the box. First, I needed to add an error
handler that would convert the E_RECOVERABLE_ERROR to an
InvalidArgumentException so that I would get useful error messages from PHPUnit.
Next, once I had done that, I had tests that were throwing invalid input at
methods, and validating that the invalid arguments were caught. This worked in
all but one specific case: passing a float value to an argument expecting an
integer. In this particular case, the value was coerced to an integer, albeit
lossily, and no error was raised.

When I changed my test case to operate under strict_types mode, the test
executed as I had originally expected, and succeeded.

Analysis

I did not expect the float value to be coerced, particularly as it had a
fractional part. Yes, I understand that this is how casting currently works in
PHP, and that the patch uses the current casting rules. However, I'd expect that
any float with a fractional value would not be cast to an integer; doing so is
lossy, and can lead to unexpected results.

The strict_types mode operated how I would expect it, but meant I was forced to
add the declaration to the top of the file. Which leads to this:

My tests operated differently in strict vs normal mode. That means my code does,
too. Testing both modes would be difficult to do in an automated fashion;
essentially, you're left needing to choose to support one mode or the other.
This goes back to the point I was making earlier this week: I worry that having
two modes will lead to a schism in userland libraries: those that support
strict, and those that do not; there will be little possibility of testing both.

It seems you still are still confused about that one :) This opt-in
mode is the key value of this RFC. Nothing changes if you do not care
and do not want to care.

And if you care and want to port part of your code, you can, being
fully informed about what it means for this file.

STHcoerce

With STHcoerce, my tests also failed out of the box, but not for the same
reason. Instead, I had a bunch of errors reported based on code that PHPUnit was
executing! In this case, I discovered that:

• PHPUnit passes a boolean false to debug_backtrace()... which is documented
  as expecting an integer! (There are actually several constant values it
  accepts, all of which are integer values.) In this case, PHPUnit is relying
  on the fact that the engine casts booleans to the integers 0 and 1. (Zeev has
  written to the list already indicating that this coercion path will be
  supported in the patch.)

And now let wait all other cases not covered by errors but casting to
different values, maybe only a few, maybe none, we simply do not know.

Cheers,

De : Matthew Weier O'Phinney [mailto:matthew@zend.com]

• PHPUnit passes a boolean false to debug_backtrace()... which is
  documented
  as expecting an integer! (There are actually several constant values it
  accepts, all of which are integer values.) In this case, PHPUnit is relying
  on the fact that the engine casts booleans to the integers 0 and 1. (Zeev has
  written to the list already indicating that this coercion path will be
  supported in the patch.)

AFAIK, we won't support boolean to integer. IMO, considering Boolean as integer is a bug and must not be supported.

• PHPUnit is passing the results of $reflector->getDocComment() blindly to
  substr() and preg_match*(). getDocComment() is documented as returning
  EITHER
  a string OR boolean false. Again, PHPUnit is relying on PHP to cast boolean
  false to an empty string. (Zeev has also indicated this coercion path may be
  re-introduced.)

The same as above for bool -> string.

I hope you're wrong because I wouldn't like supporting boolean to scalar back again.

Your test demonstrates this because you found undetected bugs. I am more and more sure that, what I first said as a joke, will prove true : during the next years, STH will be used mostly as a debugging tool, proving opposite arguments were FUD or, at least, phantasm.

Regards

François

Matthew,

On Fri, Feb 27, 2015 at 1:29 AM, Matthew Weier O'Phinney matthew@zend.com
wrote:

I've taken some time the last couple days to compile both the Scalare Type
Hints
v0.5 (heretofor STHv5) and Coercive Scalar Type Hints (heretofore
STHcoerce)
patches and test some code against them.

In each case, I modified the Phly\Http\Response class from my phly/http
package to add scalar type hints, remove previous argument validation, and
then
ran the unit tests. Here's my details of the experience, and analysis.

STHv5

With STHv5, my tests failed out of the box. First, I needed to add an error
handler that would convert the E_RECOVERABLE_ERROR to an
InvalidArgumentException so that I would get useful error messages from
PHPUnit.
Next, once I had done that, I had tests that were throwing invalid input at
methods, and validating that the invalid arguments were caught. This
worked in
all but one specific case: passing a float value to an argument expecting
an
integer. In this particular case, the value was coerced to an integer,
albeit
lossily, and no error was raised.

When I changed my test case to operate under strict_types mode, the test
executed as I had originally expected, and succeeded.

Analysis

I did not expect the float value to be coerced, particularly as it had a
fractional part. Yes, I understand that this is how casting currently
works in
PHP, and that the patch uses the current casting rules. However, I'd
expect that
any float with a fractional value would not be cast to an integer; doing
so is
lossy, and can lead to unexpected results.

The strict_types mode operated how I would expect it, but meant I was
forced to
add the declaration to the top of the file. Which leads to this:

My tests operated differently in strict vs normal mode. That means my code
does,
too. Testing both modes would be difficult to do in an automated fashion;
essentially, you're left needing to choose to support one mode or the
other.
This goes back to the point I was making earlier this week: I worry that
having
two modes will lead to a schism in userland libraries: those that support
strict, and those that do not; there will be little possibility of testing
both.

STHcoerce

With STHcoerce, my tests also failed out of the box, but not for the same
reason. Instead, I had a bunch of errors reported based on code that
PHPUnit was
executing! In this case, I discovered that:

• PHPUnit passes a boolean false to debug_backtrace()... which is
  documented
  as expecting an integer! (There are actually several constant values it
  accepts, all of which are integer values.) In this case, PHPUnit is
  relying
  on the fact that the engine casts booleans to the integers 0 and 1.
  (Zeev has
  written to the list already indicating that this coercion path will be
  supported in the patch.)
• PHPUnit is passing the results of $reflector->getDocComment() blindly to
  substr() and preg_match*(). getDocComment() is documented as returning
  EITHER
  a string OR boolean false. Again, PHPUnit is relying on PHP to cast
  boolean
  false to an empty string. (Zeev has also indicated this coercion path
  may be
  re-introduced.)

I was able to fix these in a matter of minutes, and then my tests ran, and
passed without any changes.

Analysis

STHcoerce worked about how I expect STHv5 will work if every file
were
declared in strict_types mode. In other words, it uncovered errors in
calling
both userland and internal functions. Userland typehints worked as I
expected,
with floats using fractional values not casting to integers.

Wrap Up

STHcoerce was actually far more strict than I found strict_types mode to
be in
STHv5. The reason is that it's a single, non-optional mode. This poses a
potential challenge to migration, as noted in my problems using PHPUnit
(Yes, I
WILL be sending patches their way soon!): method calls and arguments that
previously worked because of how PHP juggles types often do not work,
particularly when going from boolean to other scalar values. However, the
patch
does what I'd expect in terms of preventing operations that would result in
either data loss or data additions, all of which can have unexpected side
effects if you don't understand the casting rules currently.

The flip side to this is that you do not need to make any changes to your
code
in order to locate and fix errors. What this means from a library/framework
author perspective is that, if the STHcoerce patch is merged, I can test
against
PHP 7, make fixes, and my code is not only forward-compatible, but also
backwards to the various PHP 5 versions I might already be supporting —
and that
code now benefits from being more correct.

Because STHv5 is opt-in only, the only way to see similar type errors is to
somehow automate the addition of the strict_types declaration to all files
in
your project. While it can be done with tools like sed and awk,
realistically it
means that existing libraries cannot and/or will not benefit easily from
the
changes until they support PHP 7 as a minimum version. Additionally, it
poses
potential problems in terms of different testing results based on which
mode you
run in, leading to ambiguity for users. The only way to address the latter
as a
library author is to specify which mode your code is known to run in.

Ironically, I'm seeing STHcoerce as more strict than STHv5 in terms of type
handling, due to the fact that every call, userland or internal, is
affected.
The fact that it's always on makes it simpler for me to test against it
now, and
makes it easier to make my existing code both forward-compatible and more
correct — even if I do not add type hints until I update my minimum
supported
version to PHP 7.

I am slightly worried about the amount of work needed to make code run
from v5
to v7 if STHcoerce is adopted. That said, the issues I found in PHPUnit
were
corrected in minutes (though a more thorough patch that allows early
return from
methods if doc comments are not strings would be better), and my own
library's
tests ran without problems once the PHPUnit issues were corrected (both
code
with and without typehints). Strict_types is definitely interesting, but I
found
that strict_types mode was more or less how I wanted code to operate, and
to
opt-in to that means changes to every file in my library — which is a much
larger migration problem.

In the end: kudos to everyone who is working on these patches and RFCs. I'm
excited about scalar type hints in PHP 7!

Can you gist the PHPUnit changes and point us to the coercive patch you
compiled?

Your results seem to be:

1. Weak mode in Dual Mode patch is too weak.
2. Coercive patch gives good feedback without strict.
3. Strict is too strict to apply to all your code retroactively that
   hasn't been designed for this from the beginning.

Ironically if both RFCs are accepted you would get the best of your world,
because Coercive STH is only a tightening of the rules for Weak mode of
STHDual.

--
Matthew Weier O'Phinney
Principal Engineer
Project Lead, Zend Framework and Apigility
matthew@zend.com
http://framework.zend.com
http://apigility.org
PGP key: http://framework.zend.com/zf-matthew-pgp-key.asc

• PHPUnit passes a boolean false to debug_backtrace()... which is
  documented
  as expecting an integer! (There are actually several constant values it
  accepts, all of which are integer values.) In this case, PHPUnit is
  relying
  on the fact that the engine casts booleans to the integers 0 and 1.
  (Zeev has
  written to the list already indicating that this coercion path will be
  supported in the patch.)

Matthew,

I just wanted to point out that actually, supporting boolean->int coercion
is not planned. Converting boolean to integer is actually a very likely
scenario where you'd have an outcome that you didn't expect. It's debatable
whether sending false to a bitmask is OK or not, but given it's a bitmask,
an explicit 0 is a lot more correct.

• PHPUnit is passing the results of $reflector->getDocComment() blindly to
  substr() and preg_match*(). getDocComment() is documented as returning
  EITHER
  a string OR boolean false. Again, PHPUnit is relying on PHP to cast
  boolean
  false to an empty string. (Zeev has also indicated this coercion path
  may be
  re-introduced.)

We're looking to re-introduce string->boolean (which will likely be common
in database scenarios), but not the other way around - where it's very
likely hiding a bug. Virtually all of the boolean->string coercions that
were flagged as deprecated by the patch were at the very least suspects of
being real world bugs.

Thanks!

Zeev

Matthew,

Thanks a bunch for going through this and writing such a detailed report.

STHv5

[snip]

Analysis

I did not expect the float value to be coerced, particularly as it had a
fractional part. Yes, I understand that this is how casting currently works in
PHP, and that the patch uses the current casting rules. However, I'd expect that
any float with a fractional value would not be cast to an integer; doing so is
lossy, and can lead to unexpected results.

The strict_types mode operated how I would expect it, but meant I was forced to
add the declaration to the top of the file. Which leads to this:

My tests operated differently in strict vs normal mode. That means my code does,
too. Testing both modes would be difficult to do in an automated fashion;
essentially, you're left needing to choose to support one mode or the other.
This goes back to the point I was making earlier this week: I worry that having
two modes will lead to a schism in userland libraries: those that support
strict, and those that do not; there will be little possibility of testing both.

I think this last bit is misguided. Your tests are running differently
depending if they are in strict and weak mode, that is correct, but the
code's behavior itself is not changing based on the test suite's strictness.

That does not mean your library is tested to run in mode X or Y, it just
means your test suite's correctness relies on testing the PHP type hints
in this case, and it breaks when you change the way the hints behave.

For users the code will work the same no matter if they are in strict or
weak mode, but if they are in weak mode they should indeed watch out if
they send floats they will be silently truncated as those are the rules
of weak mode.

In the end: kudos to everyone who is working on these patches and RFCs. I'm
excited about scalar type hints in PHP 7!

I agree with Benjamin's conclusion that having both RFCs combined might
be for the best. At the very least lossy float->int should be disabled
in the STHv5's weak mode because that's dangerous and unlikely to occur IMO.

The other casting changes from STHcoerce I am not sure if they're for
the best or not. I am a bit worried we change things arbitrarily based
on who has a good argument not to change them, and we might end up with
a completely inconsistent set of casting rules.

Cheers

Am 27.02.2015 um 01:29 schrieb Matthew Weier O'Phinney:

• PHPUnit passes a boolean false to debug_backtrace()... which is documented
  as expecting an integer! (There are actually several constant values it
  accepts, all of which are integer values.) In this case, PHPUnit is relying
  on the fact that the engine casts booleans to the integers 0 and 1. (Zeev has
  written to the list already indicating that this coercion path will be
  supported in the patch.)
• PHPUnit is passing the results of $reflector->getDocComment() blindly to
  substr() and preg_match*(). getDocComment() is documented as returning EITHER
  a string OR boolean false. Again, PHPUnit is relying on PHP to cast boolean
  false to an empty string. (Zeev has also indicated this coercion path may be
  re-introduced.)

Pull requests for PHPUnit would be appreciated ;-)