Skip to content

Improve some PHPdocs based on existing Symfony stubs in PHPstan and Psalm #47016

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 3 commits into from
Aug 2, 2022

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jul 21, 2022

Q A
Branch? 6.2
Bug fix? no
New feature? no
Deprecations? no (strictly spoken yes, as I guess DebugClassLoader will trigger deprecations for some now?)
Tickets Replaces #40783
License MIT
Doc PR symfony/symfony-docs#17064

Todo

  • Review the Psalm check of this PR
  • Test the changes with DebugClassLoader in a real app

Description

This PR adds some more information to the PHPdoc of Symfony classes. By moving the information from the current stubs of static analyzers into Symfony itself: (1) we can improve the experience for all users, (2) reduce false-positives from our own Psalm check and (3) make sure they are in sync with changes done on these classes.

To avoid a PR that is too big to review and maintain, the scope of this PR is deliberately kept narrow:

  • Only PHPdoc from either Psalm's Symfony stubs or PHPStan's Symfony stubs is added
  • The PHPdoc MUST NOT break PHPstorm
  • The PHPdoc MUST be supported by both PHPstan and Psalm (those are the only two static analyzers that currently ship an official Symfony plugin afaik)
  • The PHPdoc SHOULD NOT duplicate anything that can already be deduced from either PHP's type declarations or already existing PHPdoc.
  • The PHPdoc MUST document the contract and NOT be added to fit a 95% use-case or improve auto-completion.

EDIT: Replaced the guidelines by symfony/symfony-docs#17064

On top of this, to get this PR approved quicker, I've left out all stubs from the Form (based on the discussions in #40783) and most of PHPStan's Envelope stub (due to complexity of the PHPdoc).

@@ -33,7 +33,7 @@ interface VoterInterface
* @param mixed $subject The subject to secure
* @param array $attributes An array of attributes associated with the method being invoked
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on phpstan/phpstan-symfony#291, this may be changed to array<mixed>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I don't like PHPstan's requirement to make any array a generic (so requiring to specify at least one type). I think it's OK to use Psalm & PHPstorm's definition of array = array<array-key, mixed> in Symfony's code base and keep changes like this for a PHPstan stub.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are multiple places in the Symfony code where array is used and could be replaced by string[] or others more precise types. So currently, array just mean "We didn't precise, but maybe don't accept everything" and not "Every array<array-key, mixed> is allowed".

PHPStan strategy force to pass on every array to write instead, sometimes string[], sometime array<string, mixed> and sometimes array<mixed>. The advantage of array<mixed> (or mixed[] if preferred) is the fact than we do know that the function really accept everything, it's supposed to be checked when changing array into array<mixed> (or string[] or ...).

I may be wrong but I don't feel like changing array into array<mixed> on some places would require a lot of extra-maintenance to the symfony/symfony project when it would require to duplicate a lot of classes/interface on phpstan/phpstan-symfony just to solve this. So it would be a nice gesture to Symfony+PHPStan users to consider using array<mixed>.

@wouterj wouterj force-pushed the staticanalysis-stubs branch from b753827 to b6808dc Compare July 21, 2022 18:36
@stof
Copy link
Member

stof commented Jul 21, 2022

@wouterj you need to update the .github/expected-missing-return-types.diff file to account for the change in Command::getHelper()

@wouterj wouterj force-pushed the staticanalysis-stubs branch from b6808dc to a51431d Compare July 21, 2022 21:16
@wouterj wouterj requested a review from yceruto as a code owner July 21, 2022 21:16
@wouterj wouterj force-pushed the staticanalysis-stubs branch from a51431d to 8ccd900 Compare July 21, 2022 21:36
@wouterj
Copy link
Member Author

wouterj commented Jul 23, 2022

This one is ready to merge now imho

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About this line: https://github.com/symfony/symfony/pull/47016/files#diff-94e123982c1528593508077df90d9228847942a3b8dc077fd93560b09e5b196bR167

$args[\is_string($k) ? $this->resolveValue($k, $resolving) : $k] = $this->resolveValue($v, $resolving);

Depending on $resolving, this may access the array with a non scalar key. Should we modify the code and throw an exception or should we specify $resolving harder?

(I’m on mobile, sorry for the poor syntax)

@wouterj
Copy link
Member Author

wouterj commented Jul 23, 2022

Depending on $k, this may access the array with a non scalar key. Should we modify the code and throw an exception or should we specify $k harder?

This is a good catch by Psalm that is easy to produce in a real Symfony app:

parameters:
    some_list: [1, 2, 3]
    broken_parameter:
        '%some_list%': foo

Note that resolveString() already has a nice exception message if the array key would have been 'app.%some_list%'. I've added a check and exception for this newly discovered case.

@wouterj wouterj force-pushed the staticanalysis-stubs branch from 958ae00 to 1db9fb4 Compare July 26, 2022 13:31
nicolas-grekas added a commit that referenced this pull request Aug 1, 2022
…wouterj)

This PR was merged into the 6.2 branch.

Discussion
----------

[Config] Add conditional types to conditional builders

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #46546
| License       | MIT
| Doc PR        | n/a

Replaces #46581 , submitted to 6.2 instead. This matches with the changes proposed in #47016 and the config builder contained quite a big refactor for comments in 6.x so this avoids nasty conflicts.

Original description:

> One of the main benefits of using the new PHP config builders in application code is the ability to use PHP community tooling to autocomplete and validate the configuration. In this light, I think it makes sense to add the correct PHPdocs to make Psalm and PHPstan understand the config builders.
>
> On top of that, as these config classes are live generated, this is not something that can be easily stubbed in the special PHPstan and Psalm Symfony plugins.

Commits
-------

ce1087e [Config] Add conditional types to conditional builders
@nicolas-grekas
Copy link
Member

Thank you @wouterj.

@nicolas-grekas nicolas-grekas merged commit 1dedb22 into symfony:6.2 Aug 2, 2022
@nicolas-grekas
Copy link
Member

And thank you @mdeboer also!

@wouterj wouterj deleted the staticanalysis-stubs branch August 2, 2022 13:26
chalasr added a commit that referenced this pull request Nov 30, 2022
…Stan (ogizanagi)

This PR was merged into the 6.2 branch.

Discussion
----------

[Console] Fix `OutputInterface` options int-mask for PHPStan

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | N/A <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT
| Doc PR        | N/A

When upgrading an application to 6.2, I encountered this issue with PHPStan:

```shell
 ------ -------------------------------------------------------------------------------------------------------
  Line   Pilot/Runner/Output/MultiplexingOutput.php
 ------ -------------------------------------------------------------------------------------------------------
  79     Default value of the parameter #3 $options (0) of method
         App\Pilot\Runner\Output\MultiplexingOutput::write() is incompatible with type 1|2|4|16|32|64|128|256.
 ------ -------------------------------------------------------------------------------------------------------
```

The `MultiplexingOutput` implements the `OutputInterface` and defined `0` as the default value for `$options`:

```php
    public function write(string|iterable $messages, bool $newline = false, int $options = 0): void
```

as defined by the interface:

https://github.com/symfony/symfony/blob/69f46f231b16be1bce1e2ecfa7f9a1fb192cda22/src/Symfony/Component/Console/Output/OutputInterface.php#L40

When trying to use `self::OUTPUT_NORMAL | self::VERBOSITY_NORMAL` as default value:

```shell
 ------ -------------------------------------------------------------------------------------------------------
  Line   Pilot/Runner/Output/MultiplexingOutput.php
 ------ -------------------------------------------------------------------------------------------------------
  79     Default value of the parameter #3 $options (33) of method
         App\Pilot\Runner\Output\MultiplexingOutput::write() is incompatible with type 1|2|4|16|32|64|128|256.
 ------ -------------------------------------------------------------------------------------------------------
```

Or simply using `Output::write()` with specific options:

```shell
 ------ -------------------------------------------------------------------------------------------------------
  Line   Pilot/Runner/Output/MultiplexingOutput.php
 ------ -------------------------------------------------------------------------------------------------------
  81     Parameter #3 $options of method Symfony\Component\Console\Output\Output::write() expects
         1|2|4|16|32|64|128|256, 33 given.
 ------ -------------------------------------------------------------------------------------------------------
```

Using PHPStan's [`int-mask-of`](https://phpstan.org/writing-php-code/phpdoc-types#integer-masks) is the solution for this, and allows by nature the `0` value.
It was even used [at some point](#47016 (comment)), but reverted to use `self::VERBOSITY_*|self::OUTPUT_*`. However, it does not behave as expected for masks.

So, either we use `int-mask-of`, or we revert to `int`?

Commits
-------

304a17a [Console] Fix OutputInterface options int-mask for PhpStan
PhilETaylor pushed a commit to PhilETaylor/symfony that referenced this pull request Sep 6, 2023
…ubs in PHPstan and Psalm (mdeboer, wouterj)

This PR was merged into the 6.2 branch.

Discussion
----------

Improve some PHPdocs based on existing Symfony stubs in PHPstan and Psalm

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no (strictly spoken yes, as I guess DebugClassLoader will trigger deprecations for some now?)
| Tickets       | Replaces symfony#40783
| License       | MIT
| Doc PR        | symfony/symfony-docs#17064

Todo
---

* [x] Review the Psalm check of this PR
* [x] Test the changes with DebugClassLoader in a real app

Description
---

This PR adds some more information to the PHPdoc of Symfony classes. By moving the information from the current stubs of static analyzers into Symfony itself: (1) we can improve the experience for all users, (2) reduce false-positives from our own Psalm check and (3) make sure they are in sync with changes done on these classes.

<s>To avoid a PR that is too big to review and maintain, the scope of this PR is deliberately kept narrow:

* Only PHPdoc from either [Psalm's Symfony stubs](https://github.com/psalm/psalm-plugin-symfony/tree/master/src/Stubs) or [PHPStan's Symfony stubs](https://github.com/phpstan/phpstan-symfony/tree/1.2.x/stubs) is added
* The PHPdoc MUST NOT break PHPstorm
* The PHPdoc MUST be supported by both PHPstan and Psalm (those are the only two static analyzers that currently ship an official Symfony plugin afaik)
* The PHPdoc SHOULD NOT duplicate anything that can already be deduced from either PHP's type declarations or already existing PHPdoc.
* The PHPdoc MUST document the contract and NOT be added to fit a 95% use-case or improve auto-completion.</s>

EDIT: Replaced the guidelines by symfony/symfony-docs#17064

On top of this, to get this PR approved quicker, I've left out all stubs from the Form (based on the discussions in symfony#40783) and most of [PHPStan's `Envelope` stub](https://github.com/psalm/psalm-plugin-symfony/blob/master/src/Stubs/common/Component/Messenger/Envelope.stubphp) (due to complexity of the PHPdoc).

Commits
-------

f5a802a [DependencyInjection] Add nice exception when using non-scalar parameter as array key
fa7703b [ErrorHandler] Do not patch return types that are not a valid PHP type
38ab6de Improve some PHPdocs based on existing Symfony stubs in PHPstan and Psalm
nicolas-grekas added a commit that referenced this pull request Nov 26, 2024
…block (kbond)

This PR was merged into the 6.4 branch.

Discussion
----------

[Messenger] fix `Envelope::all()` conditional return docblock

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | n/a
| License       | MIT

I think this was a typo in #47016.

Commits
-------

e1a5beb [Messenger] fix `Envelope::all()` conditional return docblock
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.