Sorry, but I don't think we want to merge this change. At least, not without having a discussion about using this new notation in Symfony (AFAIK, we have never used it anywhere else).

I agree with @fabpot that this is something we'll have to discuss (and I'm 100% in favor of this btw). The Psalm team is maintaining https://github.com/psalm/psalm-plugin-symfony/tree/master/src/Stubs to add stub-files with these annotations for precisely this usage (in e.g. property accessor). I think it's much better to move all of these into the Symfony code, but I'm against having some of this in the code and some of this in the stubs.

Generics in PHPDoc have broad tooling support. PhpStorm, PHPStan and Psalm support this notation and it improves the DX a lot if we used it. As @wouterj has pointed out, people already build packages to deliver type information that we don't provide. I think it's better if we maintain the type information directly in our PHPDoc blocks.

I'm ok to do this as it is modern phpdoc. Hope we can remove it in a future PHP version though.

I think it's worth mentioning that this makes the contract more precise, which should be covered by our BC promise:
This method does not just guarantee to always return a BadgeInterface instance anymore, but an instance of the concrete implementation that is requested via method argument.

I'm ok-ish only if this is something "standard" that is understood by many tools (for some definition of many).

I'm ok-ish only if this is something "standard" that is understood by many tools (for some definition of many).

Like @derrabus mentioned, this has broad support in tooling and is not just for PhpStorm and Psalm.

I am glad to see so many reactions and to be honest, I completely forgot about the stubs repository @wouterj mentioned. I agree with his opinion in that having code in both a third-party stubs repository and in the Symfony code is not ideal.

As this piece of code is marked experimental, I suppose we could make the first steps of merging these annotations from the stubs into the Symfony code where it makes sense.

Thanks all for taking the time! 👍🏻

I personally would be OK to import all annotations that exist on the stubs.
Of course, we should import only the annotations that are supported by many tools (eg not psalm-specific annotations.)

@mdeboer would you mind updating this PR to add them all?

I was in the process of opening a similar PR for forms when @nicolas-grekas commented: 5.x...dunglas:feat/form-psalm-template.

Having such annotations will allow to not introduce "useless" code just to have autocompletion working properly such as proposed in #40799 (comment).

+1 on my side to have these annotations directly in the code base. It will increase the DX of Symfony for both PHPStorm and Psalm/PHPStan users.

I will update the PR to add all the relevant stubs that are not just for psalm (e.g. without psalm prefix or supported by others with psalm prefix).

So I made some fixes, hopefully this ticks all the boxes. Unfortunately fabbot complains about a comment about the return value in Symfony\Component\Form\Extension\Core\DataTransformer::transform, wanting to replace /** @var string[] */ with for example /* @var string[] */. Would it help to make it a multi-line comment?

I haven't been able to set up PhpStorm for this project properly yet I reckon but I really haven't got much time at the moment to deep dive into getting all this set up properly.

Are you able to finish this PR? Thanks

@mdeboer There is another file for PostSubmit event: https://github.com/psalm/psalm-plugin-symfony/blob/master/src/Stubs/common/forms/PostSubmitEvent.stubphp

Are you able to finish this PR? Thanks

Sorry, really busy weeks. I might give this another go at the end of this week as things probably slow down.

@mdeboer There is another file for PostSubmit event: https://github.com/psalm/psalm-plugin-symfony/blob/master/src/Stubs/common/forms/PostSubmitEvent.stubphp

Okay, probably missed it. I will see if that was intentional or not and add it accordingly 👍🏻

I don't think that those generic annotations should be added in the form component. Most of my review comments about types being wrong relate to the fact that the Form component is very flexible, which makes it very hard to define type definitions for that (adding a data transformer changes the view data or the model data for that form type for instance)

@stof

I don't think that those generic annotations should be added in the form component.

If unused, they would still work as before (mixed). But adding them prevents lots of errors and @var annotations.

The only (somewhat) drawback is the need for this:

$form = $this->createForm(UserType::class);   //   must use 'empty_data' callable to populate

// submit, handle...
if ($user = $form->getData()) {    // must be this way because Form::getData() returns nullable T

    // here psalm **knows** it is instance of User
    $this->dispatch(
        new SomeUserMessage($user);
    );
}

Message:

class SomeUserMessage
{
    private string $id;

    public function __construct(User $user)
    {
        $this->id = $user->getId();
    }
}

@mdeboer the code base evolved a lot 😬 Can you please rebase?

@zmitic the issue is that you are not adding generic types only for the Form instance, but for many parts of the components, and those don't match the behavior of the component.

A Form instance has 3 types associated to it (the model data, the normalized data and the view data).
The various data transformers are about converting between those, but they are not DataTransformerInterface<TModel, TNormalized>. They are a chain of transformers being composed, when the input of one of them is the output of the previous one.
Properly defining generic types for the form building API is probably impossible, as the FormBuilder API is about mutating an existing object, and so adding transformers is changing the data types of that instance (which is something that type systems cannot represent AFAIK), and event listeners added on it are expected to use the final data types anyway (even if they are configured before the transformers).

Having wrong generic types will be worse for static analyzers than not having them at all.

/cc @derrabus

Thing is I have very, very limited time and in regard of the Form stuff, I never ever touched it so I really have no clue with what is the correct way to proceed there.

This only really started as I noticed that the PassportInterface was lacking docblocks with generics. Though I'd love to be able to help, I simply cannot afford to put work aside and focus on this, specially with the holidays and the on-going crisis. Trying hard to not end up in a burn-out.

If anyone would like repo access, please let me know and I'll grant you access. Also feel free to submit pull requests, I'd be happy to merge them so they end up in this PR.

Take care all and happy holidays!

@mdeboer No worries. I intend to take over this PR for 6.1 because I am probably the one who caused most of the conflicts. 🙂

@derrabus Friendly ping here :)

Hi @mdeboer! I tried to reignite this pull request, but unfortunately there are just too many conflicts with version 6.2 (given we've added lots of native types and PHPdoc for iterator generics in 5.4/6.0, most of the changes in this PR conflict).

So I've decided that it is best to start all over in a new PR: #47016 I've kept your original commit (adding a template to passport) and the discussions in this PR were very useful in order to judge which changes are "quick wins" and which need a more thorough review (the new PR only contains the quick wins).

I hope you can understand and hopefully we'll see more contributions from you in the future. We've now learned the lesson (once again) that big PRs are very hard to merge... :(

No worries @wouterj I completely understand! I am glad it was of use 😁 I appreciate your follow up.