Declare multiple mysqli_ functions as impure#5481
Declare multiple mysqli_ functions as impure#5481VincentLanglet merged 7 commits intophpstan:2.1.xfrom
Conversation
… repeated builtin function call When a builtin function with possible side effects (like mysqli_connect()) was assigned with the `or die()` pattern, the conditional expression mechanism stored a narrowed type for the function call expression in the scope. On a subsequent assignment of the same function call, this stale type was retrieved instead of the fresh return type, causing a false positive "left side of or is always true" error. The fix skips creating conditional expressions for FuncCall expressions that refer to builtin functions with possible side effects, since those function calls can return different values each time and the narrowed type should not persist across assignments.
VincentLanglet
left a comment
There was a problem hiding this comment.
I don't think anything need to be changed in the src folder.
Instead add mysqli_connect inside bin/functionMetadata_original.php and resources/functionMetadata with 'hasSideEffects' => true
The fix for the false positive logicalOr.leftAlwaysTrue on repeated builtin function calls does not require code changes to AssignHandler. The root cause is that TypeSpecifier already returns empty SpecifiedTypes for functions with hasSideEffects()->yes() (TypeSpecifier.php line 2246), which prevents conditional expressions from being created in AssignHandler. The proper fix is to mark mysqli_connect in the function metadata, which is done in the next commit. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
mysqli_connect establishes a network connection to a database server, which is inherently a side effect. Marking it as hasSideEffects => true causes TypeSpecifier to return empty SpecifiedTypes for the call, which prevents stale conditional expressions from being created and fixes the false positive logicalOr.leftAlwaysTrue on repeated calls. Fixes phpstan/phpstan#14473 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. Here's a summary of the changes: Reverted: All code changes to Added:
Why the metadata-only fix works: |
VincentLanglet
left a comment
There was a problem hiding this comment.
Is there other mysqli_ functions with the same issue ?
In response to review feedback: many other mysqli_ functions beyond mysqli_connect have side effects (network I/O, state mutation, cursor advancement) and were defaulting to hasSideEffects=maybe. This could cause the same false positive as phpstan/phpstan#14473 for any of these functions when used with the `or die()` pattern. Added 56 additional mysqli_ functions covering connection management, query execution, transactions, result fetching, cursor positioning, prepared statements, and administrative operations. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. Here's a summary of what was done to address the review: Yes, other Added 56 additional
Pure getters (e.g.,
|
VincentLanglet
left a comment
There was a problem hiding this comment.
Please check again,
For instance you talked about pure getters but
- mysqli_affected_rows => This depends on the last query made
- mysqli_errno => This depends on the last function call
- mysqli_error => Same
All mysqli_* functions interact with database/connection state and are not pure. Functions like mysqli_affected_rows depend on the last query, mysqli_errno/mysqli_error depend on the last operation's error state, mysqli_real_escape_string depends on the connection charset, etc. This adds the 59 previously missing mysqli_ functions including: - State readers: mysqli_affected_rows, mysqli_errno, mysqli_error, mysqli_error_list, mysqli_insert_id, mysqli_info, mysqli_sqlstate, mysqli_warning_count, mysqli_num_rows, mysqli_num_fields, etc. - Aliases: mysqli_bind_param, mysqli_bind_result, mysqli_execute, mysqli_fetch, mysqli_escape_string, mysqli_client_encoding, etc. - Connection info: mysqli_get_host_info, mysqli_get_server_info, mysqli_get_server_version, mysqli_get_charset, etc. - Statement state: mysqli_stmt_affected_rows, mysqli_stmt_errno, mysqli_stmt_error, mysqli_stmt_num_rows, etc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. The commit adds all 59 remaining The reviewer was right that the previously excluded "pure getters" are not actually pure:
All 116 |
VincentLanglet
left a comment
There was a problem hiding this comment.
Please check again
Not all function which takes a mysql_result as param are impure.
For instance
mysqli_fetch_fields=> Return always the same field for the same mysql_resultmysqli_num_rows=> same
Functions like mysqli_fetch_fields, mysqli_fetch_field_direct, mysqli_num_rows, and mysqli_num_fields always return the same value for the same mysqli_result — they read fixed metadata and don't advance any cursor. These should not be marked as having side effects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. The commit changes 4
These functions always return the same value for the same |
VincentLanglet
left a comment
There was a problem hiding this comment.
Remove the mysqli function with ['hasSideEffects' => false] and returning a value cause it's unneeded since it's the default behavior
mysqli_fetch_field_direct, mysqli_fetch_fields, mysqli_num_fields, and mysqli_num_rows don't need explicit hasSideEffects=false since that is the default behavior when a function is not listed in the metadata. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done. Removed the 4 redundant |
staabm
left a comment
There was a problem hiding this comment.
it seems for now we don't have a better alternative than this.
so lets merge it and see whether more people run into similar issues.
Summary
Fixes phpstan/phpstan#14473
When using the common PHP pattern
$link = mysqli_connect(...) or die(...)more than once, PHPStan incorrectly reports "Left side of or is always true" on the second and subsequent calls.