False Positives on disallowedFunctionCalls Rule in Version 4.1.1

[yoannblot]

Description

Summary:

After upgrading from version 4.0.1 to 4.1.1, the disallowedFunctionCalls rule produces false positives when certain strings are present in method names, incorrectly identifying them as forbidden function calls.

Steps to Reproduce:

Configure disallowedFunctionCalls as follows:

parameters:
  disallowedFunctionCalls:
    - function:
        - 'data_get()'
        - 'data_set()'
        - 'optional()'
      message: 'This function is not allowed'

Define a method or function with a name that includes the substring of a forbidden function, such as testItHandlesSomeOptional().
Run PHPStan analysis.

Observed Behavior:

PHPStan flags the method as violating the disallowedFunctionCalls rule:

message: '#^Calling optional\(\) is forbidden, This function is not allowed\.$#'

Expected Behavior:

PHPStan should not flag method or function names that merely include substrings matching forbidden function names. Only actual calls to the forbidden functions should trigger a violation.

Environment:

PHPStan version: 2.1.1
spaze/phpstan-disallowed-calls: 4.1.1
Previous working version: 4.0.1

Additional Notes:

This appears to be a regression introduced in version 4.1.1. The pattern matching for the disallowedFunctionCalls rule seems to be overly aggressive, treating method or function names as if they were calls to the forbidden functions.

Let me know if more details are needed!

[spaze]

Hi, this case is actually tested for quite some time already:

printf is disallowed:

phpstan-disallowed-calls/tests/Calls/FunctionCallsTest.php

Lines 49 to 53 in c921da2

	'function' => 'printf',
	'allowIn' => [
	__DIR__ . '/../src/disallowed-allow/*.php',
	__DIR__ . '/../src/*-allow/*.*',
	],

Then in the tested file there's also printfunk on line 17:

phpstan-disallowed-calls/tests/src/disallowed/functionCalls.php

Line 17 in c921da2

printfunk();

But there's no error reported on line 17:

phpstan-disallowed-calls/tests/Calls/FunctionCallsTest.php

Lines 240 to 247 in c921da2

	[
	'Calling exec() is forbidden. [exec() matches exe*()]',
	13,
	],
	[
	'Calling print_r() is forbidden, nope.',
	25,
	],

4.1.0 (and 4.1.1) has started detecting disallowed calls also in callables (e.g. array_map('disallowedFunction', [])) and in some other places, please check the release notes. Can you please check whether that's not a forbidden function being called that way? Or can you provide a working testfile I can run? You can start with modifying the files above and then run the test with

vendor/bin/phpunit tests/Calls/FunctionCallsTest.php

[yoannblot]

Thanks for your answer 🙏

I've made a minimal failing test, and for an unknown reason, it only fails with a disallowed function called optional 🤔

Failing test

Configuration

includes:
  - ./quality/phpstan/vendor/spaze/phpstan-disallowed-calls/extension.neon

parameters:
  level: 5

  disallowedFunctionCalls:
    - function:
        - 'optional()'

Code to analyze

use PHPUnit\Framework\TestCase;

final class NothingTest extends TestCase
{
    public function testItDoesNothing(): void
    {
        $this->assertSame('optional', $this->do());
    }

    private function do(): string
    {
        return 'optional';
    }
}

Result

Success test

Code

I success my analyzing by switching the assertSame() with an assertEquals() :

$this->assertEquals('optional', $this->do());

Result

That's a bit weird, and seems an edge case, anyway it could hide a more specific bug 🐛 , wdyt?

[spaze]

PHPStan 2.x says that the "expected" param of the assertSame method is callable, see the test here:

string(15) "param->getName:"
string(8) "expected"
string(18) "param->isCallable:"
string(3) "Yes"

https://github.com/spaze/phpstan-disallowed-calls/actions/runs/12911818495/job/36005135640#step:6:19
(It's the https://github.com/spaze/phpstan-disallowed-calls/tree/spaze/callable-param branch)

And because optional() is most probably an existing function in your code, why would you like to disallow it if it wasn't 😅, then the string optional is treated as a callable reference to that function. If the function wouldn't exist, then this condition here would be false, and that's probably why only optional triggers this behavior in your case (my test uses foobar):

phpstan-disallowed-calls/src/RuleErrors/DisallowedCallableParameterRuleErrors.php

Lines 90 to 92 in c921da2

	if (!$this->reflectionProvider->hasFunction($name, $scope)) {
	continue;
	}

And optional is reported because your configuration disallows it.

If the param wouldn't be reported as callable, then this condition here would be false and no error would be reported:

phpstan-disallowed-calls/src/RuleErrors/DisallowedCallableParameterRuleErrors.php

Lines 178 to 180 in c921da2

	if (!TypeCombinator::removeNull($parameter->getType())->isCallable()->yes() || !isset($reorderedArgs[$key])) {
	continue;
	}

PHPStan 1.12 doesn't report the parameter to be callable and the test passes (marked as risky though because of the vardumps):

string(15) "param->getName:"
string(8) "expected"
string(18) "param->isCallable:"
string(5) "Maybe"

https://github.com/spaze/phpstan-disallowed-calls/actions/runs/12911818495/job/36005136087#step:6:19

Honestly, I don't know what needs to be changed here, if anything in this extension.

[yoannblot]

Ok, I see, so there is something weird between PHPStan 2.x & PHPUnit...
Thank you so much @spaze for your investigation 🙏

FYI, I am using Laravel framework, which contains too many ways to write ugly code, that's why I want to disallow existing methods like optional() (link).

[yoannblot]

Not a bug here 👍

[spaze]

Maybe @ondrejmirtes could shed some light on why PHPStan 2.x shows PHPUnit\Framework\TestCase::assertSame()'s param expected as callable = yes, while PHPStan 1.x shows it as callable = maybe. It's still possible that I'm doing something funky somewhere 😁

This is the test subject (spaze/callable-param branch):

https://github.com/spaze/phpstan-disallowed-calls/blob/941f6ee029ce2b932bdb6bae798cbc08376e6ba6/tests/src/disallowed/testCase.php

and the testcase here:

https://github.com/spaze/phpstan-disallowed-calls/blob/941f6ee029ce2b932bdb6bae798cbc08376e6ba6/tests/Calls/MethodCallsTest.php

Running vendor/bin/phpunit tests/Calls/MethodCallsTest.php after composer update shows failure (and some intentional vardumps), while after composer update --prefer-lowest phpstan/phpstan phpstan/phpstan-deprecation-rules --with-all-dependencies shows Ok (but risky because of vardumps).

[ondrejmirtes]

I know! You're trying to get the signatures of PHPUnit's assertSame parameters, right?

This is how the signature looks like:

    /**
     * @template ExpectedType
     * @param ExpectedType $expected
     * @throws ExpectationFailedException
     * @phpstan-assert =ExpectedType $actual
     */
    final public static function assertSame(mixed $expected, mixed $actual, string $message = ''): void

You're doing:

$parametersAcceptor = ParametersAcceptorSelector::selectFromArgs($scope, $node->getArgs(), $reflection->getVariants());

Which triggers the whole generics type inference. What you get back for this call $this->assertSame('foobar', 'whatever') is:

    /**
     * @param 'foobar' $expected
     * @throws ExpectationFailedException
     * @phpstan-assert ='foobar' $actual
     */
    final public static function assertSame(mixed $expected, mixed $actual, string $message = ''): void

So when you ask for the type of $expected, you get 'foobar' which is callable-yes because ConstantStringType knows there's a function of the same name!

In PHPStan 1.x (without bleeding edge) you'd just get string instead of 'foobar' which is just maybe-callable.

As a possible fix you should just get $reflection->getVariants() and skip if count !== 1. The get the only one with [0]. You can still call ArgumentsNormalizer with that.

[spaze]

Wow, excellent! Thanks @ondrejmirtes for the analysis and the proposed fix, it works and doesn't report errors in the assertSame call. Just one thing to note, I was using bleeding edge with PHPStan 1.x as well, the configuration was the same for both 1.x and 2.x.

[spaze]

I've released the fix in 4.2.1 now, @yoannblot please let me know if it works as expected now.

[yoannblot]

I've released the fix in 4.2.1 now, @yoannblot please let me know if it works as expected now.

It works like a charm! Thank you so much @spaze & @ondrejmirtes ❤ !