Skip to content

[ErrorHandler] improve parsing of phpdoc by DebugClassLoader #42149

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

Merged
merged 1 commit into from
Aug 18, 2021

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jul 16, 2021

Q A
Branch? 5.4
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

The goal of this PR is to improve DebugClassLoader to make it able:

  • to parse union types,
  • to trigger a deprecation when an @return defines a narrower type than the native return type,
  • to patch those return types in "force" mode
  • populate INTERNAL_TYPES from 8.1's ReturnTypeWillChange

@nicolas-grekas nicolas-grekas force-pushed the eh-ret-types branch 2 times, most recently from 99a48fb to ad03e81 Compare July 20, 2021 13:46
nicolas-grekas added a commit that referenced this pull request Jul 21, 2021
…thods (nicolas-grekas)

This PR was merged into the 6.0 branch.

Discussion
----------

Add return type unions to private/internal/final/test methods

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Spotted thanks to progress made on #42149

Commits
-------

af7ff7e Add return type unions to private/internal/final/test methods
@nicolas-grekas nicolas-grekas changed the base branch from 6.0 to 5.4 July 22, 2021 12:38
@nicolas-grekas
Copy link
Member Author

PR updated. It now contains a script that parses the source code of PHP 8.1 to extract all new tentative return types. This is used to generate the new internal TentativeTypes class, which is in turn used to trigger deprecations when a method misses a return type or annotation.

As you can see, this makes tests fail because we're missing many such annotations.

In a next step, we should add the missing annotations and add also the missing #[\ReturnTypeWillChange] attributes. The code shouldn't be that far from being able to do it on its own.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jul 23, 2021

I'm going on a break for the following 2 weeks and will be mostly AFK.
The code in this PR is working, but needs more testing.

If you want to play locally with it, check it out, empty exclude-from-classmap in composer.json, then run composer i -o. The -o is important as that's how existing classes are discovered.

Then run SYMFONY_PATCH_TYPE_DECLARATIONS=force=1 php .github/patch-types.php to add return types, or SYMFONY_PATCH_TYPE_DECLARATIONS=force=docblock php .github/patch-types.php to add phpdoc.

What is missing here:

  • fix tests
  • make DebugClassLoader able to add #[ReturnTypeWillChange] attributes where needed for PHP 8.1
  • look at current failures in the CI and figure out which annotations/return types are missing. E.g. anonymous classes aren't parsed right now but would need to.

The plan I have in mind for Symfony 6 relies heavily on this work.

I want to help the community by providing in 5.4 a DebugClassLoader that they can turn on in their CI to spot where they're missing return-types (or @return annotations before breaking BC.)

The plan for the OSS community would then be: turn this on, fix deprecations, plan a major version bump if needed, then allow Symfony 6 in their composer.json file.

I fear that allowing Symfony 6.0 right now in OSS libraries might create a huge WTF moment for the community: the day we'll add the return types in our 6.0 branch, we'll potentially break all those libs that allowed ^6.

We might need a similar approach for public properties btw. Help wanted on all those topics.

nicolas-grekas added a commit that referenced this pull request Aug 10, 2021
…est methods (nicolas-grekas)

This PR was merged into the 6.0 branch.

Discussion
----------

Narrow existing return types on private/internal/final/test methods

| Q             | A
| ------------- | ---
| Branch?       | 6.0
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

More progress from #42149

Commits
-------

d67a927 Narrow existing return types on private/internal/final/test methods
@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Aug 10, 2021

@nicolas-grekas nicolas-grekas force-pushed the eh-ret-types branch 5 times, most recently from 3126d88 to e8a5d5b Compare August 11, 2021 13:01
@nicolas-grekas
Copy link
Member Author

PR is ready. It requires the last two PRs listed above to be green for tests. Psalm/fabbot reports are false positives.

nicolas-grekas added a commit that referenced this pull request Aug 11, 2021
This PR was merged into the 5.4 branch.

Discussion
----------

More return type fixes

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Extracted from #42149

Commits
-------

f7abdec More return type fixes
@nicolas-grekas nicolas-grekas modified the milestones: 6.0, 5.4 Aug 17, 2021
@nicolas-grekas nicolas-grekas force-pushed the eh-ret-types branch 3 times, most recently from f49d877 to eb72183 Compare August 17, 2021 16:37
@nicolas-grekas
Copy link
Member Author

This PR is ready, remaining failures are false positives.

nicolas-grekas added a commit that referenced this pull request Aug 18, 2021
… (nicolas-grekas)

This PR was merged into the 5.4 branch.

Discussion
----------

Add missing return types to tests/internal/final methods

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Thanks to #42149 again

Commits
-------

0f39a0f Add missing return types to tests/internal/final methods
@nicolas-grekas nicolas-grekas force-pushed the eh-ret-types branch 3 times, most recently from 9d8c5e8 to 11a81d3 Compare August 18, 2021 08:30
@nicolas-grekas nicolas-grekas merged commit 7347f98 into symfony:5.4 Aug 18, 2021
@nicolas-grekas nicolas-grekas deleted the eh-ret-types branch August 18, 2021 09:00
nicolas-grekas added a commit that referenced this pull request Aug 25, 2021
…cations by default + add mode to turn them into native types (nicolas-grekas)

This PR was merged into the 5.4 branch.

Discussion
----------

[ErrorHandler] Turn return-type annotations into deprecations by default + add mode to turn them into native types

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Leverages #42149

We need extensive doc on the topic for sure, a whole new chapter.

DebugClassLoader allows patching an app or a lib by going through these steps:

- require `symfony/error-handler` if not already there
- run `composer install -q --optimize-autoloader`
- copy/paste the below script to the root of the app/lib, in `patch-types.php`
- run the script with `SYMFONY_PATCH_TYPE_DECLARATIONS='force=phpdoc' php patch-types.php`

`SYMFONY_PATCH_TYPE_DECLARATIONS` can be set to:
- `'force=phpdoc'` to copy ``@return`` annotations from parent classes, to express that the next major version of that lib is going to add a native return types;
- `'force=1'` to turn ``@return`` annotations into native return types, but only on tests/private/final/internal methods;
- `'force=2'` to turn ``@return`` annotations into native return types, for all possible methods.

```php
<?php

if (false === getenv('SYMFONY_PATCH_TYPE_DECLARATIONS')) {
    echo "Please define the SYMFONY_PATCH_TYPE_DECLARATIONS env var when running this script.\n";
    exit(1);
}

$loader = require __DIR__.'/vendor/autoload.php';

Symfony\Component\ErrorHandler\DebugClassLoader::enable();

foreach ($loader->getClassMap() as $class => $file) {
    switch (true) {
        case false !== strpos($file = realpath($file), '/vendor/'):
        case false !== strpos($file, '/src/path-to-exclude/'): // add as many as required
            continue;
    }

    class_exists($class);
}

Symfony\Component\ErrorHandler\DebugClassLoader::checkClasses();
```

Commits
-------

bb11e62 [ErrorHandler] Turn return-type annotations into deprecations by default, add mode to turn them into native types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants