You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
When enabling PHPStan's checkUninitializedProperties, using @required to autowire a public property, or through a public method will still mark the property as uninitialized, although Symfony's autowiring will make sure it isn't.
This is on;
phpstan/phpstan 1.10.13
phpstan/phpstan-symfony 1.3.1
Okay, cool. This helps implement the case for public properties with @required or #[Required]. There is another strategy to autowire: through public methods with this annotation or attribute.
I've found \PHPStan\Node\ClassPropertiesNode, which calls \PHPStan\Rules\Properties\ReadWritePropertiesExtension::isInitialized. ClassPropertiesNode has all information needed to implement this, in getPropertyUsages. The PropertyWrite information would allow access to all setting methods, which could be checked for the annotation/attribute. This information, nor the Node/Scope, are made available to the extension, meaning that there is no way to access the setting methods.
<?phpdeclare(strict_types=1);
namespaceInfrastructure\PhpStan;
usePHPStan\Node\ClassStatementsGatherer;
usePHPStan\Reflection\Php\PhpPropertyReflection;
usePHPStan\Reflection\PropertyReflection;
usePHPStan\Rules\Properties\ReadWritePropertiesExtension;
usePHPStan\Type\FileTypeMapper;
useSymfony\Contracts\Service\Attribute\Required;
class SymfonyRequiredInitialization implements ReadWritePropertiesExtension
{
privateFileTypeMapper$fileTypeMapper;
publicfunction__construct(FileTypeMapper$fileTypeMapper)
{
$this->fileTypeMapper = $fileTypeMapper;
}
publicfunctionisAlwaysRead(PropertyReflection$property, string$propertyName): bool
{
returnfalse;
}
publicfunctionisAlwaysWritten(PropertyReflection$property, string$propertyName): bool
{
returnfalse;
}
publicfunctionisInitialized(PropertyReflection$property, string$propertyName): bool
{
// If the property is public, check for @required on the property itselfif ($property->isPublic()) {
if ($property->getDocComment() !== null) {
$phpDoc = $this->fileTypeMapper->getResolvedPhpDoc(null, null, null, null, $property->getDocComment());
foreach ($phpDoc->getPhpDocNodes() as$node) {
// @required annotation is available, meaning this property is always initializedif (count($node->getTagsByName('@required')) > 0) {
returntrue;
}
}
}
// Check for the attribute versionif ($propertyinstanceof PhpPropertyReflection && count($property->getNativeReflection()->getAttributes(Required::class)) > 0) {
returntrue;
}
} else {
// 1. Get property writes of $property// 2. Collect annotations or attributes of the function that writes to $property// 3. Check if any annotation or attribute sets the @required or #[Required]// This will not work because we do not have a node or scope// $classStatementsGatherer = new ClassStatementsGatherer($property->getDeclaringClass(), fn () => null);
}
returnfalse;
}
}
The top part, if ($property->isPublic()) {, is functional (although I'm sure there's ways to improve it). The else part is where the issue lies. The $one in the analysed code will pass correctly, whereas $two will not, but should be as setTwo will make sure it is initialized.
Yeah, this isn't great. I worry there would be performance implications by repeating the work of ClassStatementsGatherer there.
But there's something more promising to do - there's a setting additionalConstructors that 's used for example for PHPUnit's TestCase::setUp().
This setting is currently statically defined in the config, but we could create a new extension that would decide about "additional constructors" programatically.
I've created a new PR for this package now, which should cover this. However, I'm running into a couple things I'm not quite understanding how to solve at this moment. As you can see in the builds, I'm getting these errors;
Error: Although PHPStan\Reflection\Php\PhpPropertyReflection is covered by backward compatibility promise, this instanceof assumption might break because it's not guaranteed to always stay the same.
Error: Creating new PHPStan\Rules\Properties\UninitializedPropertyRule is not covered by backward compatibility promise. The class might change in a minor PHPStan version.
Error: Creating new PHPStan\Reflection\ConstructorsHelper is not covered by backward compatibility promise. The class might change in a minor PHPStan version.
I'm not sure how to fix these. I'm also seeing a couple of similar ones in the baseline, so would baselining these be OK?
Then there's this error;
Error: Class Symfony\Contracts\Service\Attribute\Required not found.
Seems to me like this one should be available, but I must be missing something here.
Activity
ondrejmirtes commentedon Apr 18, 2023
Please contribute an extension as per https://phpstan.org/developing-extensions/always-read-written-properties. Thanks.
raalderink commentedon Apr 18, 2023
Okay, cool. This helps implement the case for public properties with
@required
or#[Required]
. There is another strategy to autowire: through public methods with this annotation or attribute.I've found
\PHPStan\Node\ClassPropertiesNode
, which calls\PHPStan\Rules\Properties\ReadWritePropertiesExtension::isInitialized
.ClassPropertiesNode
has all information needed to implement this, ingetPropertyUsages
. ThePropertyWrite
information would allow access to all setting methods, which could be checked for the annotation/attribute. This information, nor theNode
/Scope
, are made available to the extension, meaning that there is no way to access the setting methods.How should I approach this?
ondrejmirtes commentedon Apr 18, 2023
Please show a piece of code you’re talking about - both analysed and the extension you’re writing.
raalderink commentedon Apr 18, 2023
This is an example piece of code to analyse;
This is the extension thus far;
The top part,
if ($property->isPublic()) {
, is functional (although I'm sure there's ways to improve it). Theelse
part is where the issue lies. The$one
in the analysed code will pass correctly, whereas$two
will not, but should be assetTwo
will make sure it is initialized.ondrejmirtes commentedon Apr 18, 2023
Yeah, this isn't great. I worry there would be performance implications by repeating the work of ClassStatementsGatherer there.
But there's something more promising to do - there's a setting
additionalConstructors
that 's used for example for PHPUnit'sTestCase::setUp()
.This setting is currently statically defined in the config, but we could create a new extension that would decide about "additional constructors" programatically.
Here's the relevant code: https://github.com/phpstan/phpstan-src/blob/1.11.x/src/Reflection/ConstructorsHelper.php
This is very similar thing we previously did for
%stubFiles%
: phpstan/phpstan-src@2ba9332ondrejmirtes commentedon Apr 19, 2023
You can continue here now :) https://github.com/phpstan/phpstan/releases/tag/1.10.14
Update Composer dev tooling dependencies (#116)
Set properties autowired with @required as initialized
Set properties autowired with @required as initialized
raalderink commentedon Apr 21, 2023
I've created a new PR for this package now, which should cover this. However, I'm running into a couple things I'm not quite understanding how to solve at this moment. As you can see in the builds, I'm getting these errors;
I'm not sure how to fix these. I'm also seeing a couple of similar ones in the baseline, so would baselining these be OK?
Then there's this error;
Seems to me like this one should be available, but I must be missing something here.