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

Enums: new function enum_exists #6431

Closed
Tracked by #6425
weirdan opened this issue Sep 5, 2021 · 10 comments · Fixed by #7404
Closed
Tracked by #6425

Enums: new function enum_exists #6431

weirdan opened this issue Sep 5, 2021 · 10 comments · Fixed by #7404

Comments

@weirdan
Copy link
Collaborator

weirdan commented Sep 5, 2021

psalm.dev/r/9b5935609d

Guide to editing callmaps: Editing callmaps.

@weirdan weirdan mentioned this issue Sep 5, 2021
15 tasks
@weirdan weirdan changed the title New function enum_exists: psalm.dev/r/9b5935609d New function enum_exists Sep 5, 2021
@weirdan
Copy link
Collaborator Author

weirdan commented Sep 5, 2021

psalm.dev/r/9b5935609d

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/9b5935609d
<?php

enum Status: int
{
    case FOO;
    case BAR = 42;
}

enum_exists(Status::FOO); //returns true
enum_exists(Status::BAZ); //returns false
Psalm output (using commit 916b098):

ERROR: ParseError - 3:6 - Syntax error, unexpected T_STRING on line 3

ERROR: ParseError - 5:5 - Syntax error, unexpected T_CASE on line 5

@weirdan weirdan added this to the PHP 8.1 milestone Sep 5, 2021
@weirdan weirdan changed the title New function enum_exists Enums: new function enum_exists Sep 5, 2021
@weirdan weirdan changed the title Enums: new function enum_exists Enums: new function enum_exists Sep 5, 2021
@brayniverse
Copy link

I should be able to do this one as well if you want @weirdan?

@weirdan
Copy link
Collaborator Author

weirdan commented Sep 7, 2021

Sure. Although this one will need some more work, as it's similar to class_exist() for which Psalm has special treatment here and there:

$ rg "'class_exists'" src
src/Psalm/Internal/Codebase/Functions.php
452:            'class_exists', // impure by virtue of triggering autoloader

src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php
1772:            && strtolower($stmt->name->parts[0]) === 'class_exists'

src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php
81:        if ($function_id === 'class_exists') {

src/Psalm/Internal/PhpVisitor/Reflector/ExpressionResolver.php
327:        if ($function->name->parts === ['class_exists']

src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentsAnalyzer.php
169:            if ($method_id === 'class_exists'

@orklah
Copy link
Collaborator

orklah commented Sep 7, 2021

If you're stuck, just ping us, we'll help :)

@brayniverse
Copy link

Interesting, I'll give it a crack tonight and let you know if I get stuck.

@brayniverse
Copy link

I had a first attempt last night but got a little lost. I'll try again tonigh and tell you where I'm getting lost if I don't make any progress.

@brayniverse
Copy link

Hey, sorry I've gone quiet. I haven't had enough time to get into this. Please let anyone else pick this up if they want to do it. Whenever I do have time I'll pick something else up.

@DannyvdSluijs
Copy link

DannyvdSluijs commented Oct 20, 2021

I took a stab at this one and was able create some additional test cases in tests/FunctionCallTest.php based on the class_exists ones that are already there based on the red-green principle. The test fail, showing the lack of support for enum_exists. So my journey continued.

I started to fill in the gaps you pointed out.

  • Related to the AssertionFinder I added the hasEnumExistsCheck function similar to the hasClassExistsCheck and hasTraitExistsCheck.
  • But when needing to implement the function around https://github.com/DannyvdSluijs/psalm/blob/b664850cdc15cefd8f0022fbf97e59529175934b/src/Psalm/Internal/Analyzer/Statements/Expression/AssertionFinder.php#L765 I got suck in deciding what to do. Basically enums are a valid class, calling class_exists on a Enum class returns true but the other way around isn't true. So you typically would like to annotate it as enum-string
  • When I arrived at src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentsAnalyzer.php it became hazy and I was uncertain at this point if I was following down the correct path. Some inputs on what is needed would be welcomed. Should I expand the context to know if it is inside an enum_exists call or should we alias it to the class_exists behaviour that already is there?

Also from the documentation I noticed the Callmap and Callmap_81_delta should be adjusted which was a walk in the park, assuming it is nothing more that making sure the signature is available in the callmap. As that is a straight up copy from class_exists it was easy.

You can see my fork at https://github.com/DannyvdSluijs/psalm/tree/6431-new-function--enum_exists if you're interested and it would help. The test are still failing.

@orklah
Copy link
Collaborator

orklah commented Oct 20, 2021

I got suck in deciding what to do. Basically enums are a valid class, calling class_exists on a Enum class returns true but the other way around isn't true. So you typically would like to annotate it as enum-string

Your call. You could probably go the way of interface-string by creating an enum-string which would be an alias of class-string. You could also go the way of trait-string who is a fully fledged type TTraitString. I would probably tend towards the second one myself, this will probably more flexible to handle future specific behaviors for enums

it became hazy and I was uncertain at this point if I was following down the correct path

Please try without adding anything here. I'm not actually sure why we have specific handling here, we'll see if we can make the tests pass without it

Seems like a good start :)

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

Successfully merging a pull request may close this issue.

4 participants