Skip to content
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

Make templating Psalm-specific #8769

Merged
merged 1 commit into from
Aug 4, 2021

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Jun 15, 2021

If $className is a class-string, then we return an instance of that
class. However, if it's an alias, things get complicated, and it's hard
to tell. There does not seem to be a way to express that for now.

We should probably consider dropping namespace aliases entirely and add
support for ::class inside DQL if it's not the case already.

Refs #8767 , phpstan/phpstan#5175 , cc @VincentLanglet

@greg0ire greg0ire marked this pull request as ready for review June 15, 2021 11:46
@greg0ire greg0ire added the Bug label Jun 15, 2021
@VincentLanglet
Copy link
Contributor

Not sure it's the right decision since it works fine for psalm. https://psalm.dev/r/738daf594d
IMHO it should be a feature request for phpstan phpstan/phpstan#5175 (comment)

The only other way to solve this is to drop the string support and only allowing class-string.
As a first step, just removing the string part will solve the issue (and could be the beginning of non passing a class string).

@greg0ire
Copy link
Member Author

Maybe yeah. This means the solution would be to use a non-templated version for phpstan with phpstan- annotations, and keep the psalm- annotations as they are I suppose.

@greg0ire greg0ire force-pushed the remove-unhelpful-template branch 2 times, most recently from 9b7d61d to b5b1a70 Compare June 15, 2021 17:58
@greg0ire
Copy link
Member Author

I pushed a commit that does that, but that means we now have a different behavior for Psalm and for Phpstan, and it's going to be hard to satisfy both of them now. To illustrate this, I have added a test within the static analysis directory you introduced.

@@ -321,11 +321,13 @@ public function hasFilters();
* {@inheritDoc}
*
* @psalm-param string|class-string<T> $className
* @phpstan-param string|class-string<T> $className
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @phpstan-param string|class-string<T> $className
* @phpstan-param string $className

@VincentLanglet
Copy link
Contributor

VincentLanglet commented Jun 15, 2021

I pushed a commit that does that, but that means we now have a different behavior for Psalm and for Phpstan, and it's going to be hard to satisfy both of them now. To illustrate this, I have added a test within the static analysis directory you introduced.

Yes, it reflects more the reality of the different behavior of those libraries, but I clearly don't like it.
It would be nice to have the annotation string|class-string<T> understood by phpstan.

If the feature-request is accepted by phpstan, maybe this code should be untouched.
As soon as the feature will be added to phpstan it string|class-string<T> will be understood instead of string.

Phpstan requires that the template can be resolved in all cases, while
Psalm seems to tolerate this ambiguous situation.
See phpstan/phpstan#5175 (comment)
@greg0ire greg0ire changed the title Remove template from method signature Make templating Psalm-specific Jun 17, 2021
@greg0ire
Copy link
Member Author

greg0ire commented Jul 4, 2021

We should probably consider dropping namespace aliases entirely

Related: #6935

@VincentLanglet
Copy link
Contributor

We should probably consider dropping namespace aliases entirely

Related: #6935

Deprecation should be introduced to the 2.x branch then ?
And maybe the phpdoc could be updated to drop the string part in order to promote the changes by user.

@greg0ire
Copy link
Member Author

greg0ire commented Jul 4, 2021

Yes, we just created #8818 in case you want to contribute it.
Regarding the phpdoc, I don't think the user is the only thing to take into account: Psalm and Phpstan are going to make this an error when it should just be a deprecation.

@VincentLanglet
Copy link
Contributor

Regarding the phpdoc, I don't think the user is the only thing to take into account: Psalm and Phpstan are going to make this an error when it should just be a deprecation.

That's what I had in mind.
The Psalm and PHPstan error will help the developper to update the code where string could have been passed.
To me, this is really similar to deprecation ; when you're using phpunit at the stricter level, deprecation is making the build to fail.

If someone is using phpunit at a strict level, he'll have to add group legacy or fix the code now.
If someone is using phpstan/psalm at a strict enough level, he'll have to ignore the error or fix the code now.

And one of the benefit is that this will fix the following issue phpstan/phpstan#5175 for all the 2.x user, without having to wait for the 3.0 for this.

@greg0ire
Copy link
Member Author

greg0ire commented Jul 4, 2021

I think you might be right, we'll have to see how it looks in practice, IMO it might result in a lot of ignore rules to add for us, as some checks are going to appear redundant, like checking if the string contains a colon.

@VincentLanglet
Copy link
Contributor

Should we close this in favor of #8818 @greg0ire ?

@greg0ire
Copy link
Member Author

greg0ire commented Aug 1, 2021

I don't know… #8818 is about deprecating namespace aliases, but it does not remove them, so the issue will remain until 3.0 😬

@VincentLanglet
Copy link
Contributor

I don't know… #8818 is about deprecating namespace aliases, but it does not remove them, so the issue will remain until 3.0 😬

I see three "solutions" then.

  • Merging a PR like yours, phpstan and psalm will have a different behavior.
  • Doing nothing, I think it will end with the same behavior than the first solution, since phpstan is already transforming string|class-string<T> to string ; but it might be more confusing for developpers.
  • Changing string|class-string<T> to class-string<T>. This will fix the phpstan issue, but you'll end up with new psalm issues in the doctrine code (see Fix annotation for phpstan #8869) because you're sometimes passing string to the method. This can be quickly fixed by adding them to the baseline. Static analysis error will also encourage developer to stop passing string and pass class-string instead, so it'll help the deprecation. But this might have some sens to do this in the same time than adding the deprecation.

Since the deprecation is planned #8818 / #8820, I would say the option 3 seems the more natural and phpdoc can be pre-emptively fixed in 2.10 version.

@greg0ire
Copy link
Member Author

greg0ire commented Aug 2, 2021

Thanks for laying out the 3 options again @VincentLanglet ! As you know, I'm not too fond of the 3rd option, since it will express that namespace aliases are no longer supported (and break some pipelines), when they will in fact still be supported. Not everybody treat deprecations as a failure in their CI.

@orklah hi! Do you have a recommendation on this one?

@orklah
Copy link
Contributor

orklah commented Aug 2, 2021

I agree that since namespace aliases are still supported, we shouldn't remove string from phpdoc (Psalm will probably complain too if there's code for handling non-class-string where the phpdoc says it can't happen)

I also understand the behavior in PHPStan and I wouldn't see that as a bug in the tool. string|class-string is indeed a string the same way that false|bool is a bool.
Unfortunately, once this has been said, we're left with a complicated situation because psalm and phpstan will both try to read annotations that were made for the other tool (for compatibility) and that force us to have both @psalm-param and @phpstan-param in order to satisfy everyone.

However, Doctrine is widely used and we're fortunate to have plugins custom made for it to handle the quirks that can't be handled natively with annotations.

So my advice would be to use @psalm annotations to describe the ideal behavior, use @phpstan annotations that doesn't use templates, ignore the issues that may arise in phpstan and ping the maintainers of the phpstan doctrine plugin that a special handling is needed here. When namespace aliases will be removed, this will be easy to go back to a single annotation for both tools and the case will be removed in the plugin

@greg0ire
Copy link
Member Author

greg0ire commented Aug 2, 2021

Sounds good to me! Does that sound OK to you @VincentLanglet ?

@VincentLanglet
Copy link
Contributor

Sounds good to me! Does that sound OK to you @VincentLanglet ?

Sure

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

There was so much thought put into this, that I'm not sure if I missed something important. 😅
But it will become easier once the aliases are gone.

@greg0ire greg0ire merged commit 055b646 into doctrine:2.9.x Aug 4, 2021
@greg0ire greg0ire deleted the remove-unhelpful-template branch August 4, 2021 05:42
@greg0ire greg0ire added this to the 2.9.4 milestone Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants