Add psalm static analysis, improve type inference #21

tux-rampage · 2020-10-23T15:31:26Z

Q	A
Documentation	no
Bugfix	no
BC Break	no
New Feature	yes
RFC	no
QA	yes

Description

As decided by the technical steering committee, components should be covered by psalm static analysis.
This PR attempts to replace phpstan with psalm.

It will also fix reported issues and provide support for templates/generics when possible.

tux-rampage · 2020-10-23T16:04:44Z

Note: Psalm cannot be installed as dependency with php 8 at the moment. See vimeo/psalm#4408

psalm.xml

weierophinney · 2021-04-13T19:35:17Z

@tux-rampage I've rebased your branch based on current 3.3.x, which includes the switch from Travis to GHA for CI purposes. I've also renamed the psalm.xml file to psalm.xml.dist (for consistency with other repos) and ensured an up-to-date composer.lock is available (built using PHP 7.4).

CS checks and Psalm checks are currently failing, which you can now see in the checks list.

tux-rampage

I have added as types as explicit as possible for now. I have decided to not work through all tests since this would make this already big PR even bigger and harder to review. Test-Code issues are added to the baseline. We can keep up with smaller PR to work these things out.

tux-rampage · 2021-09-28T12:32:35Z

composer.json

@@ -26,7 +26,7 @@
        }
    },
    "require": {
-        "php": "^7.3 || ~8.0.0 || ~8.1.0",
+        "php": "^7.4 || ~8.0.0 || ~8.1.0",


PHP 7.3 runs out of support this year. Frankly I'd drop PHP 7 entirely here and push to 8.0 and 8.1 (separate PR)

tux-rampage · 2021-09-28T12:33:55Z

phpcs.xml

@@ -22,10 +22,16 @@
        <exclude-pattern>src/Resolver/AbstractInjection.php</exclude-pattern>
    </rule>

+    <!-- This must be excluded since it is not compatible with php < 8 -->
+    <rule ref="SlevomatCodingStandard.Classes.ModernClassNameReference.ClassNameReferencedViaFunctionCal">
+        <exclude-pattern>*</exclude-pattern>


Since we still have to support 7.4 this rule must be excluded or test cases will fail.

$object::class does not work with php < 8 and tests will fail.

Let's add <config name="php_version" value="70499"/> at the top instead

Perfect, that's way better than my exclude.

tux-rampage · 2021-09-28T12:36:54Z

phpunit.xml.dist

+<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.5/phpunit.xsd"
+         bootstrap="vendor/autoload.php"
+         convertDeprecationsToExceptions="true"


With the most recent PHPUnit update, this must be enabled explicitly.
Is runtime deprecations still a thing? Can we drop them in favour of static analysis tools like psalm.

There are good reasons for not triggering deprecations at runtime. They're also a horror to maintain.

See #30 and #31

src/Config.php

tux-rampage · 2021-09-28T15:30:33Z

src/ConfigProvider.php

-                LegacyInjectorGenerator::class => CodeGenerator\InjectorGenerator::class,
+                'Zend\Di\InjectorInterface'               => InjectorInterface::class,
+                'Zend\Di\ConfigInterface'                 => ConfigInterface::class,
+                'Zend\Di\CodeGenerator\InjectorGenerator' => CodeGenerator\InjectorGenerator::class,


Is this still a thing? Could we drop this since this package is now marked as conflicting to zend-di?

Not really: these string keys were still strings, and not class aliases

Do you have a good idea on how to get rid of this gracefully?

tux-rampage · 2021-09-28T15:36:48Z

test/ConfigTest.php

    {
        $config = new Config($this->fixture);
-        $this->assertEquals(['Foo', 'Bar'], $config->getConfiguredTypeNames());
+        $this->assertEquals([TestAsset\Config\SomeClass::class, 'SomeAlias'], $config->getConfiguredTypeNames());


All changes like this were necessary since we do now type with class-string explicitly. This was caused especially by the typeOf config key which always had to be an existing interface or class name (See docs).

It just revealed a logical bug that slipped due to lax typing.

Ocramius · 2021-09-28T15:58:34Z

phpcs.xml

@@ -22,10 +22,16 @@
        <exclude-pattern>src/Resolver/AbstractInjection.php</exclude-pattern>
    </rule>

+    <!-- This must be excluded since it is not compatible with php < 8 -->
+    <rule ref="SlevomatCodingStandard.Classes.ModernClassNameReference.ClassNameReferencedViaFunctionCal">
+        <exclude-pattern>*</exclude-pattern>


Let's add <config name="php_version" value="70499"/> at the top instead

Ocramius · 2021-09-28T16:05:21Z

src/CodeGenerator/FactoryInterface.php

@@ -6,12 +6,16 @@

 use Psr\Container\ContainerInterface;

+/**
+ * @template T extends object


Will this work in inheritance?

Yepp should be no issue. The chance the this is implemented directly is almost zero.

Oh yes, Implementing classes should use @template-implements. I adjusted the tests for the generated classes.

Edit: See #21 (comment)

Is this a working psalm annotation?
Shouldn't this be @template T of object instead?

Correct, ith schould be of. This was TypeScript comming through.

Ocramius · 2021-09-28T16:05:53Z

src/CodeGenerator/GeneratorTrait.php

@@ -44,6 +49,8 @@ protected function ensureDirectory(string $dir)
     *
     * @throws LogicException
     * @throws GenerateCodeException
+     * @return void
+     * @psalm-assert non-empty-string $this->outputDirectory


Not sure this works with private state 🤔

Yes it does

Here you go: https://psalm.dev/r/205038dd2c

Ocramius · 2021-09-28T16:10:22Z

src/ConfigProvider.php

-                LegacyInjectorGenerator::class => CodeGenerator\InjectorGenerator::class,
+                'Zend\Di\InjectorInterface'               => InjectorInterface::class,
+                'Zend\Di\ConfigInterface'                 => ConfigInterface::class,
+                'Zend\Di\CodeGenerator\InjectorGenerator' => CodeGenerator\InjectorGenerator::class,


Not really: these string keys were still strings, and not class aliases

Ocramius · 2021-09-28T16:10:47Z

src/Container/AutowireFactory.php

@@ -36,7 +37,7 @@ private function getInjector(ContainerInterface $container)
    /**
     * Check creatability of the requested name
     *
-     * @param string $requestedName
+     * @param string|Stringable $requestedName


Oooof, no, we should not really accept Stringable :-\

That was just to keep BC with the config arrays. But I get your point.

Nope you're right, this is actually extending the method signature which was not my intention.
Could we place a native string typehint?

Some may argue that this would somehow be a BC break for user's who ignore the DocBlock and rely on the string cast behavior of this method.

Adding a native string type hint is valid imo, since everything else already violates the existing contract from the DocBlock.

tux-rampage · 2021-09-28T20:17:39Z

test/_files/expected-codegen-results/factories/with-params.php

@@ -28,10 +31,15 @@ public function create(ContainerInterface $container, array $options = [])
                array_key_exists('anyDep', $options) ? $options['anyDep'] : null,
            ];

+        /** @psalm-suppress MixedArgument */
        return new \LaminasTest\Di\TestAsset\Constructor\MixedArguments(...$args);


The interface type would correctly be: public function create(ContainerInterface $container, PartialArray<ConstructArgsArray<T>> $options = []): T

This can only be achieved with a psalm plug-in as far as I know. But this is out of scope of this PR.
To avoid additional overhead in these generated classes, a suppress is added and responsibility is passed to PHP's type system until a better solution can be provided.

tux-rampage · 2021-10-19T22:18:35Z

Thanks for your reviews @boesing and @Ocramius
I've addressed your feedback. Can you please re-check?

boesing

Ooof, well this PR has really plenty of code changes and its not that easy to review. However, I've added a few things which should be addressed.

Especially those null properties which could be empty arrays to make things simpler.

boesing · 2021-11-03T03:15:36Z

phpcs.xml

@@ -1,16 +1,17 @@
 <?xml version="1.0"?>
 <ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="vendor/squizlabs/php_codesniffer/phpcs.xsd">

+    <config name="php_version" value="70499"/>


this shouldn't be necessary as we've changed the CI matrix so that QA tooling (such as codestyle checks) are only executed on the minimum supported PHP version of a component.

As this component requires PHP 7.4 as its minimum, you can remove this line here so it won't lead to issues when removing PHP 7.4.

I see. I guess this happened since I'm running php 8 locally and stumbled upon this.

Kept this: still useful for local runs

boesing · 2021-11-03T03:18:41Z

psalm.xml.dist

+    </issueHandlers>
+
+    <stubs>
+        <file name="./stubs/psr-container.phpstub" />


I prefer using lctrs/psalm-psr-container-plugin but I know that this is a bit opinionated and thus, I know that @Ocramius does not like such magic.

But I'd suggest we go either with the existing psalm-plugin or not.
In both cases, we should drop the stub from this project.

And I agree with @Ocramius since all that Plugin does (as far as I got it) is re-implementing what psalm already can do with templates/generics (and therefore adds a greater potential to break or prevent a psalm upgrade).

Anyways I can replace it with that Plugin. Dropping this without a replacement would require additional typecheck code and make this already big PR even bigger.

Edit: Submitted this comment too soon.

boesing · 2021-11-03T03:19:24Z

src/CodeGenerator/AbstractInjector.php

@@ -13,10 +13,10 @@
 */
 abstract class AbstractInjector implements InjectorInterface
 {
-    /** @var string[]|FactoryInterface[] */
+    /** @var array<string, class-string<FactoryInterface>|FactoryInterface> */


maybe bump minimum requirement of laminas-servicemanager.
I've added plenty of psalm types there which can be imported here for re-usage.

laminas-servicemanager is a dev-dependency and it's not meant to be a requirement.
Importing types from this project would require to make it a runtime dependency just to pass static analysis, which is too much imho.

This is tested against a recent and locked servicemanager: all good

boesing · 2021-11-03T03:24:33Z

src/CodeGenerator/AutoloadGenerator.php

@@ -49,6 +49,7 @@ private function buildFromTemplate(string $templateFile, string $outputFile, arr
        $template = file_get_contents($templateFile);

        assert(is_string($template));
+        assert(is_string($this->outputDirectory));


Imho, thats not safe. This is only true in case someone called the setter. Using php assert in this case is not how it should be used.

https://github.com/laminas/technical-steering-committee/blob/main/meetings/minutes/2021-08-02-TSC-Minutes.md#allow-php-assert-along-with-webmozart-assert-in-specific-cases

You are right. $this->ensureOutputDirectory() would be correct here, which does provide the correct assert and the required directory existence checks / create attempts.

boesing · 2021-11-03T03:24:57Z

src/CodeGenerator/FactoryGenerator.php

@@ -183,6 +183,8 @@ public function generate(string $class): string
        $factoryClassName                          = $this->namespace . '\\' . $this->buildClassName($class);
        [$namespace, $unqualifiedFactoryClassName] = $this->splitFullyQualifiedClassName($factoryClassName);

+        assert(is_string($this->outputDirectory));


Imho, thats not safe. This is only true in case someone called the setter. Using php assert in this case is not how it should be used.

https://github.com/laminas/technical-steering-committee/blob/main/meetings/minutes/2021-08-02-TSC-Minutes.md#allow-php-assert-along-with-webmozart-assert-in-specific-cases

Correct. Same as for the AutoloadGenerator.

boesing · 2021-11-03T03:37:51Z

src/Definition/Reflection/ClassDefinition.php

@@ -69,7 +71,10 @@ public function getInterfaces(): array
        return $this->reflection->getInterfaceNames();
    }

-    private function reflectParameters()
+    /**
+     * @psalm-assert array<string, Parameter> $this->parameters


instead of this, wouldn't it be better to simply set parameters to array with a default value?