Add return-types with help from DebugClassLoader in the CI #33236

nicolas-grekas · 2019-08-19T09:41:22Z

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

I've spent a great deal of time on this PR, experimenting with adding return types to the codebase.

TL;DR: my conclusion is that we cannot make it for 5.0.

There are two reasons for this:

The burden this will put on the community is immense, especially when considering that third party libs must also be updated for any apps to work at all on a return-typed 5.0. Symfony must add them last, not first.
We need return type covariance, yet this won't be available before PHP 7.4, while 5.0 supports 7.2.

What's attached?

~~a draft patching logic in DebugClassLoader to add return-type where it discovers this should be done~~
return types added automatically thanks to [ErrorHandler] make DebugClassLoader able to add return type declarations #33283
~~manual fixes for situations not handled (yet, if possible at all) by that logic in DebugClassLoader~~ [3.4] Fix return types declarations #33332

What's achieved? Tests are green \o/

At this stage, I think we have to acknowledge we won't add return-types in 5.0 but prepare a serious plan to add them in 6.0.

This plan could be:

make DebugClassLoader able to automate adding return types.
in 4.4: add all possible return types that don't break BC, e.g. in Tests and in generated code
spot and fix places where annotations aren't accurate, add more annotations where possible.
ensure DebugClassLoader triggers the best possible deprecations that encourage ppl to add return-types in their libs/apps. This means we could decide to disable the current ones (see Return type deprecations for library tests #33235) and to re-enable them in 5.1. This will also give us the time to fine-tune the tooling (item 1. on this list)

Ideally, we could reach a point where we could test branch 4.4 with return-types: we'd use the tooling to add them automatically in the CI job, then we'd run tests and they should be green. Let's do this?

Help Wanted, here is how:

With PHP 7.4, run php .github/patch-types.php. This will add return types everywhere possible.

Then run tests, e.g. ./phpunit src/Symfony/Component/HttpFoundation --exclude-group legacy,issue-32995

Here are the components that fail with return types added, please help me check them all with a PR on my fork:

fabpot · 2019-08-19T10:59:33Z

Sounds like a solid plan to me. We are not in a hurry. We need to carefully build a plan and targeting 6.0 is probably the thing to do.

derrabus · 2019-08-19T12:20:11Z

First of all, thank you very much for all the effort you have put into this!

TL;DR: my conclusion is that we cannot make it for 5.0.

That's a sad and disappointing conclusion. 😞 But I refuse to give up at this point – especially after the wall of text that I posted last night as #33228. 😉 We might not get fully typed, but I'm still convinced we can get further than we are at the moment.

We already have added a few return types (mainly because we were somewhat forced to: #27905) and probably will be required to add a few more (#33039).

The burden this will put on the community is immense,

Yes, but that'll still be the case two years from now. The main difference I see is that we won't have a maintained Symfony version anymore that still supports php 5. I admit, that might help a little. 🤔

especially when considering that third party libs must also be updated for any apps to work at all on a return-typed 5.0.

Why's that? Having types in Symfony does not require the whole world to release typed versions of their libraries as well.

Third-party libraries only need to be updated if they implement/extend Symfony interfaces/classes that received types with 5.0. Perhaps we can find some problematic hot spots and exclude them when adding return types, such as (list might be incomplete):

Symfony\Component\Config\Definition\ConfigurationInterface
Symfony\Component\DependencyInjection\Extension\ExtensionInterface
Symfony\Component\EventDispatcher\EventSubscriberInterface
Symfony\Component\HttpKernel\Bundle\BundleInterface

That could ease the migration for many third-party bundles.

We need return type covariance, yet this won't be available before PHP 7.4, while 5.0 supports 7.2.

Can you elaborate on this point? I mean, having full covariance is certainly nice to have, but wouldn't call it a requirement. Apart from fluent interfaces (for which we have found a reasonable compromise), where would we actually depend on covariance? Maybe we can exclude those classes when adding return types or work with a combination of a more generic return type declaration and a more specific @return annotation here? We should even be able to teach DebugClassLoader to detect such cases.

I mean, there might always be features in newer php releases that might come handy. Who knows, maybe in two years from now, generics have been added to php 8.1 and then we say: "Hey, we can't add return types because we desperately need generics to go fully typed!"

in 4.4: add all possible return types that don't break BC, e.g. in Tests and in generated code

For 5.0: At least add return types to everything that is flagged as @internal? 🥺

spot and fix places where annotations aren't accurate, add more annotations where possible.

I expect to find most of these cases when actually adding return types. 🤔

Ideally, we could reach a point where we could test branch 4.4 with return-types: we'd use the tooling to add them automatically in the CI job, then we'd run tests and they should be green. Let's do this?

Yes! Even if we will never add types, we would end up with properly configured mocks and a codebase without inconsistent return points. Wouldn't be too bad after all. 😎

In preparation of the issue I wrote yesterday, I've done some experiments on my own. I followed a more manual approach, using PhpStorm inspections and refactorings. One result that I already have is a typed version of the EventDispatcher. I will post a PR later – even if we don't merge it, it might still be a good reference. Also, we might want to cherry-pick the changes I've made to the tests. 😃

nicolas-grekas · 2019-08-19T15:19:23Z

That's a sad and disappointing conclusion.

The issue is complex enough, let's keep a cool head. I don't think it's sad when it's a well-crafted engineering decision, and when there's a plan forward to bring return-types in a less brutal way.

We already have added a few return types (mainly because we were somewhat forced to: #27905) and probably will be required to add a few more (#33039).

True, that's the cost of having third-party dependencies. Yet the scope is very limited, it's not the same story than doing it for the whole codebase.

The burden this will put on the community is immense,
Yes, but that'll still be the case two years from now.

Absolutely not, you missed the last item on my plan above: we would throw deprecation notices starting from 5.1 (the same as DebugClassLoader does right now), so that ppl would be encouraged to add return types well ahead of v6.

Having types in Symfony does not require the whole world to release typed versions of their libraries as well.
Third-party libraries only need to be updated if they implement/extend Symfony interfaces/classes

That's enough a blocker. ALL lines changed in this PR are individual BC breaks. That's way too abrupt. This can make v5 fail totally, as far as adoption is concerned.

having full covariance is certainly nice to have, but wouldn't call it a requirement.

That's what I was reminded when working on this PR: e.g. FrameworkBundle's Client::doRequest() returns Response, while the parent returns object. That's totaly legal. Form has similar needs for covariance, DoctrineBridge too. DebugClassLoader can detect them, but cannot change this: if ppl add the parent's return type as they would be forced to do if we add them, it will be impossible to add those more specific return types ever.

For 5.0: At least add return types to everything that is flagged as @internal? 🥺

Yes, or 4.4.

Even if we will never add types, we would end up with properly configured mocks and a codebase without inconsistent return points. Wouldn't be too bad after all.

😎

Tobion · 2019-08-19T15:33:21Z

For 5.0: At least add return types to everything that is flagged as @internal? 🥺

Yes, or 4.4.

We could also add return types to final classes, ref. #33152

…rekas) This PR was merged into the 4.4 branch. Discussion ---------- [DI] add return-types to generated containers | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - A chunk of #33236 ready for 4.4 Commits ------- 9170919 [DI] add return-types to generated containers

derrabus · 2019-08-19T17:09:15Z

That's a sad and disappointing conclusion.

The issue is complex enough, let's keep a cool head. I don't think it's sad when it's a well-crafted engineering decision, and when there's a plan forward to bring return-types in a less brutal way.

Sure, I meant no offense. After all the effort that has gone into this already, of course I am disappointed if the conclusion is that we won't proceed for now. That doesn't mean that you've made a bad decision. Your plan sounds reasonable and we really might be in a better position two years from now.

Yet, you make it sound like we have to decide between two extreme options: Leaving everything as it is or going 100%. I was trying to find a middle ground. Adding types where the impact is low. Of course, estimating the impact of each change is hard and nothing we could do fully automated.

But this is ultimately not my decision. If the plan is to make adding return types a smooth two-year project, I'm still in and would offer my help with executing the plan, if I may.

Also, the more I think about your suggestion of a CI run with an automatically patched typed version of the current codebase, the more I like it. That way, we might already get some benefits of adding return types without actually adding them. Can you that that up? I would offer to help fixing those tests.

ALL lines changed in this PR are individual BC breaks. That's way too abrupt. This can make v5 fail totally, as far as adoption is concerned.

Yes, you are right. I might have underestimated that danger.

derrabus · 2019-08-19T21:17:28Z

One result that I already have is a typed version of the EventDispatcher. I will post a PR later – even if we don't merge it, it might still be a good reference.

As promised: #33248

… (nicolas-grekas) This PR was merged into the 5.0-dev branch. Discussion ---------- [5.0] Add return types on internal|final|private methods | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #33228 #33236 | License | MIT | Doc PR | - I think some changes here can be backported to 4.4 Also, this fails with e.g. `Return value of Symfony\Component\Serializer\Normalizer\ArrayDenormalizer::denormalize() must be an object, array returned` Which means we need to update `DenormalizerInterface::denormalize()` with `@return object|array` in 3.4. Commits ------- 6f303b4 [5.0] Add return types on internal|final|private methods

@derrabus

…hods (nicolas-grekas, derrabus) This PR was merged into the 4.4 branch. Discussion ---------- Add return types to tests and final|internal|private methods | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #33228 #33236 | License | MIT | Doc PR | symfony/symfony-docs#...  #33266 made me spot an issue with my logic to add return types with help from `DebugClassLoader`. Here is a completing PR, with the logic expanded to test classes. I need help to fix tests, /cc @derrabus :) Commits ------- c39fd9c Fixed tests on the Security and Form components fc186bb Add return types to tests and final|internal|private methods

This PR was merged into the 3.4 branch. Discussion ---------- Use PHP 7.4 on deps=low | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Having a job on PHP 7.4 will allow using return type covariance as described in #33236 Commits ------- 21b8702 Use PHP 7.4 on deps=low

nicolas-grekas · 2019-08-25T20:03:50Z

PR now green on Travis, help wanted for fixing failures on appveyor.

…rabus) This PR was merged into the 3.4 branch. Discussion ---------- [3.4] Fix return types declarations | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This is the subset of #33236 that applies to branch 3.4. This PR ensures return types are correctly declared, and adjust some implementations that return incorrect types. Commits ------- 2ceb453 [SecurityBundle] fix return type declarations c1d7a88 [BrowserKit] fix return type declarations 07405e2 [PropertyInfo] fix return type declarations 5f3b4b6 [Bridge/Doctrine] fix return type declarations 8706f18 [Form] fix return type declarations a32a713 [Console] fix return type declarations 523e9b9 [Intl] fix return type declarations 73f504c [Templating] fix return type declarations 2b8ef1d [DomCrawler] fix return type declarations 2ea98bb [Validator] fix return type declarations 6af0c80 [Process] fix return type declarations 28646c7 [Workflow] fix return type declarations 5f9aaa7 [Cache] fix return type declarations 5072cfc [Serializer] fix return type declarations 70feaa4 [Translation] fix return type declarations ca1fad4 [DI] fix return type declarations 9c63be4 [Config] fix return type declarations 05fe553 [HttpKernel] Fix return type declarations e0d79f7 [Security] Fix return type declarations c1b7118 [Routing] Fix return type declarations ef5ead0 [HttpFoundation] fix return type declarations

…n type declarations (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [ErrorHandler] make DebugClassLoader able to add return type declarations | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #33236 | License | MIT | Doc PR | - This is the current state of the patching logic I'm using in `DebugClassLoader`. I run it after a `composer i -o` with the excluded classes emptied. It's not perfect, it requires manual changes/review, but it did the job so far. Patching is enabled with the `SYMFONY_PATCH_TYPE_DECLARATIONS` env var. The value of the env var is an URL-encoded array, with the following parameters as of now: - `force=0/1/docblock`: - `0` adds return types only to non-BC breaking places: internal/final/test methods; - `1` adds return types everywhere possible, potentially breaking BC with child classes that didn't declare the return types before; - `docblock` adds `@return` annotations to opt-out from deprecation notices that `DebugClassLoader` trigger otherwise - the annotation meaning: "I know a return type should be used here instead of an annotation, but I can't add it for BC reasons for now". - `php71-compat=0/1`: to prevent using the `object` type when the code must be compatible with PHP 7.1 Commits ------- 72f6a97 [ErrorHandler] make DebugClassLoader able to add return type declarations

…CI (nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- Add return-types with help from DebugClassLoader in the CI | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #33228 | License | MIT | Doc PR | - I've spent a great deal of time on this PR, experimenting with adding return types to the codebase. TL;DR: my conclusion is that we cannot make it for 5.0. There are two reasons for this: 1. The burden this will put on the community is immense, especially when considering that third party libs must also be updated for any apps to work at all on a return-typed 5.0. Symfony must add them last, not first. 2. We need return type covariance, yet this won't be available before PHP 7.4, while 5.0 supports 7.2. What's attached? - ~a draft patching logic in `DebugClassLoader` to add return-type where it discovers this should be done~ - return types added automatically thanks to #33283 - ~manual fixes for situations not handled (yet, if possible at all) by that logic in `DebugClassLoader`~ #33332 What's achieved? Tests are green \o/ At this stage, I think we have to acknowledge we won't add return-types in 5.0 but prepare a serious plan to add them in 6.0. This plan could be: - [x] make DebugClassLoader able to automate adding return types. - [x] in 4.4: add all possible return types that don't break BC, e.g. in `Tests` and in generated code - [x] spot and fix places where annotations aren't accurate, add more annotations where possible. - [x] ensure `DebugClassLoader` triggers the best possible deprecations that encourage ppl to add return-types in their libs/apps. This means we could decide to disable the current ones (see #33235) and to re-enable them in 5.1. This will also give us the time to fine-tune the tooling (item 1. on this list) Ideally, we could reach a point where we could test branch 4.4 *with* return-types: we'd use the tooling to add them automatically in the CI job, then we'd run tests and they should be green. Let's do this? Help Wanted, here is how: *With PHP 7.4*, run `php .github/patch-types.php`. This will add return types everywhere possible. Then run tests, e.g. `./phpunit src/Symfony/Component/HttpFoundation --exclude-group legacy,issue-32995` Here are the components that fail with return types added, please help me check them all with a PR on [my fork](https://github.com/nicolas-grekas/symfony/tree/eh-return-types): - [x] src/Symfony/Bridge/Doctrine - [x] src/Symfony/Bridge/Monolog - [x] src/Symfony/Bridge/PhpUnit - [x] src/Symfony/Bridge/ProxyManager - [x] src/Symfony/Bridge/Twig - [x] src/Symfony/Bundle/DebugBundle - [x] src/Symfony/Bundle/FrameworkBundle - [x] src/Symfony/Bundle/SecurityBundle - [x] src/Symfony/Bundle/TwigBundle - [x] src/Symfony/Bundle/WebProfilerBundle - [x] src/Symfony/Bundle/WebServerBundle - [x] src/Symfony/Component/Asset - [x] src/Symfony/Component/BrowserKit - [x] src/Symfony/Component/Cache - [x] nicolas-grekas#28 src/Symfony/Component/Config - [x] src/Symfony/Component/Console - [x] src/Symfony/Component/CssSelector - [x] src/Symfony/Component/Debug - [x] nicolas-grekas#28 src/Symfony/Component/DependencyInjection - [x] src/Symfony/Component/DomCrawler - [x] src/Symfony/Component/Dotenv - [x] src/Symfony/Component/ErrorHandler - [x] src/Symfony/Component/ErrorRenderer - [x] nicolas-grekas#24 src/Symfony/Component/EventDispatcher - [x] src/Symfony/Component/ExpressionLanguage - [x] src/Symfony/Component/Filesystem - [x] src/Symfony/Component/Finder - [x] src/Symfony/Component/Form - [x] src/Symfony/Component/HttpClient - [x] src/Symfony/Component/HttpFoundation - [x] src/Symfony/Component/HttpKernel - [x] src/Symfony/Component/Inflector - [x] src/Symfony/Component/Intl - [x] src/Symfony/Component/Ldap - [x] src/Symfony/Component/Lock - [x] src/Symfony/Component/Mailer - [x] src/Symfony/Component/Messenger - [x] src/Symfony/Component/Mime - [x] src/Symfony/Component/OptionsResolver - [x] src/Symfony/Component/Process - [x] src/Symfony/Component/PropertyAccess - [x] src/Symfony/Component/PropertyInfo - [x] nicolas-grekas#25 src/Symfony/Component/Routing - [x] nicolas-grekas#26 src/Symfony/Component/Security - [x] src/Symfony/Component/Security/Core - [x] src/Symfony/Component/Security/Guard - [x] src/Symfony/Component/Security/Http - [x] nicolas-grekas#29 src/Symfony/Component/Serializer - [x] src/Symfony/Component/Security/Csrf - [x] src/Symfony/Component/Stopwatch - [x] src/Symfony/Component/Templating - [x] nicolas-grekas#27 src/Symfony/Component/Translation - [x] src/Symfony/Component/Validator - [x] src/Symfony/Component/VarDumper - [x] src/Symfony/Component/VarExporter - [x] src/Symfony/Component/WebLink - [x] src/Symfony/Component/Workflow - [x] src/Symfony/Component/Yaml - [x] src/Symfony/Contracts Commits ------- 11149a1 Add return-types with help from DebugClassLoader in the CI

nicolas-grekas added this to the next milestone Aug 19, 2019

nicolas-grekas requested review from dunglas, lyrixx, sroze and xabbuh as code owners August 19, 2019 09:41

carsonbot added the Status: Needs Review label Aug 19, 2019

This was referenced Aug 19, 2019

[5.0] Add return type declarations #33228

Closed

Return type deprecations for library tests #33235

Closed

nicolas-grekas force-pushed the eh-return-types branch from f6ab891 to 70f6e37 Compare August 19, 2019 12:20

nicolas-grekas mentioned this pull request Aug 19, 2019

[DI] add return-types to generated containers #33242

Merged

derrabus mentioned this pull request Aug 19, 2019

[EventDispatcher] Add return type declarations #33248

Closed

This was referenced Aug 20, 2019

Add return types to internal|final|private methods #33255

Merged

[5.0] Add return types on internal|final|private methods #33259

Merged

nicolas-grekas mentioned this pull request Aug 21, 2019

Add return types to tests and final|internal|private methods #33279

Merged

nicolas-grekas mentioned this pull request Aug 21, 2019

[ErrorHandler] make DebugClassLoader able to add return type declarations #33283

Merged

nicolas-grekas force-pushed the eh-return-types branch from 70f6e37 to 8c8534e Compare August 21, 2019 16:42

nicolas-grekas mentioned this pull request Aug 21, 2019

Use PHP 7.4 on deps=low #33288

Merged

nicolas-grekas force-pushed the eh-return-types branch 2 times, most recently from 1c32a23 to d066b06 Compare August 22, 2019 10:43

nicolas-grekas mentioned this pull request Aug 22, 2019

Add return types to internal & magic methods when possible #33294

Merged

nicolas-grekas mentioned this pull request Aug 25, 2019

[3.4] Fix return types declarations #33332

Merged

nicolas-grekas force-pushed the eh-return-types branch 2 times, most recently from 6b73f04 to 557dfb1 Compare August 26, 2019 07:03

nicolas-grekas force-pushed the eh-return-types branch 2 times, most recently from 4589b55 to 670dd85 Compare August 26, 2019 09:44

nicolas-grekas changed the title ~~Add return-types with help from DebugClassLoader~~ Add return-types with help from DebugClassLoader in the CI Aug 26, 2019

nicolas-grekas force-pushed the eh-return-types branch from 670dd85 to 47b03af Compare August 26, 2019 09:54

nicolas-grekas force-pushed the eh-return-types branch 9 times, most recently from 1e1e0e7 to 528083d Compare August 26, 2019 13:06

Add return-types with help from DebugClassLoader in the CI

11149a1

nicolas-grekas force-pushed the eh-return-types branch from 528083d to 11149a1 Compare August 26, 2019 13:31

nicolas-grekas merged commit 11149a1 into symfony:4.4 Aug 26, 2019

nicolas-grekas deleted the eh-return-types branch September 1, 2019 12:27

aik099 mentioned this pull request Sep 6, 2019

Fixing deprecations minkphp/driver-testsuite#38

Closed

nicolas-grekas mentioned this pull request Sep 30, 2019

Command::execute() should always return int - deprecate returning null #33747

Closed

romaricdrigon mentioned this pull request Oct 16, 2019

Add return types to generated guard authenticators symfony/maker-bundle#459

Closed

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

lyrixx mentioned this pull request Jan 22, 2020

[Workflow] Added parameter type hinting where possible. #35434

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add return-types with help from DebugClassLoader in the CI #33236

Add return-types with help from DebugClassLoader in the CI #33236

nicolas-grekas commented Aug 19, 2019 •

edited

fabpot commented Aug 19, 2019

derrabus commented Aug 19, 2019 •

edited

nicolas-grekas commented Aug 19, 2019 •

edited

Tobion commented Aug 19, 2019

derrabus commented Aug 19, 2019

derrabus commented Aug 19, 2019

nicolas-grekas commented Aug 25, 2019

Add return-types with help from DebugClassLoader in the CI #33236

Add return-types with help from DebugClassLoader in the CI #33236

Conversation

nicolas-grekas commented Aug 19, 2019 • edited

fabpot commented Aug 19, 2019

derrabus commented Aug 19, 2019 • edited

nicolas-grekas commented Aug 19, 2019 • edited

Tobion commented Aug 19, 2019

derrabus commented Aug 19, 2019

derrabus commented Aug 19, 2019

nicolas-grekas commented Aug 25, 2019

nicolas-grekas commented Aug 19, 2019 •

edited

derrabus commented Aug 19, 2019 •

edited

nicolas-grekas commented Aug 19, 2019 •

edited