BC issue, help needed

[zmitic]

I updated psalm (dev-master) today and suddenly got lots of new errors, all pointing to same problem.

---

Consider stub file like this one:

Before today, if users did not use template, psalm would not complain and assume T: mixed. Which is totally fine, not all forms need correct type due to their dynamic nature.

For example:

class MyType extends AbstractType {}

would work exactly as if I have written:

/** @extends AbstractType<mixed> */
class MyType extends AbstractType {}

With latest update, psalm requires this annotation and I can't find the issue that changed this behavior. The result is that all users of symfony/psalm and other stubs will suddenly get lots of errors.

---

Any suggestions how to mitigate the problem expect for ignoring MissingTemplateParam issue?

[psalm-github-bot]

Hey @zmitic, can you reproduce the issue on https://psalm.dev ?

[weirdan]

Refs #7492

[orklah]

@zmitic thanks for the feedback!

We're in no particular rush for this one, Psalm 5 hasn't been released (even as alpha) yet, so there is still time to refine features. Glad we have people testing with master for that :)

The PR #7492 was made to be a feature ensuring people don't forget annotating their templates. It's supposed to mimick what PHPStan already does for quite some time (even on array).

You find the change too big? Do you have an idea of the number of errors in a typical symfony project?

@danog, do you have ideas about what we could do to reduce the impact on end projects?

[danog]

@orklah I don't think there's any way to make this rule softer, apart from removing it altogether: this can indeed be considered a BC-breaking feature, and that's why it will be released in 5.0.

The raison d'être of this check is precisely to make sure eventual template types aren't forgotten about when extending a class, which is especially useful in the case of iterators.

It can also direct Psalm beginners in the right direction by introducing them to template types, directly indicating how they can help in iterators and other places.

[zmitic]

Glad we have people testing with master for that :)

I am into extreme sports; skydiving, paintball, late Friday deploys and using unreleased products 😄

Do you have an idea of the number of errors in a typical symfony project?

I don't have any errors, and my psalm setup is level 1. For example: when I noticed this change today, it took me < 30 mins to fix my forms, and make PR to remove some useless code in symfony plugin.

But I can't talk about other users.

You find the change too big?

Actually I don't, and I like this decision a lot.

But these stubs I made have not been verified by core members; forms can be very dynamic in nature, especially with DataTransformers.

So while I don't have a single issue, I am afraid lots of others might. It is not just for form stubs, but other stubbed packages as well.

---

Suggestion for Psalm5;
treat MissingTemplateParam as info by default, even on level 1? That would give users time to upgrade their code.

[danog]

Suggestion for Psalm5;
treat MissingTemplateParam as info by default, even on level 1? That would give users time to upgrade their code.

I'm happy that you agree with the design decision :D
However, I really don't think this preventive measure is needed: users will gradually upgrade their code as they gradually upgrade to Psalm v5, that's the point of a major release: make breaking changes, and stop maintaining older versions so people will gradually adapt their codebases as they upgrade to the new major.

[zmitic]

However, I really don't think this preventive measure is needed: users will gradually upgrade their code as they gradually upgrade to Psalm v5,

You are right, it does make more sense.

So different suggestion:

• how about MissingTemplateParam trigger info in 4.x branch by default, if that template comes from stub file?

[danog]

how about MissingTemplateParam trigger info in 4.x branch by default, if that template comes from stub file?

That sounds fun!
If @orklah / @weirdan are OK with it, I could send in a PR for that :D

[orklah]

how about MissingTemplateParam trigger info in 4.x branch by default, if that template comes from stub file?

That sounds fun! If @orklah / @weirdan are OK with it, I could send in a PR for that :D

You want to check the user_defined property for that?

I'm afraid we just saw it was not reliable for now (we'll have to revert/disable your change on ReturnTypeWillChange because of that...)

[danog]

I'm afraid we just saw it was not reliable for now (we'll have to revert/disable your change on ReturnTypeWillChange because of that...)

I saw, I tried playing around a bit, replacing the user_defined check with

        $storage->user_defined = true;
        if (class_exists($fq_classlike_name)) {
            $reflect = new ReflectionClass($fq_classlike_name);
            $storage->user_defined = !$reflect->isInternal();
        }

However I haven't yet managed to reproduce the original issue in the stub tests, will probably give it another go tomorrow.

[kissifrot]

Just my 2 cents, migrating to psalm v5 added the MissingTemplateParam error for all symfony form types we implemented 😅

[danog]

Yep, this is intended!

[rogierknoester]

Hi @danog, is this BC also intended for a case where a parent::__construct provides the type, like for Doctrine repositories?

https://psalm.dev/r/3672bc9542

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/3672bc9542

<?php

/**
 * @template T of object
 */
class ServiceEntityRepository
{
    /**
     * @psalm-param class-string<T> $entityClass
     */
    public function __construct(string $entityClass)
    {
    }
}

class FooRepository extends ServiceEntityRepository {
    
    public function __construct() {
        parent::__construct(Foo::class);
    }
    
}

class Foo {
}

Psalm output (using commit f723e08):

ERROR: MissingTemplateParam - 16:7 - FooRepository has missing template params when extending ServiceEntityRepository, expecting 1