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

Add backed enum type annotation #10907

Open
wants to merge 14 commits into
base: 5.x
Choose a base branch
from

Conversation

michaelcozzolino
Copy link

@michaelcozzolino michaelcozzolino commented Apr 14, 2024

This fixes #8268.

Basically I added the /** @template T of int|string */ in the BackedEnum stub and then i managed to add issues when
the implemented template type mismatches with the backing type.

                    // will give an error
                    /**
                     * @implements BackedEnum<string>
                     */
                    enum NumberEnum: int {
                        case Zero = 0;
                        case One = 1;
                    }
                    // won't give an error
                    /**
                     * @implements BackedEnum<non-negative-int>
                     */
                    enum NumberEnum: int {
                        case Zero = 0;
                        case One = 1;
                    }

Furhtermore, it's not mandatory to add the@implements annotation if the backing type is just int or string

                    // won't give an error
                    /**
                     * @implements BackedEnum<int>
                     */
                    enum NumberEnum: int {
                        case Zero = 0;
                        case One = 1;
                    }
                    // won't give an error
                    enum NumberEnum: int {
                        case Zero = 0;
                        case One = 1;
                    }

Further cases can be seen in the tests.
I am not sure this is the right implementation. Let me know if, in case, we can make some adjustments.

@michaelcozzolino michaelcozzolino marked this pull request as ready for review April 14, 2024 08:47
@michaelcozzolino
Copy link
Author

@orklah @danog @weirdan please review!

@staabm
Copy link
Contributor

staabm commented Apr 16, 2024

I wonder why this phpdoc typing is necessary.

All enum values are known at static analysis time, so why doesn't psalm just turn the ->values type into a union of all case-types?

@michaelcozzolino
Copy link
Author

I wonder why this phpdoc typing is necessary.

All enum values are known at static analysis time, so why doesn't psalm just turn the ->values type into a union of all case-types?

and how will you know the type of the enum you want to infer? let's say you want an enum having only non-empty-string. how would you know this?

@staabm
Copy link
Contributor

staabm commented Apr 16, 2024

so you want a type which accepts any enum which is backed by a string? I can't think of a use-case for such a vague type. couldn't you accept any literal-string then? whats special about a string when it comes from a enum case?

@michaelcozzolino
Copy link
Author

so you want a type which accepts any enum which is backed by a string?

well, when you do enum MyEnum: string{...} aren't your already doing it? i just want to be strict about the types to pass in an enum, i would like to have enum having non empty strings as i would have classes with properties that are non empty string

@staabm
Copy link
Contributor

staabm commented Apr 16, 2024

you can't pass a type into a enum. every enum has a native type per default and cannot be defined in a wrong way without a fatal runtime error, e.g. https://3v4l.org/VlZAK

I don't see which information psalm could take from the phpdoc which is not already there by the raw definition.

don't get me wrong: I am not a psalm developer, so I just share my thoughts.
lets see what psalm guys say about said.

@michaelcozzolino
Copy link
Author

from the phpdoc which is not already there by th

Checking that the backed type is the same as the case value is not the main goal of this pr even if it's a subcase of it. Actually the main goal of the pr is to say that you want to infer a subtype of the backed type in order to have an enum whose case values respect both types.
So in case of

                     * @implements BackedEnum<non-negative-int>
                     */
                    enum NumberEnum: int {
                        case Zero = 0;
                        case One = 1;
                       case MinusOne = -1
                    }

you would get an error.
your example, as i already said is a subcase of this pr, but it does not mean that it should not be considered. At the end psalm will still give you errors for lines of code that would probably generate a fatal error, such as usages of non existing methods.

@orklah
Copy link
Collaborator

orklah commented Apr 28, 2024

My main issue here is that it will make any BackedEnum mandatory to be templated. If PHPStan doesn't, it will lead to complains about not being able to use both. It could be great to discuss it with them to know if it can be implemented on their side (and if they agree with the feature)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add way to annotate BackedEnum backing type
3 participants