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.

Sign up for GitHub

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

Add stringable access check to ClassConstantRule #3910

Draft
wants to merge 1 commit into
base: 2.1.x
Choose a base branch
from
Draft

Add stringable access check to ClassConstantRule #3910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions conf/bleedingEdge.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
parameters:
featureToggles:
bleedingEdge: true
checkNonStringableDynamicAccess: true
checkParameterCastableToNumberFunctions: true
skipCheckGenericClasses!: []
stricterFunctionMap: true
Expand Down
6 changes: 6 additions & 0 deletions conf/config.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ parameters:
tooWideThrowType: true
featureToggles:
bleedingEdge: false
checkNonStringableDynamicAccess: false
checkParameterCastableToNumberFunctions: false
skipCheckGenericClasses: []
stricterFunctionMap: false
Expand Down Expand Up @@ -882,6 +883,11 @@ services:
-
class: PHPStan\Rules\ClassForbiddenNameCheck

-
class: PHPStan\Rules\Classes\ClassConstantRule
arguments:
checkNonStringableDynamicAccess: %featureToggles.checkNonStringableDynamicAccess%
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Instead, config.level0.neon should be modified:

  1. Remove the rule from rules section.
  2. Add the service same way you did here.
  3. But also add tags section with the corresponding tag.


-
class: PHPStan\Rules\Classes\LocalTypeAliasesCheck
arguments:
Expand Down
1 change: 1 addition & 0 deletions conf/parametersSchema.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ parametersSchema:
])
featureToggles: structure([
bleedingEdge: bool(),
checkNonStringableDynamicAccess: bool(),
checkParameterCastableToNumberFunctions: bool(),
skipCheckGenericClasses: listOf(string()),
stricterFunctionMap: bool()
Expand Down
19 changes: 19 additions & 0 deletions src/Rules/Classes/ClassConstantRule.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public function __construct(
private RuleLevelHelper $ruleLevelHelper,
private ClassNameCheck $classCheck,
private PhpVersion $phpVersion,
private bool $checkNonStringableDynamicAccess = true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

)
{
}
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Delete this line, it's not used anywhere.

$typeResult = $this->ruleLevelHelper->findTypeToCheck(
$scope,
$node->name,
'',
static fn (Type $type) => $type->toString()->isString()->yes()
);
Copy link
Member

Choose a reason for hiding this comment

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

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.


if (! $typeResult->getType()->isString()->yes() ||
$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')
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

->build();
}
}
}

foreach ($constantNameScopes as $constantName => $constantScope) {
Expand Down
60 changes: 60 additions & 0 deletions tests/PHPStan/Rules/Classes/ClassConstantRuleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ class ClassConstantRuleTest extends RuleTestCase

private int $phpVersion;

private bool $checkNonStringableDynamicAccess;
Copy link
Member

Choose a reason for hiding this comment

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

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


protected function getRule(): Rule
{
$reflectionProvider = $this->createReflectionProvider();
Expand All @@ -30,12 +32,14 @@ protected function getRule(): Rule
new ClassForbiddenNameCheck(self::getContainer()),
),
new PhpVersion($this->phpVersion),
$this->checkNonStringableDynamicAccess,
);
}

public function testClassConstant(): void
{
$this->phpVersion = PHP_VERSION_ID;
$this->checkNonStringableDynamicAccess = true;
$this->analyse(
[
__DIR__ . '/data/class-constant.php',
Expand Down Expand Up @@ -99,6 +103,7 @@ public function testClassConstant(): void
public function testClassConstantVisibility(): void
{
$this->phpVersion = PHP_VERSION_ID;
$this->checkNonStringableDynamicAccess = true;
$this->analyse([__DIR__ . '/data/class-constant-visibility.php'], [
[
'Access to private constant PRIVATE_BAR of class ClassConstantVisibility\Bar.',
Expand Down Expand Up @@ -168,6 +173,7 @@ public function testClassConstantVisibility(): void
public function testClassExists(): void
{
$this->phpVersion = PHP_VERSION_ID;
$this->checkNonStringableDynamicAccess = true;
$this->analyse([__DIR__ . '/data/class-exists.php'], [
[
'Class UnknownClass\Bar not found.',
Expand Down Expand Up @@ -242,12 +248,14 @@ public function dataClassConstantOnExpression(): array
public function testClassConstantOnExpression(int $phpVersion, array $errors): void
{
$this->phpVersion = $phpVersion;
$this->checkNonStringableDynamicAccess = true;
$this->analyse([__DIR__ . '/data/class-constant-on-expr.php'], $errors);
}

public function testAttributes(): void
{
$this->phpVersion = PHP_VERSION_ID;
$this->checkNonStringableDynamicAccess = true;
$this->analyse([__DIR__ . '/data/class-constant-attribute.php'], [
[
'Access to undefined constant ClassConstantAttribute\Foo::BAR.',
Expand Down Expand Up @@ -287,18 +295,21 @@ public function testRuleWithNullsafeVariant(): void
}

$this->phpVersion = PHP_VERSION_ID;
$this->checkNonStringableDynamicAccess = true;
$this->analyse([__DIR__ . '/data/class-constant-nullsafe.php'], []);
}

public function testBug7675(): void
{
$this->phpVersion = PHP_VERSION_ID;
$this->checkNonStringableDynamicAccess = true;
$this->analyse([__DIR__ . '/data/bug-7675.php'], []);
}

public function testBug8034(): void
{
$this->phpVersion = PHP_VERSION_ID;
$this->checkNonStringableDynamicAccess = true;
$this->analyse([__DIR__ . '/data/bug-8034.php'], [
[
'Access to undefined constant static(Bug8034\HelloWorld)::FIELDS.',
Expand All @@ -310,6 +321,7 @@ public function testBug8034(): void
public function testClassConstFetchDefined(): void
{
$this->phpVersion = PHP_VERSION_ID;
$this->checkNonStringableDynamicAccess = true;
$this->analyse([__DIR__ . '/data/class-const-fetch-defined.php'], [
[
'Access to undefined constant ClassConstFetchDefined\Foo::TEST.',
Expand Down Expand Up @@ -411,6 +423,7 @@ public function testPhpstanInternalClass(): void
$tip = 'This is most likely unintentional. Did you mean to type \AClass?';

$this->phpVersion = PHP_VERSION_ID;
$this->checkNonStringableDynamicAccess = true;
$this->analyse([__DIR__ . '/data/phpstan-internal-class.php'], [
[
'Referencing prefixed PHPStan class: _PHPStan_156ee64ba\AClass.',
Expand All @@ -427,6 +440,7 @@ public function testClassConstantAccessedOnTrait(): void
}

$this->phpVersion = PHP_VERSION_ID;
$this->checkNonStringableDynamicAccess = true;
$this->analyse([__DIR__ . '/data/class-constant-accessed-on-trait.php'], [
[
'Cannot access constant TEST on trait ClassConstantAccessedOnTrait\Foo.',
Expand All @@ -442,8 +456,17 @@ public function testDynamicAccess(): void
}

$this->phpVersion = PHP_VERSION_ID;
$this->checkNonStringableDynamicAccess = true;

$this->analyse([__DIR__ . '/data/dynamic-constant-access.php'], [
[
'Access to undefined constant ClassConstantDynamicAccess\Foo::FOO.',
17,
],
[
'Cannot fetch class constant with a non-stringable type object.',
19,
],
[
'Access to undefined constant ClassConstantDynamicAccess\Foo::FOO.',
20,
Expand Down Expand Up @@ -479,4 +502,41 @@ public function testDynamicAccess(): void
]);
}

public function testStringableDynamicAccess(): void
{
if (PHP_VERSION_ID < 80300) {
$this->markTestSkipped('Test requires PHP 8.3.');
}

$this->phpVersion = PHP_VERSION_ID;
$this->checkNonStringableDynamicAccess = true;

$this->analyse([__DIR__ . '/data/dynamic-constant-stringable-access.php'], [
[
'Cannot fetch class constant with a non-stringable type mixed.',
13,
],
[
'Cannot fetch class constant with a non-stringable type string|null.',
14,
],
[
'Cannot fetch class constant with a non-stringable type Stringable|null.',
15,
],
[
'Cannot fetch class constant with a non-stringable type int.',
16,
],
[
'Cannot fetch class constant with a non-stringable type int|null.',
17,
],
[
'Cannot fetch class constant with a non-stringable type DateTime|string.',
18,
],
]);
}

}
3 changes: 1 addition & 2 deletions tests/PHPStan/Rules/Classes/data/dynamic-constant-access.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public function test(string $string, object $obj): void
{
$bar = 'FOO';

echo self::{$foo};
echo self::{$bar};
echo self::{$string};
echo self::{$obj};
echo self::{$this->name};
Expand Down Expand Up @@ -44,5 +44,4 @@ public function testScope(): void
echo self::{$name};
}


}
21 changes: 21 additions & 0 deletions tests/PHPStan/Rules/Classes/data/dynamic-constant-stringable-access.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php // lint >= 8.3

namespace ClassConstantDynamicStringableAccess;

use Stringable;
use DateTime;

final class Foo
{

public function test(mixed $mixed, ?string $nullableStr, ?Stringable $nullableStringable, int $int, ?int $nullableInt, DateTime|string $datetimeOrStr): void
{
echo self::{$mixed};
echo self::{$nullableStr};
echo self::{$nullableStringable};
echo self::{$int};
echo self::{$nullableInt};
echo self::{$datetimeOrStr};
}

}
Loading