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

Support dynamic name expressions in rules #3886

Merged
merged 7 commits into from
Mar 19, 2025

Conversation

zonuexe
Copy link
Contributor

@zonuexe zonuexe commented Mar 18, 2025

resolve #3885 (review)

$methodNames = [$node->name->name];
} else {
$callType = $scope->getType($node->name);
$methodNames = array_map(static fn ($type): string => $type->getValue(), $callType->getConstantStrings());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This applies to all the rules. We need to pair each name with its own Scope.

Let's say we have this code:

if (rand(0, 1)) {
    $foo = 'doFoo';
    $bar = 1;
} else {
    $foo = 'doBar';
    $bar = 'a';
}

$this->$foo($bar);

If the first parameter of doFoo accepts int, and the first parameter of doBar accepts string, this code should not report any errors.

You can see the way it should be done here:

->filterByTruthyValue(new BinaryOp\Identical($node->name, new String_($constantString->getValue())))

Also please feel free to do the same thing here:

$names = array_map(static fn (ConstantStringType $type): string => $type->getValue(), $scope->getType($node->name)->getConstantStrings());
(I thought it already works but apparently not).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh wow. thats clever.

@zonuexe zonuexe force-pushed the fix/dynamic-access-expr-node-name branch from 416437c to c5e320a Compare March 18, 2025 16:04
} else {
$nameType = $scope->getType($node->name);
$methodNames = array_map(static fn (ConstantStringType $type): string => $type->getValue(), $nameType->getConstantStrings());
$methodNameScopes = array_column(array_map(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write less clever code without multiple array_map and array_column calls. A simple foreach would suffice!

@zonuexe zonuexe force-pushed the fix/dynamic-access-expr-node-name branch from a83bdc4 to a451859 Compare March 18, 2025 17:56
@zonuexe zonuexe force-pushed the fix/dynamic-access-expr-node-name branch from a451859 to fc63761 Compare March 18, 2025 18:32
} else {
$nameType = $scope->getType($node->name);
$methodNameScopes = [];
foreach ($nameType->getConstantStrings() as $constantString) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Now please do it consistently for all the touched rules. Thanks.

@zonuexe zonuexe marked this pull request as ready for review March 18, 2025 19:52
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes ondrejmirtes merged commit 75debf6 into phpstan:2.1.x Mar 19, 2025
415 of 417 checks passed
@ondrejmirtes
Copy link
Member

Perfect, thank you! Please add the new error in a single rule and once we agree how it should work, you can copy the approach to other rules.

@zonuexe zonuexe deleted the fix/dynamic-access-expr-node-name branch March 19, 2025 08:02
@VincentLanglet
Copy link
Contributor

VincentLanglet commented Mar 19, 2025

I think it solved phpstan/phpstan#2920

Should I write a non regression test or someone already on it ?

And seems like it also solved phpstan/phpstan#7990

For the issue phpstan/phpstan#4608, I'm not sure... Seems like there is a regression since
the last two lines of

$c = new class {
	public function abc(): void {}
};

$s = rand(0, 1) ? 'abc' : 'not_abc';

$c->{$s}();
call_user_func([$c, $s]);
[$c, $s]();

are not reported anymore. @ondrejmirtes @zonuexe

@ondrejmirtes
Copy link
Member

phpstan/phpstan#2920 does not need a regression test, there are many examples of tests in this PR with the same value here already. Same for phpstan/phpstan#7990.

In phpstan/phpstan#4608 the lines 10 and 11 stopped being reported in september 2022, unrelated to this PR.

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

Successfully merging this pull request may close these issues.

5 participants