-
Notifications
You must be signed in to change notification settings - Fork 523
Add stringable access check to ClassConstantRule #3910
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
Changes from 5 commits
8e8bcd5
4e59d4f
f9d4a82
808ed2e
b0539eb
399e22d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -5,9 +5,11 @@ | |||||
use PhpParser\Node; | ||||||
use PhpParser\Node\Expr\BinaryOp\Identical; | ||||||
use PhpParser\Node\Expr\ClassConstFetch; | ||||||
use PhpParser\Node\Name; | ||||||
use PhpParser\Node\Scalar\String_; | ||||||
use PHPStan\Analyser\NullsafeOperatorHelper; | ||||||
use PHPStan\Analyser\Scope; | ||||||
use PHPStan\DependencyInjection\AutowiredParameter; | ||||||
use PHPStan\DependencyInjection\RegisteredRule; | ||||||
use PHPStan\Internal\SprintfHelper; | ||||||
use PHPStan\Php\PhpVersion; | ||||||
|
@@ -42,6 +44,8 @@ public function __construct( | |||||
private RuleLevelHelper $ruleLevelHelper, | ||||||
private ClassNameCheck $classCheck, | ||||||
private PhpVersion $phpVersion, | ||||||
#[AutowiredParameter(ref: '%featureToggles.checkNonStringableDynamicAccess%')] | ||||||
private bool $checkNonStringableDynamicAccess, | ||||||
) | ||||||
{ | ||||||
} | ||||||
|
@@ -63,6 +67,27 @@ 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) { | ||||||
$nameTypeResult = $this->ruleLevelHelper->findTypeToCheck( | ||||||
$scope, | ||||||
$node->name, | ||||||
'', | ||||||
static fn (Type $type) => $type->isString()->yes(), | ||||||
); | ||||||
|
||||||
$type = $nameTypeResult->getType(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
|
||||||
if (!$type->isString()->yes()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All calls to findTypeToCheck follow this pattern. |
||||||
$className = $node->class instanceof Name | ||||||
? $scope->resolveName($node->class) | ||||||
: $scope->getType($node->class)->describe(VerbosityLevel::typeOnly()); | ||||||
|
||||||
$errors[] = RuleErrorBuilder::message(sprintf('Class constant name for %s must be a string, but %s was given.', $className, $nameType->describe(VerbosityLevel::precise()))) | ||||||
->identifier('classConstant.nameNotString') | ||||||
->build(); | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
foreach ($constantNameScopes as $constantName => $constantScope) { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
<?php // lint >= 8.3 | ||
|
||
namespace ClassConstantDynamicStringableAccess; | ||
|
||
use Stringable; | ||
use DateTime; | ||
use DateTimeImmutable; | ||
|
||
abstract class Foo | ||
{ | ||
|
||
public function test(mixed $mixed, ?string $nullableStr, ?Stringable $nullableStringable, int $int, ?int $nullableInt, DateTime|string $datetimeOrStr, Stringable $stringable): void | ||
{ | ||
echo self::{$mixed}; | ||
echo self::{$nullableStr}; | ||
echo self::{$nullableStringable}; | ||
echo self::{$int}; | ||
echo self::{$nullableInt}; | ||
echo self::{$datetimeOrStr}; | ||
echo self::{1111}; | ||
echo self::{(string)$stringable}; | ||
echo self::{$stringable}; // Uncast Stringable objects will cause a runtime error | ||
} | ||
|
||
} | ||
|
||
final class Bar extends Foo | ||
{ | ||
|
||
public function test(mixed $mixed, ?string $nullableStr, ?Stringable $nullableStringable, int $int, ?int $nullableInt, DateTime|string $datetimeOrStr, Stringable $stringable): void | ||
{ | ||
echo parent::{$mixed}; | ||
echo self::{$mixed}; | ||
} | ||
|
||
public function testClassDynamic(DateTime|DateTimeImmutable $datetime, object $obj, mixed $mixed): void | ||
{ | ||
echo $datetime::{$mixed}; | ||
echo $obj::{$mixed}; | ||
} | ||
|
||
} |
There was a problem hiding this comment.
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:
Some rules return "unknown class errors" but that's irrelevant here.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dynamic fetching of constants doesn't do implicit casting, so it seems appropriate to simply check
isString()->yes()
withouttoString()
.I was wrong in the early stages of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rename the variable to
$nameTypeResult
.