Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RemoveExtraParametersRector] False positive on "defer" name #7671

Closed
msaladna opened this issue Dec 15, 2022 · 2 comments · Fixed by rectorphp/rector-src#3211
Closed

[RemoveExtraParametersRector] False positive on "defer" name #7671

msaladna opened this issue Dec 15, 2022 · 2 comments · Fixed by rectorphp/rector-src#3211
Labels

Comments

@msaladna
Copy link

Bug Report

Subject Details
0.15.1 Required parameters removed from function rewrite

Minimal PHP Code Causing Issue

Sample code:

<?php
	class Deferred extends SplStack
	{
	}

	function defer(?Deferred &$context, callable $callback): void
	{
		$context = $context ?? new Deferred();
		$callback();
	}

	defer($_, static function () {
		echo "baz";
	});

Refactored code:

1 file with changes
===================

1) lib/rector.php:4

    ---------- begin diff ----------
@@ @@

        function defer(?Deferred &$context, callable $callback): void
        {
-               $context = $context ?? new Deferred();
+               $context ??= new Deferred();
                $callback();
        }

-       defer($_, static function () {
-               echo "baz";
-       });
+       defer($_);
    ----------- end diff -----------

Applied rules:
 * RemoveExtraParametersRector (https://www.reddit.com/r/PHP/comments/a1ie7g/is_there_a_linter_for_argumentcounterror_for_php/)
 * NullCoalescingOperatorRector (https://wiki.php.net/rfc/null_coalesce_equal_operator)

Interestingly, renaming "defer" to "defer1" preserves the parameters as intended. Likewise placing the top-level defer() function into a namespace preserves as intended. I was able to reproduce this through the online demo as well 92c4d9d2-c0d6-4eb5-8aa8-91a2aeaccc72.

So long as the function call is "defer" it assumes the actual parameter count to be 1.

Expected Behaviour

No change in defer() function call.

@msaladna msaladna added the bug label Dec 15, 2022
@samsonasik
Copy link
Member

Just guessing, defer or Deferred name looks like a special name in internal phpstan, ref https://github.com/phpstan/phpstan-src/blob/03786827d92df439c3a31760bcd98d560035a33f/src/Process/ProcessPromise.php#L20, as I tried different naming and its working ok, I think it should can just be skipped.

@msaladna
Copy link
Author

"Deferred" has no effect; this is a reduced example from my use case. Substituting "Deferred" with a string still refactors incorrectly: 79055dae-7877-434c-8bf8-fa26337db5ca. "defer" is the issue.

It looks like a scoping conflict during reflection, which results in a spurious code change. $functionLikeReflection->getVariants() applies all extension top-level functions without testing extension presence on host. From phpstan.phar, vendor/jetbrains/phpstorm-stubs/swoole/functions.stub:

/**
 * Defers the execution of a callback function until the surrounding function of a coroutine returns.
 *
 * This function is an alias of function swoole_coroutine_defer(); it's available only when directive
 * "swoole.use_shortname" is not explicitly turned off.
 *
 * @return void
 * @see swoole_coroutine_defer()
 *
 * @example
 * <pre>
 * go(function () {      // The surrounding function of a coroutine.
 *   echo '1';
 *   defer(function () { // The callback function to be deferred.
 *     echo '3';
 *   });
 *   echo '2';
 * });
 * <pre>
 */
function defer(callable $callback) {}

I am able to reproduce this if I change the function name to "solr_get_version":

<?php
	function solr_get_version(string $foo, callable $callback): void
	{
		$callback($foo);
	}

	solr_get_version("foo", static function ($bar) {
		echo $bar;
	});

Any top-level functions in "functions.stub" would get refactored by this rule, which includes:

swoole_version
swoole_cpu_num
swoole_last_error
swoole_async_dns_lookup_coro
swoole_async_set
swoole_coroutine_create
swoole_coroutine_defer
swoole_coroutine_socketpair
swoole_test_kernel_coroutine
swoole_client_select
swoole_select
swoole_set_process_name
swoole_get_local_ip
swoole_get_local_mac
swoole_strerror
swoole_errno
swoole_clear_error
swoole_error_log
swoole_error_log_ex
swoole_ignore_error
swoole_hashcode
swoole_mime_type_add
swoole_mime_type_set
swoole_mime_type_delete
swoole_mime_type_get
swoole_get_mime_type
swoole_mime_type_exists
swoole_mime_type_list
swoole_clear_dns_cache
swoole_substr_unserialize
swoole_substr_json_decode
swoole_internal_call_user_shutdown_begin
swoole_get_objects
swoole_get_vm_status
swoole_get_object_by_handle
go
defer
swoole_event_add
swoole_event_del
swoole_event_set
swoole_event_isset
swoole_event_dispatch
swoole_event_defer
swoole_event_cycle
swoole_event_write
swoole_event_wait
swoole_event_exit
swoole_timer_set
swoole_timer_after
swoole_timer_tick
swoole_timer_exists
swoole_timer_info
swoole_timer_stats
swoole_timer_list
swoole_timer_clear
swoole_timer_clear_all
rd_kafka_get_err_descs
rd_kafka_thread_cnt
rd_kafka_err2str
rd_kafka_errno2err
rd_kafka_errno
rd_kafka_offset_tail
solr_get_version
kafka_err2name
kafka_err2str
kafka_get_err_descs
kafka_offset_tail
kafka_thread_cnt

Extensions for reference

Array
(
    [0] => Core
    [1] => date
    [2] => libxml
    [3] => openssl
    [4] => pcre
    [5] => sqlite3
    [6] => zlib
    [7] => bcmath
    [8] => bz2
    [9] => calendar
    [10] => ctype
    [11] => curl
    [12] => dba
    [13] => dom
    [14] => FFI
    [15] => fileinfo
    [16] => filter
    [17] => ftp
    [18] => hash
    [19] => iconv
    [20] => intl
    [21] => json
    [22] => mbstring
    [23] => SPL
    [24] => session
    [25] => pcntl
    [26] => standard
    [27] => mysqlnd
    [28] => PDO
    [29] => pdo_mysql
    [30] => pdo_pgsql
    [31] => pdo_sqlite
    [32] => pgsql
    [33] => Phar
    [34] => posix
    [35] => readline
    [36] => Reflection
    [37] => mysqli
    [38] => SimpleXML
    [39] => soap
    [40] => sockets
    [41] => sodium
    [42] => tokenizer
    [43] => xml
    [44] => xmlreader
    [45] => xmlwriter
    [46] => zip
    [47] => apcu
    [48] => igbinary
    [49] => redis
    [50] => event
    [51] => ionCube Loader
    [52] => Zend OPcache
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants