[DI] Add compiler pass to check arguments type hint #27825

alcalyn · 2018-07-03T12:41:57Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#27744
License	MIT
Doc PR

Actually, when you inject a service to another through constructor/method/factory, if the type hint is invalid, you get an error at runtime only.

This PR adds a pass to check type hints during compilation.

See RFC: #27744

Testing with a Symfony skeleton

Assuming you have symfony installed and fetched this branch:

# install a project skeleton in another directory
$ pwd
/var/www/symfony
$ cd ../
$ composer create-project symfony/skeleton

# make skeleton use your symfony with this feature
cd symfony/
./link ../skeleton

# run lint:container command
cd ../skeleton/
./bin/console lint:container

To make linter display an error

Add this services configuration to test the command:

# config/services.yaml

    s1:
        class: stdClass

    my_service:
        class: App\MyService
        arguments:
            - "@logger" # either this line, (should fail)
            - "@s1"     # or this one (should success)

It needs this fake service to be created:

# src/MyService.php
<?php

namespace App;

class MyService
{
    public function __construct(\stdClass $arg)
    {
    }
}

Then run the command:

$ ./bin/console lint:container

  Invalid definition for service "my_service": argument 0 of "App\MyService::__construct" requires  
   a "stdClass", "Symfony\Component\HttpKernel\Log\Logger" passed.

You should the error when passing an instance of logger to MyService constructor whereas it expects an instance of stdClass.

nicolas-grekas

Looks really nice, here are some comments to fine tune the PR.
I think we should now wonder how this should be used.
My preference would be to find a way to use this pass in a single dedicated test case, but I'm not sure how hard this would be to achieve. WDYT?

nicolas-grekas · 2018-07-03T20:10:18Z

src/Symfony/Component/DependencyInjection/CHANGELOG.md

@@ -5,6 +5,7 @@ CHANGELOG
 -----

 * added `ServiceSubscriberTrait`
+ * added CheckTypeHintsPass to check injected parameters type during compilation


missing backticks around CheckTypeHintsPass

nicolas-grekas · 2018-07-03T20:10:50Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+{
+    private $autoload;
+
+    public function __construct(bool $autoload = false)


we could have a sentence telling what $autoload is useful for

nicolas-grekas · 2018-07-03T20:12:26Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+        }
+
+        // When service is declared with $container->register(Foo::class); and not $container->register(Foo::class, Foo::class);
+        if (null === $value->getClass()) {


not sure we need this case: when autoload is false, the bellow if will catch it, and when autoload is true, this pass should be registered after all the other passes, so that this situation won't exist then

removed it and all tests still pass

nicolas-grekas · 2018-07-03T20:13:14Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+        foreach ($value->getMethodCalls() as $methodCall) {
+            $reflectionMethod = $this->getReflectionMethod($value, $methodCall[0]);
+
+            $this->checkArgumentsTypeHints($reflectionMethod /* warn */, $methodCall[1]);


nicolas-grekas · 2018-07-03T20:14:25Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+    /**
+     * Check type hints for every parameter of a method/constructor.
+     *
+     * @param \ReflectionMethod $reflectionMethod


should be removed: we don't add annotations when they just duplicate the signature (same everywhere in the PR)

nicolas-grekas · 2018-07-03T20:41:54Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+     *
+     * @param mixed $value
+     *
+     * @return string


should be replaced by a real return type

nicolas-grekas · 2018-07-03T20:42:43Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+            } elseif (null === $class) {
+                $class = $value->getClass();
+            } elseif ($class instanceof Definition) {
+                $class = $class->getClass();


$class = $this->getClassName($class);

nicolas-grekas · 2018-07-03T20:46:56Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+                $class = $class->getClass();
+            }
+        } else {
+            $class = is_string($factory) ? null : $value->getClass();


should we remove the is_string check?

Unit tests say yes, so removed it

nicolas-grekas · 2018-07-03T20:48:17Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+        if (is_array($factory = $value->getFactory())) {
+            list($class, $method) = $factory;
+            if ($class instanceof Reference) {
+                $class = $this->container->findDefinition((string) $class)->getClass();


my mistake I think: here we're looking for the class of a referenced definition - not sure the class of its factory should be used for the validation. just the class of the definition, factory or not, should be more correct. WDYT?

I'm not sure about this part as only one test enters inside the if (is_array($factory = $value->getFactory())) {, so I'd say factories uses cases are not enough covered

For me too we should always return $value->getClass(), we only need to handle factories for the constructor and it's done by getConstructor. Since getClassName is used to check whether a definition implements the right type hint, it should return the definition class.

nicolas-grekas · 2018-07-03T20:50:33Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+            return parent::processValue($value, $isRoot);
+        }
+
+        if (!$this->autoload && !\class_exists($this->getClassName($value), false)) {


interface_exists also on the definition's class (same for the other class_exists below)
there is a case where we load code here: when there is factory, the class checked here can be different than the class loaded below when loading methods, isn't it? (TDD should tell :) )

alcalyn · 2018-07-04T10:36:05Z

Also I think the autowired cases are not covered. I tested my pass in a symfony skeleton, and this exception is thrown:

if (count($configurationArguments) < $numberOfRequiredParameters) {
    throw new InvalidArgumentException(sprintf(
        'Invalid definition for service "%s": "%s::%s()" requires %d arguments, %d passed.', $this->currentId, $reflectionMethod->class, $reflectionMethod->name, $numberOfRequiredParameters, count($configurationArguments)));
}

I needed to register the AutowirePass before this pass:

$container->addCompilerPass(new AutowirePass());
$container->addCompilerPass(new CheckTypeHintsPass());

I think I'll add a notice about this, in exemple in pass docblock. Is there a better way to declare the dependency to another pass ?

alcalyn · 2018-07-04T10:49:45Z

I think we should now wonder how this should be used.
My preference would be to find a way to use this pass in a single dedicated test case, but I'm not sure how hard this would be to achieve. WDYT?

I think the pass can be integrated in the skeleton, in dev environment as error messages are clearer: why can now be aware of which service is badly configured.

Or, maybe we could create a command like lint:container that run all Check*Pass. Or a production pre-check command.

Fun fact: if we integrate the pass in skeleton, we have at least one type hint to fix:

Invalid definition for service "web_profiler.debug_toolbar": argument 1 of "Symfony\Bundle\WebProfilerBundle\EventListener\WebDebugToolbarListener::__construct" requires a "bool", "string" passed.

alcalyn · 2018-07-04T15:50:25Z

See together, about the autowiring, I'm ok with the fact that we can register the pass after others with:

$container->addCompilerPass(new CheckTypeHintsPass(), PassConfig::TYPE_AFTER_REMOVING);

Doing this works better, however I'm getting another error I can't reproduce in DependencyInjection component: Invalid definition for service "App\Controller\HelloController": argument 0 of "App\Controller\HelloController::__construct" requires a "App\Service\Newsletter", "object" passed.

I'll maybe catch this when working on functional tests in FrameworkBundle.

About strict mode, I removed support of this and ignore them so that the pass won't throw exception when passing i.e a string in a bool parameter.

GuilhemN

Great addition, should it also check factories return type?

GuilhemN · 2018-07-05T08:38:42Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+            return parent::processValue($value, $isRoot);
+        }
+
+        if (!$this->autoload && !class_exists($this->getClassName($value), false)) {


As @nicolas-grekas said, there should also be an interface_exists check.

GuilhemN · 2018-07-05T09:08:12Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+        }
+
+        foreach ($value->getMethodCalls() as $methodCall) {
+            $reflectionMethod = $this->getReflectionMethod($value, $methodCall[0]);


In case an interface is used, the real implementation may have methods the interface doesn't have. Should we support this case and not throw an exception for unexistent methods for objects not instantiated by sf (factories and shared services at least)?

javiereguiluz · 2018-07-06T08:03:11Z

This looks really great! Thanks @alcalyn. When this PR is finished, and just before merging it, could we please profile a real Symfony app to verify that the performance has not degraded significantly? Thanks!

alcalyn · 2018-07-06T08:28:54Z

Hello @javiereguiluz , Thanks to you ! For now we are not planning to run this pass on every compilation, but on demand, in a phpunit test or with a command.

But yes this sounds great, we should measure the cost of this pass to evaluate if it makes sense to register it for dev environment.

javiereguiluz · 2018-07-06T08:54:36Z

Something like lint:container? Sounds great! If this is really executed on demand, then it doesn't matter if it's a bit slow. Thanks!

xabbuh · 2018-07-06T09:46:24Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+
+/**
+ * Checks whether injected parameters types are compatible with type hints.
+ * This pass should be added before removing (PassConfig::TYPE_BEFORE_REMOVING).


Shouldn't that happen after removing passes have been executed to ignore invalid definitions?

I think no because it removes also user's unused service that may have bad type argument. Then it won't display the type hint error, whereas I expect all my services are checked, even if not (yet) used in my application.

IMO that's something we can live with. My reasoning here is that we could otherwise end up with errors for third-party bundles we don't control which is probably a worse experience than having to fix your own config at a later time when your service is actually used. If your own removed services are not checked, that's not a big deal IMO as they won't be used at runtime anyway.

Ok I understand, this is a different point of view which is also good. I'd say before removing is still relevant as it could be used in continuous quality to detect errors in services definitions before they're used. I'll update the comment to say that both before and after can be used, and explain differences.

…taining another Definition

alcalyn · 2018-08-14T12:52:00Z

Hi, I have not enough in in August as I have holidays and a talk to prepare. I'd have some time to work on this as of September...

fabpot · 2018-10-10T10:39:15Z

@alcalyn Do you think you will have time to work on this PR in the near future?

alcalyn · 2018-10-10T12:07:07Z

Yes, I restarted to work on this PR last week, but I'm still stuck on few errors I get when running the pass with lint:container, and how I will finish it

Matts · 2019-06-06T07:47:38Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+        $numberOfRequiredParameters = $reflectionFunction->getNumberOfRequiredParameters();
+
+        if (count($configurationArguments) < $numberOfRequiredParameters) {
+            throw new InvalidArgumentException(sprintf(


This implementation only shows an error if not enough parameters are given.

Wouldn't it be nice to see where too many parameters are given. Even though the compiler will not throw an error; a warning would be nice. This, for example, may occur if someone changes the signature of the constructor without changing the service definition...

This may prevent issues on the long term as someone may re-add a parameter to the signature, what will then break all implementations.

Matts · 2019-06-06T07:50:19Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+                return;
+            }
+
+            if ('Traversable' === $parameter->getType()->getName() && $configurationArgument instanceof IteratorArgument) {


Personally, I do not like this, as it seems that iterable and Traversable seems to be case sensitive.

iterable does seem to be case insensitive as it's a php builtin type, however Traversable is case sensitive indeed.

Matts · 2019-06-06T07:51:09Z

src/Symfony/Component/DependencyInjection/Compiler/CheckTypeHintsPass.php

+                return;
+            }
+
+            $checkFunction = 'is_'.$parameter->getType()->getName();


sprintf('is_%s', $parameter->getType()->getName()) can be used here. Looks more neat than appending

nicolas-grekas · 2019-06-28T13:14:39Z

Continued in #32256, thanks for working on this.

…ces wiring matches type declarations (alcalyn, GuilhemN, nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [DI] Add compiler pass and command to check that services wiring matches type declarations | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27744 | License | MIT | Doc PR | PR replacing #27825. It adds a `lint:container` command asserting the type hints used in your code are correct. Commits ------- 8230a15 Make it really work on real apps 4b3e9d4 Fix comments, improve the feature a6292b9 [DI] Add compiler pass to check arguments type hint

…ces wiring matches type declarations (alcalyn, GuilhemN, nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [DI] Add compiler pass and command to check that services wiring matches type declarations | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27744 | License | MIT | Doc PR | PR replacing symfony/symfony#27825. It adds a `lint:container` command asserting the type hints used in your code are correct. Commits ------- 8230a1543e Make it really work on real apps 4b3e9d4c96 Fix comments, improve the feature a6292b917b [DI] Add compiler pass to check arguments type hint

carsonbot added Status: Needs Review Feature labels Jul 3, 2018

alcalyn changed the title ~~[#27744] Add compiler pass to check arguments type hint~~ [DI] Add compiler pass to check arguments type hint Jul 3, 2018

alcalyn force-pushed the feature/check-type-hints-pass branch 2 times, most recently from 2cc0647 to fbe56a4 Compare July 3, 2018 14:33

nicolas-grekas added this to the next milestone Jul 3, 2018

nicolas-grekas reviewed Jul 3, 2018

View reviewed changes

alcalyn force-pushed the feature/check-type-hints-pass branch 2 times, most recently from 83d05b8 to 702aa51 Compare July 4, 2018 15:42

alcalyn force-pushed the feature/check-type-hints-pass branch from b0fec89 to 9aae7a2 Compare July 4, 2018 16:13

GuilhemN reviewed Jul 5, 2018

View reviewed changes

xabbuh reviewed Jul 6, 2018

View reviewed changes

alcalyn force-pushed the feature/check-type-hints-pass branch 2 times, most recently from d701691 to 3b5d4b2 Compare July 9, 2018 19:20

alcalyn added 2 commits July 9, 2018 22:19

[symfony#27744] Add compiler pass to check arguments type hint

4b59201

update phpdoc

1aefc17

alcalyn force-pushed the feature/check-type-hints-pass branch from 3b5d4b2 to 3e9c37d Compare July 9, 2018 20:19

add missing case with 'iterable' type hint

cc8c4de

alcalyn force-pushed the feature/check-type-hints-pass branch from 3e9c37d to cc8c4de Compare July 9, 2018 20:22

lint:container command

ddba549

alcalyn force-pushed the feature/check-type-hints-pass branch from 4f02c27 to ddba549 Compare July 11, 2018 23:11

alcalyn added 3 commits July 12, 2018 13:45

handle case when passing array to Traversable, ignore definitions con…

9740eee

…taining another Definition

command lint:container allows to check only used services

361e8a6

lint

b305a69

nicolas-grekas self-requested a review July 29, 2018 15:33

nicolas-grekas mentioned this pull request Jun 5, 2019

[DI] Validate constructor arguments #31882

Closed

Matts reviewed Jun 6, 2019

View reviewed changes

GuilhemN mentioned this pull request Jun 28, 2019

[DI] Add compiler pass and command to check that services wiring matches type declarations #32256

Merged

nicolas-grekas closed this Jun 28, 2019

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

fu22ybear mentioned this pull request Jan 1, 2021

Service constructor arguments validating #39678

Closed

Uh oh!

[DI] Add compiler pass to check arguments type hint #27825

[DI] Add compiler pass to check arguments type hint #27825

Uh oh!

Conversation

Uh oh!

Testing with a Symfony skeleton

To make linter display an error

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!