Report non-stringable names in variable variables, dynamic method calls and dynamic static property/method access

[phpstan-bot]

Summary

$$this (and other dynamic accesses whose name expression cannot become a string) was not reported by PHPStan, even though it fatals at runtime (Object of class X could not be converted to string). PHPStan already detected this for dynamic property access ($obj->{$expr}) and class constant access (Foo::{$expr}) behind the checkNonStringableDynamicAccess feature toggle, but the same family of constructs was unchecked for variable variables, dynamic method calls, dynamic static method calls and dynamic static property access.

This PR extends the existing check to the rest of that family.

Changes

• Added src/Rules/NonStringableDynamicAccessCheck.php — a shared service (gated on the checkNonStringableDynamicAccess feature toggle) with:
  • checkStringCastableName() for names PHP casts to string at runtime (variable variables, property and static-property names) — objects implementing __toString are accepted.
  • checkStringName() for names that must be real strings (method, static-method and class-constant names) — Stringable is not accepted, matching PHP's "Method name must be a string".
• src/Rules/Variables/DefinedVariableRule.php — report $$expr with a non-stringable name (variable.nameNotString). This is the case from the issue.
• src/Rules/Methods/CallMethodsRule.php — report $obj->{$expr}() with a non-string name (method.nameNotString).
• src/Rules/Methods/CallStaticMethodsRule.php — report Foo::{$expr}() with a non-string name (staticMethod.nameNotString).
• src/Rules/Properties/AccessStaticPropertiesCheck.php — report Foo::${$expr} with a non-stringable name (staticProperty.nameNotString), gated on read access so assignments are not reported twice.
• src/Rules/Properties/AccessPropertiesCheck.php and src/Rules/Classes/ClassConstantRule.php — refactored to delegate to the new shared service; messages and behavior unchanged.
• Updated all manual constructions of these checks/rules in the test suite to pass the new dependency.

Root cause

These constructs all share one conceptual axis: a name expression that PHP must turn into a string before it can resolve a variable/member. The runtime-string semantics differ slightly per construct:

• variable variables, property and static-property names accept Stringable (PHP casts via __toString);
• method, static-method and class-constant names require an actual string (Stringable fatals).

PHPStan only implemented the check for two members of this family (instance properties and class constants). The fix centralizes the logic and applies it to every member, with the correct stringable-vs-string semantics for each (verified against real PHP behavior).

Test

• tests/PHPStan/Rules/Variables/data/variable-variable-name.php + DefinedVariableRuleTest::testVariableVariableName — covers the reported $$this, plus $$array/$$object (errors) and $$name/$$stringable/$$int (valid).
• tests/PHPStan/Rules/Methods/data/dynamic-method-name.php + CallMethodsRuleTest::testDynamicMethodName — covers $this, object, Stringable (rejected for methods) and int.
• tests/PHPStan/Rules/Methods/data/dynamic-static-method-name.php + CallStaticMethodsRuleTest::testDynamicStaticMethodName — same, for static calls.
• tests/PHPStan/Rules/Properties/data/dynamic-static-property-name.php + AccessStaticPropertiesRuleTest::testDynamicStaticPropertyName — covers object/array (errors), Stringable/int/string (valid), and an assignment to confirm it is reported exactly once.
• Existing data/dynamic-call.php expectations updated for the new method/static-method name errors it now surfaces.

Fixes phpstan/phpstan#4710

[VincentLanglet]

Try to do

$errors = array_merge($errors, $this->nonStringableDynamicAccessCheck->checkStringName(...));

instead by passing as args

'Method name for %s must be a string, but %s was given.'

and other needed param.

[phpstan-bot]

I've addressed VincentLanglet's review. Here's a summary of what changed:

What the reviewer asked for

Instead of each rule receiving the offending name Type back and building the RuleErrorBuilder error itself, the reviewer wanted the call sites to be a one-liner:

$errors = array_merge($errors, $this->nonStringableDynamicAccessCheck->checkStringName(...));

with the message format and other needed parameters passed as args.

Changes made

NonStringableDynamicAccessCheck — both checkStringCastableName() and checkStringName() now:

• return list<IdentifierRuleError> (empty array when usable) instead of ?Type
• accept a string $messageFormat, a list<string> $messageArgs (the sprintf args that precede the offending type), a string $identifier, and an optional ?int $line
• delegate error construction to a shared private buildError() helper that appends the offending type's describe(precise) to $messageArgs, sets the identifier, and conditionally sets the line

All 6 call sites (DefinedVariableRule, CallMethodsRule, CallStaticMethodsRule, AccessStaticPropertiesCheck, AccessPropertiesCheck, ClassConstantRule) now just array_merge the result of the check call, passing the message format + args. Each shed its local RuleErrorBuilder::message(...) block, and I cleaned up imports that became unused (RuleErrorBuilder/sprintf/VerbosityLevel where no longer referenced).

The error messages, identifiers, and lines are all unchanged — only where they're built moved.

Verification

• All affected rule tests pass (DefinedVariableRuleTest, CallMethodsRuleTest, CallStaticMethodsRuleTest, AccessStaticPropertiesRuleTest, ClassConstantRuleTest, AccessPropertiesRuleTest, the two assign-rule tests, the renamed-arg test, and Bug9307CallMethodsRuleTest).
• make phpstan reports no errors.

Committed as 922241ca6 and pushed to the PR branch.

[VincentLanglet]

1. Do we have to set manually the line (isn't it automatically ?)

2. If we have to, can't we always set it to $node->name->getStartLine() to avoid passing it to the method ?

3. The param for the sprintf are always an empty array or an array with a single element. We could have a nullable param with something like

$placeholder = $foo instanceof Name
					? $scope->resolveName($foo)
					: $scope->getType($foo)->describe(VerbosityLevel::typeOnly());

[phpstan-bot]

I've addressed all three points in VincentLanglet's review.

Summary

1) "Do we have to set manually the line (isn't it automatically)?"

I confirmed the line is set automatically — RuleErrorTransformer defaults each error's line to the processed node's start line ($node->getStartLine()). But for these checks the processed node is the whole MethodCall/StaticCall/PropertyFetch, whose start line can differ from the name expression's start line on multi-line calls. So an explicit line was needed to point at the name.

2) "Can't we always set it to $node->name->getStartLine()?"

Yes. Both checkStringName() and checkStringCastableName() already receive the name Expr, so they now set ->line($name->getStartLine()) internally in the shared buildError() helper. The ?int $line parameter is gone from both methods and dropped from all six call sites.

3) "The sprintf params are always empty or a single element — use a nullable param."

Replaced the list<string> $messageArgs parameter with a single nullable class node (Name|Expr|null). The check now computes the placeholder itself:

$messageArgs[] = $classNode instanceof Name
    ? $scope->resolveName($classNode)
    : $scope->getType($classNode)->describe(VerbosityLevel::typeOnly());

Callers just pass the relevant node — $node->var (property/method), $node->class (static/const), or null (variable variables). This removed the duplicated $className = ... instanceof Name ? ... block from CallStaticMethodsRule, ClassConstantRule, and AccessStaticPropertiesCheck, and let me drop now-unused Name/VerbosityLevel imports from three files.

Verification

• All affected rule tests pass (DefinedVariableRuleTest, CallMethodsRuleTest, CallStaticMethodsRuleTest, AccessStaticPropertiesRuleTest, ClassConstantRuleTest, AccessPropertiesRuleTest, both assign-rule tests, the renamed-arg test, Bug9307CallMethodsRuleTest).
• make phpstan reports no errors.
• Messages, identifiers, and lines are unchanged.

Committed as 4301a5585 and pushed to the PR branch.

[VincentLanglet]

The baseline is not generated in the same order, cf diff

232,233c232,233
< 			rawMessage: 'Method name for Nette\Schema\Elements\AnyOf|Nette\Schema\Elements\Structure|Nette\Schema\Elements\Type must be a string, but array|Nette\DI\Definitions\Definition|Nette\DI\Definitions\Reference|string|null was given.'
< 			identifier: method.nameNotString
---
> 			rawMessage: Variable method call on Nette\Schema\Elements\AnyOf|Nette\Schema\Elements\Structure|Nette\Schema\Elements\Type.
> 			identifier: method.dynamicName
238,239c238,239
< 			rawMessage: 'Method name for Nette\Schema\Expect must be a string, but array|Nette\DI\Definitions\Definition|Nette\DI\Definitions\Reference|string|null was given.'
< 			identifier: staticMethod.nameNotString
---
> 			rawMessage: 'Method name for Nette\Schema\Elements\AnyOf|Nette\Schema\Elements\Structure|Nette\Schema\Elements\Type must be a string, but array|Nette\DI\Definitions\Definition|Nette\DI\Definitions\Reference|string|null was given.'
> 			identifier: method.nameNotString
244,245c244,245
< 			rawMessage: Variable method call on Nette\Schema\Elements\AnyOf|Nette\Schema\Elements\Structure|Nette\Schema\Elements\Type.
< 			identifier: method.dynamicName
---
> 			rawMessage: Variable static method call on Nette\Schema\Expect.
> 			identifier: staticMethod.dynamicName
250,251c250,251
< 			rawMessage: Variable static method call on Nette\Schema\Expect.
< 			identifier: staticMethod.dynamicName
---
> 			rawMessage: 'Method name for Nette\Schema\Expect must be a string, but array|Nette\DI\Definitions\Definition|Nette\DI\Definitions\Reference|string|null was given.'
> 			identifier: staticMethod.nameNotString

[phpstan-bot]

I processed this review but have nothing to report.