Add stringable access check to ClassConstantRule #3910

zonuexe · 2025-03-29T21:03:09Z

refs #3885 (review), #3886 (comment), phpstan/phpstan#13238

In the context of class constant fetching, string keys are not implicitly cast.

https://3v4l.org/WZTCC

zonuexe · 2025-03-29T21:06:00Z

src/Rules/Classes/ClassConstantRule.php

 private RuleLevelHelper $ruleLevelHelper,
 private ClassNameCheck $classCheck,
 private PhpVersion $phpVersion,
+private bool $checkNonStringableDynamicAccess = true,


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.

My suggestion above will help you get rid of this error.

Do not make this optional.

Put #[AutowiredParameter(ref: '%featureToggles.checkNonStringableDynamicAccess%')] above the parameter.

After you composer dump it should work.

ondrejmirtes · 2025-03-30T11:31:35Z

src/Rules/Classes/ClassConstantRule.php

 }
+
+if ($this->checkNonStringableDynamicAccess) {
+$accepts = $this->ruleLevelHelper->accepts(new StringType(), $nameType, true);


Delete this line, it's not used anywhere.

ondrejmirtes · 2025-03-30T11:33:11Z

src/Rules/Classes/ClassConstantRule.php

+$node->name,
+'',
+static fn (Type $type) => $type->toString()->isString()->yes()
+);


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.

Dynamic fetching of constants doesn't do implicit casting, so it seems appropriate to simply check isString()->yes() without toString().

I was wrong in the early stages of this PR.

I'd rename the variable to $nameTypeResult.

ondrejmirtes · 2025-03-30T11:34:22Z

tests/PHPStan/Rules/Classes/ClassConstantRuleTest.php


 private int $phpVersion;

+private bool $checkNonStringableDynamicAccess;


If all the test cases use true then this property isn't needed at all. Just use true in the new expression in getRule.

ondrejmirtes · 2025-03-30T11:34:58Z

src/Rules/Classes/ClassConstantRule.php

+$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')


The error message and the identifier should reflect we're trying to access a class constant by non-stringable name. That's not obvious.

Also it might be nice to mention the class name here.

The error message is wrong regarding the isString/toString point you made above. This is no longer about "stringables", but about "strings". Also the identifier should reflect it's about "name".

ondrejmirtes · 2025-03-30T11:36:46Z

conf/config.neon

+-
+class: PHPStan\Rules\Classes\ClassConstantRule
+arguments:
+checkNonStringableDynamicAccess: %featureToggles.checkNonStringableDynamicAccess%


Why are you adding the service here in this file? Remove it please.

Instead, config.level0.neon should be modified:

Remove the rule from rules section.

Add the service same way you did here.

But also add tags section with the corresponding tag.

ondrejmirtes · 2025-03-30T11:37:34Z

src/Rules/Classes/ClassConstantRule.php

 private RuleLevelHelper $ruleLevelHelper,
 private ClassNameCheck $classCheck,
 private PhpVersion $phpVersion,
+private bool $checkNonStringableDynamicAccess = true,


My suggestion above will help you get rid of this error.

ondrejmirtes · 2025-04-24T10:56:06Z

I'd love if you could finish this (and the other rules) 😊

phpstan-bot · 2025-07-11T20:31:32Z

This pull request has been marked as ready for review.

ondrejmirtes · 2025-07-11T21:04:07Z

conf/config.level0.neon


 services:
+-
+class: PHPStan\Rules\Classes\ClassConstantRule


Why is this added here? It's not needed at all, the rule has #[RegisteredRule(level: 0)] attribute.

ondrejmirtes · 2025-07-11T21:05:04Z

src/Rules/Classes/ClassConstantRule.php

 private RuleLevelHelper $ruleLevelHelper,
 private ClassNameCheck $classCheck,
 private PhpVersion $phpVersion,
+private bool $checkNonStringableDynamicAccess = true,


Do not make this optional.

Put #[AutowiredParameter(ref: '%featureToggles.checkNonStringableDynamicAccess%')] above the parameter.

ondrejmirtes · 2025-07-11T21:05:16Z

src/Rules/Classes/ClassConstantRule.php

 private RuleLevelHelper $ruleLevelHelper,
 private ClassNameCheck $classCheck,
 private PhpVersion $phpVersion,
+private bool $checkNonStringableDynamicAccess = true,


After you composer dump it should work.

ondrejmirtes · 2025-07-11T21:07:36Z

src/Rules/Classes/ClassConstantRule.php

+$node->name,
+'',
+static fn (Type $type) => $type->toString()->isString()->yes()
+);


I'd rename the variable to $nameTypeResult.

ondrejmirtes · 2025-07-11T21:08:49Z

src/Rules/Classes/ClassConstantRule.php

+$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')


The error message is wrong regarding the isString/toString point you made above. This is no longer about "stringables", but about "strings". Also the identifier should reflect it's about "name".

phpstan-bot · 2025-07-12T22:24:36Z

This pull request has been marked as ready for review.

ondrejmirtes

After this I'll merge this and you can make the same changes to other rules 👍

ondrejmirtes · 2025-07-13T06:48:07Z

src/Rules/Classes/ClassConstantRule.php

+static fn (Type $type) => $type->isString()->yes(),
+);
+
+$type = $nameTypeResult->getType();


$type -> $nameType

ondrejmirtes · 2025-07-13T06:48:45Z

src/Rules/Classes/ClassConstantRule.php

+
+$type = $nameTypeResult->getType();
+
+if (!$type->isString()->yes()) {


Suggested change

if (!$type->isString()->yes()) {

if (!$type instanceof ErrorType && !$type->isString()->yes()) {

All calls to findTypeToCheck follow this pattern. ErrorType is returned if unknown classes were encountered there.

zonuexe · 2025-07-13T07:20:52Z

@ondrejmirtes Fixed it. I will prepare a PR for the rules for each node.

ondrejmirtes · 2025-07-13T11:41:49Z

Thank you!

zonuexe commented Mar 29, 2025

View reviewed changes

zonuexe force-pushed the feature/class-const-dynamic-access-stringable-rule branch from 25dfd4f to 4e57718 Compare March 29, 2025 21:13

ondrejmirtes requested changes Mar 30, 2025

View reviewed changes

zonuexe force-pushed the feature/class-const-dynamic-access-stringable-rule branch 4 times, most recently from 9667b8e to 9e7e1a8 Compare July 11, 2025 13:43

Add stringable access check to ClassConstantRule

8e8bcd5

zonuexe force-pushed the feature/class-const-dynamic-access-stringable-rule branch 3 times, most recently from 5a4d54a to cfab079 Compare July 11, 2025 17:19

Add class name to message

4e59d4f

zonuexe force-pushed the feature/class-const-dynamic-access-stringable-rule branch from cfab079 to 4e59d4f Compare July 11, 2025 17:21

zonuexe marked this pull request as ready for review July 11, 2025 20:31

ondrejmirtes requested changes Jul 11, 2025

View reviewed changes

zonuexe marked this pull request as draft July 12, 2025 19:30

zonuexe added 2 commits July 13, 2025 05:13

fixup! Add class name to message

f9d4a82

fixup! Add stringable access check to ClassConstantRule

808ed2e

zonuexe force-pushed the feature/class-const-dynamic-access-stringable-rule branch 2 times, most recently from d9cc0de to df669bb Compare July 12, 2025 21:35

fixup! Add stringable access check to ClassConstantRule

b0539eb

zonuexe force-pushed the feature/class-const-dynamic-access-stringable-rule branch from df669bb to b0539eb Compare July 12, 2025 22:14

zonuexe marked this pull request as ready for review July 12, 2025 22:24

ondrejmirtes requested changes Jul 13, 2025

View reviewed changes

fixup! fixup! Add stringable access check to ClassConstantRule

399e22d

zonuexe mentioned this pull request Jul 13, 2025

Rules for checking string types for dynamic access phpstan/phpstan#13238

Open

8 tasks

zonuexe mentioned this pull request Jul 13, 2025

Add stringable access check to AccessPropertiesCheck #4094

Merged

ondrejmirtes merged commit fe4bf2c into phpstan:2.1.x Jul 13, 2025
414 of 419 checks passed

zonuexe deleted the feature/class-const-dynamic-access-stringable-rule branch July 13, 2025 12:30


		private int $phpVersion;

		private bool $checkNonStringableDynamicAccess;


		$type = $nameTypeResult->getType();

		if (!$type->isString()->yes()) {

	if (!$type->isString()->yes()) {
	if (!$type instanceof ErrorType && !$type->isString()->yes()) {

Add stringable access check to ClassConstantRule #3910

Add stringable access check to ClassConstantRule #3910

Uh oh!

Conversation

zonuexe commented Mar 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonuexe Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ondrejmirtes commented Apr 24, 2025

phpstan-bot commented Jul 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phpstan-bot commented Jul 12, 2025

ondrejmirtes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zonuexe commented Jul 13, 2025

Uh oh!

ondrejmirtes commented Jul 13, 2025

Labels

3 participants

zonuexe commented Mar 29, 2025 •

edited

Loading

zonuexe Jul 11, 2025 •

edited

Loading