Update Psalm to 5.12.0

[morozov]

Q	A
Type	improvement

This patch should unblock the upgrade to PHPUnit 10. jetbrains/phpstorm-stubs need to be updated to a development version in order to include the changes from JetBrains/phpstorm-stubs#1531.

It appeared to be quite challenging to implement these changes on top of a 3.x branch. Whoever is interested in doing that can try backporting them.

[derrabus]

Thank you. It probably makes sense to leave DBAL 3 on Psalm 4. You might have witnessed me struggling with the upgrade in #5830.

[derrabus]

What's the reason for this change? According to the docs, oci_num_rows() returns int|false, so we should not encounter a string here. If I recall correctly, the only driver returning row counts as string under certain conditions is MySQLi.

[morozov]

I changed it consistently with the changes in the interface. But yeah, let me revert the unnecessary changes.

[morozov]

In the earlier days, when we had some debates about the return types, I made some partial research on how extensions handle the integer overflow. I guess I'll leave my notes here for visibility.

The DBAL returns an int|string value from:

1. Connection::executeStatement(), the number of rows affected by INSERT, UPDATE or DELETE statement.
2. Result::rowCount(), the number of rows returned by a SELECT statement.

MySQLi

We're interested in looking at mysqli::$affected_rows, mysqli_stmt::$affected_rows and mysqli_stmt::$num_rows.

1. link_affected_rows_read() (source):

   if (rc < ZEND_LONG_MAX) {
       ZVAL_LONG(retval, (zend_long) rc);
   } else {
       ZVAL_NEW_STR(retval, strpprintf(0, MYSQLI_LLU_SPEC, rc));
   }

2. MYSQLI_RETURN_LONG_INT (source):

   #define MYSQLI_RETURN_LONG_INT(__val) \
   { \
       if ((__val) < ZEND_LONG_MAX) {		\
           RETURN_LONG((zend_long) (__val));		\
       } else {				\
           /* always used with my_ulonglong -> %llu */ \
           RETURN_STR(strpprintf(0, MYSQLI_LLU_SPEC, (__val)));	\
       } \
   }

This behavior is consistent across all MySQLi use cases: connection/statement, affected rows / number of rows / last insert ID.

PDO MySQL

mysql_handle_doer, which is the handler for PDO::exec(), casts my_ulonglong (unsigned long long) to int:

PDO_DBG_RETURN((int)c);

Although, interestingly, pdo_mysql_last_insert_id uses zend_u64_to_str, i.e. always returns a string.

SQL Server

We're interested in looking at sqlsrv_rows_affected(). Interestingly, rowCount() doesn't seem to return the analog of mysqli_stmt::$num_rows (e.g. sqlsrv_num_rows()) for SELECT queries.

Both sqlsrv_rows_affected() and sqlsrv_num_rows() use stmt->current_results->row_count() which is populated via:

row_count++;
SQLSRV_ASSERT( row_count < INT_MAX, "Hard maximum of 2 billion rows exceeded in a buffered query" );

And returned via RETURN_LONG. So the SQL Server drivers can't return a value greater than PHP_INT_MAX.

[morozov]

You might have witnessed me struggling with the upgrade in #5830.

I only noticed it when I was submitting this PR 🙈

[greg0ire]

It probably makes sense to leave DBAL 3 on Psalm 4.

If we don't, new code we merge up might need a rework to be accepted by Psalm 5. If we do, then I suggest we resort to using the baseline if this is too much work, and then proceed to ignore changes to the baseline while merging up the first time.

[greg0ire]

This is less specific not more, but running this locally, I can see why it is needed: this has ripple effects throughout the codebase.

[morozov]

There are a lot of questionable changes in this PR when it comes to array types, and I won't be able to explain many of them. I can only tell that PHPStan (given the current settings) and Psalm handle them quite differently. For instance, for Psalm list<T> is a subtype of array<int<0,max>,T> but for PHPStan it's not. So there are a lot of compromises here to satisfy them both.

[morozov]

If we don't, new code we merge up might need a rework to be accepted by Psalm 5. If we do, then I suggest we resort to using the baseline if this is too much work, and then proceed to ignore changes to the baseline while merging up the first time.

I would postpone solving this problem until it becomes a problem.

We could refrain from introducing too much new code in 3.x, so it won't become a problem in the first place. Baselining the existing errors in 3.x will solve the problem only partially because the new code will be written with the baseline in place but then will be merged to 4.0.x without the baseline, so it will have to be fixed anyways.

Baselining will only allow us to acknowledge the newly introduced errors but it doesn't mean it will be possible to address them without baselining them as well. Otherwise, it would be possible to implement this patch in 3.x to begin with.

[greg0ire]

Ok, then let's try to keep this in mind while merging up, see if we need to fix Psalm issues at that time and if it is painful to do so.

[morozov]

Technically, the changes in all the commits prior to the Psalm upgrade are compatible with the old Psalm version, so they could be backported for the sake of reducing the difference between the DBAL versions. But I don't want to do it myself.