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.

Sign up for GitHub

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

Update getReference phpdoc #482

Open
wants to merge 2 commits into
base: 1.7.x
Choose a base branch
from
Open

Update getReference phpdoc #482

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9d4898
Update getReference phpdoc
Florian-B Jul 17, 2024
b12d6cb
fix: incorrect order of annotations groups
Florian-B Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/ReferenceRepository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,12 @@ public function addReference($name, $object)
}

/**
* Loads an object using stored reference
* named by $name
* Loads an object using stored reference named by $name
*
* @param string $name
* @psalm-param class-string<T>|null $class
*
* @return object
* @return T|object
* @psalm-return ($class is null ? object : T)
Copy link
Member

Choose a reason for hiding this comment

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

Is psalm-return taken into account by PHPStorm? I'm asking because I don't think having just the information that this method returns T or any object is good enough to propose auto-completion.

Copy link
Author

Choose a reason for hiding this comment

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

No, PhpStorm does not take the psalm-return annotation into account.

However, I can confirm that adding T|object in the @return annotation enables auto-completion.
It must certainly interprets the psalm-param annotation, which indicates that T is an instance of a specific object.

Additionally, I think we can migrate the @psalm-param class-string<T>|null $class annotation to the native @param class-string<T>|null $class annotation. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so let's say T in that instance is … YourClass. Why does PHPStorm enable auto-completion on YourClass|object? It is equivalent to just object, isn't it?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I fully understand your last message.

If we just put @return object, it refers to the primitive type Object, which can correspond to any object.

Whereas T refers to the type that is created by the "generics" brought with the @template, indicating that T is an instance of $class, so the correct return type is T.

However, we have to keep T|object because the parameter $class is not required at the moment.

Copy link
Member

@greg0ire greg0ire Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I know what T refers to, I know what object refers to as well, but it seems, you and Phpstorm disagree with me and Psalm/PHPStan as to what T|object means. I don't think it means "instance of T and instance of object". To me, that's T&object, which… is just T. Likewise, T|object can IMO be simplified to just object.

Copy link
Author

@Florian-B Florian-B Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't claim to be an expert on this, but it fix the auto-completion in PHPStorm (I don't have colleagues using VSCode or other popular code editor to test this fix).

Here are some resources:

We have a very large project (around 6000 getReferences to migrate) and I think it's a shame to have to add the $class parameter (which, by the way, is a very good idea) without having a type-hint in return.

IMO when $class becomes required, the return type would be @return T.

Copy link
Member

@greg0ire greg0ire Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not an expert on PHPStorm either, I don't use it, that's why I'm asking why it behaves like that, I don't know. I recall in the distant past, Collection|User[] was used at some point to mean Collection<User> because IDEs only understood | and not &, but would expect this to be fixed by now. So IMO you should file 2 PHPStorm bugs:

  1. It should no longer interpret | as &
  2. It should add support for @psalm-return ($class is null ? object : T)

BTW, maybe it already understands that syntax, but ignores it because it's @psalm-return and not return? Can you check?

Copy link
Author

Choose a reason for hiding this comment

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

I've just tried several combinations (with only @return or only @psalm-return) but nothing changes 😞

However @return T&object works very well.

Here's a new proposal for phpdoc :

    /**
     * Loads an object using stored reference named by $name
     *
     * @param class-string<T>|null $class
     *
     * @return T&object
     * @psalm-return ($class is null ? object : T)
     *
     * @throws OutOfBoundsException - if repository does not exist.
     *
     * @template T of object
     */

Note that the @psalm-param has been changed to @param because I don't think it's necessarily specific to psalm.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think T&object is completely wrong. For instance, when $class is null, there is no guarantee to get T.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I completely agree with you ! T&object is wrong when $class is null.

I took some time to test and I realize that PHPStorm seems to have trouble interpreting the conditional return of the psalm-return annotation, especially with generics (if I replace T with bool in the psalm-return, it understands bool|object).

I'll open an issue with PHPStorm after my vacation.

Do you think @return T|object is acceptable or not ? This annotation is not entirely true because it lacks precision, but it is not wrong because the method return either an instance of T or an object for which we cannot determine the type when the $class parameter is not provided.

*
* @throws OutOfBoundsException - if repository does not exist.
Expand Down