Add stringable access check to ClassConstantRule #3910
Conversation
| @@ -39,6 +39,7 @@ public function __construct( | |||
| private RuleLevelHelper $ruleLevelHelper, | |||
| private ClassNameCheck $classCheck, | |||
| private PhpVersion $phpVersion, | |||
| private bool $checkNonStringableDynamicAccess = true, | |||
There was a problem hiding this comment.
I understand that we shouldn't use the default value, but I was getting the following error, probably due to a missing DI configuration, so I temporarily added = true to test.
% make phpstan
php bin/phpstan clear-result-cache -q && php -d memory_limit=448M bin/phpstan
In Resolver.php line 677:
Service 'rules.26' (type of PHPStan\Rules\Classes\ClassConstantRule): Parameter $checkNonStringableDynamicAccess in ClassConstantRule::__construct() has no class type or default value, so its value must be
specified.
There was a problem hiding this comment.
My suggestion above will help you get rid of this error.
zonuexe
force-pushed
the
feature/class-const-dynamic-access-stringable-rule
branch
from
25dfd4f to
4e57718
Compare
| @@ -60,6 +61,24 @@ public function processNode(Node $node, Scope $scope): array | |||
| $name = $constantString->getValue(); | |||
| $constantNameScopes[$name] = $scope->filterByTruthyValue(new Identical($node->name, new String_($name))); | |||
| } | |||
|
|
|||
| if ($this->checkNonStringableDynamicAccess) { | |||
| $accepts = $this->ruleLevelHelper->accepts(new StringType(), $nameType, true); | |||
There was a problem hiding this comment.
Delete this line, it's not used anywhere.
| $node->name, | ||
| '', | ||
| static fn (Type $type) => $type->toString()->isString()->yes() | ||
| ); |
There was a problem hiding this comment.
After this call there should be:
$type = $typeResult->getType();
if ($type instanceof ErrorType) {
return [];
}Some rules return "unknown class errors" but that's irrelevant here.
| @@ -19,6 +19,8 @@ class ClassConstantRuleTest extends RuleTestCase | |||
|
|
|||
| private int $phpVersion; | |||
|
|
|||
| private bool $checkNonStringableDynamicAccess; | |||
There was a problem hiding this comment.
If all the test cases use true then this property isn't needed at all. Just use true in the new expression in getRule.
| $typeResult->getType()->toString()->isNumericString()->yes() | ||
| ) { | ||
| $errors[] = RuleErrorBuilder::message(sprintf('Cannot fetch class constant with a non-stringable type %s.', $nameType->describe(VerbosityLevel::typeOnly()))) | ||
| ->identifier('classConstant.fetchInvalidExpression') |
There was a problem hiding this comment.
The error message and the identifier should reflect we're trying to access a class constant by non-stringable name. That's not obvious.
There was a problem hiding this comment.
Also it might be nice to mention the class name here.
| - | ||
| class: PHPStan\Rules\Classes\ClassConstantRule | ||
| arguments: | ||
| checkNonStringableDynamicAccess: %featureToggles.checkNonStringableDynamicAccess% |
There was a problem hiding this comment.
Why are you adding the service here in this file? Remove it please.
There was a problem hiding this comment.
Instead, config.level0.neon should be modified:
- Remove the rule from
rulessection. - Add the service same way you did here.
- But also add
tagssection with the corresponding tag.
| @@ -39,6 +39,7 @@ public function __construct( | |||
| private RuleLevelHelper $ruleLevelHelper, | |||
| private ClassNameCheck $classCheck, | |||
| private PhpVersion $phpVersion, | |||
| private bool $checkNonStringableDynamicAccess = true, | |||
There was a problem hiding this comment.
My suggestion above will help you get rid of this error.
refs #3885 (review), #3886 (comment)