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

Added "Invokable Cases" PHPStan extension #13

Merged
merged 2 commits into from
Aug 24, 2022

Conversation

samlev
Copy link
Contributor

@samlev samlev commented Jul 14, 2022

This includes a PHPStan extension to add support for InvokableCases so that static analysis tools can understand the callable methods and their return types.

The extension has been added in a way that will allow multiple extensions in the future if required, with a single include file that will import all extensions, or the option to include only specific extensions.

This includes a PHPStan extension to add support for `InvokableCases`
so that static analysis tools can understand the callable methods and
their return types.

The extension has been added in a way that will allow multiple extensions
in the future if required, with a single include file that will import
all extensions, or the option to include only specific extensions.
@stancl
Copy link
Member

stancl commented Jul 21, 2022

Hey, sorry for the late reply.

Could you just quickly explain what issues phpstan has without this extension? And how this solves them (e.g. what specific definitions this provides).

@samlev
Copy link
Contributor Author

samlev commented Jul 21, 2022

When you use InvokableCases normally, PHPstan complains that the static methods don't exist, and it cannot discover the types.

<?php
declare(strict_types = 1);

use ArchTech\Enums\InvokableCases;

enum Suits: string
{
    use InvokableCases;

    case CLUBS = 'C';
    case DIAMONDS = 'D';
    case HEARTS = 'H';
    case SPADES = 'S';
}

enum TaskStatus: int
{
    use InvokableCases;

    case INCOMPLETE = 0;
    case COMPLETED = 1;
    case CANCELLED = 2;
}

enum Role
{
    use InvokableCases;

    case ADMINISTRATOR;
    case SUBSCRIBER;
    case GUEST;
}

function foo(string $in): void
{
    echo $in;
}

function bar(int $in): void
{
    echo $in;
} 

foo(Suits::CLUBS());
foo(TaskStatus::CANCELLED());
foo(Role::GUEST());

bar(Suits::CLUBS());
bar(TaskStatus::CANCELLED());
bar(Role::GUEST());

When tested...

$ vendor/bin/phpstan analyse -l9 test.php
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ -------------------------------------------------------------
  Line   test.php
 ------ -------------------------------------------------------------
  44     Call to an undefined static method Suits::CLUBS().
  45     Call to an undefined static method TaskStatus::CANCELLED().
  46     Call to an undefined static method Role::GUEST().
  49     Call to an undefined static method Suits::CLUBS().
  50     Call to an undefined static method TaskStatus::CANCELLED().
  51     Call to an undefined static method Role::GUEST().
------ -------------------------------------------------------------              

 [ERROR] Found 6 errors 

But when you run the code:

$ php test.php
CPHP Fatal error:
Uncaught TypeError: foo(): Argument #1 ($in) must be of type string, int given, called in test.php on line 45 and defined in test.php:34
Stack trace:
#0 test.php(45): foo()
#1 {main}
  thrown in test.php on line 34

This PR adds an extension that tells PHPStan that when the InvokableCases trait is included, that there are static methods available for each case, and the appropriate return type:

$ vendor/bin/phpstan analyse -l9 test.php
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
------ -------------------------------------------------------------
 Line   test.php
------ -------------------------------------------------------------
  45     Parameter #1 $in of function foo expects string, int given.
  48     Parameter #1 $in of function bar expects int, string given.
  49     Parameter #1 $in of function bar expects int, string given
------ -------------------------------------------------------------
[ERROR] Found 3 errors

@samlev
Copy link
Contributor Author

samlev commented Jul 21, 2022

Actually... I should probably extend the type hinting to the __invoke() method, too.

I'll update it tomorrow.

@samlev
Copy link
Contributor Author

samlev commented Aug 22, 2022

Ok, so I said "tomorrow" then disappeared for about a month, and I return... pretty much defeated.

I've got test cases in to test that the main "static" reflections work, but phpstan is being weird about the __invoke() method, and doesn't seem to (currently) have a good/clean way to hook in to it and inject the correct return type.

For now we can at least be sure that the cases can be called statically, and that PHPStan will correctly infer the return type. I'm going to leave invoking a case directly in the too hard basket. There are ways to deal with it in userland, but nothing that we can do from our end.

<?php
declare(strict_types = 1);

use ArchTech\Enums\InvokableCases;

enum Role
{
    use InvokableCases;

    case admin;
    case manager;
    case staff;
}

enum Suits: string
{
    use InvokableCases;

    case clubs = 'C';
    case diamonds = 'D';
    case hearts = 'H';
    case spades = 'S';
}

enum Status: int
{
    use InvokableCases;

    case created = 0;
    case running = 1;
    case done = 2;
}

function prtl(string $s) {
    echo sprintf("%s\n", $s);
}
function prtlint(int $i) {
    echo sprintf("%d\n", $i);
}

prtl(Role::admin->name); // admin
prtl(Role::manager()); // manager
prtl(Suits::clubs->value); // C
prtl(Suits::diamonds()); // D
prtl(Status::created->value); // PHPStan Error: Parameter #1 $s of function prtl expects string, int given.
prtl(Status::running()); // PHPStan Error: Parameter #1 $s of function prtl expects string, int given.

prtlint(Role::admin->name); // PHPStan Error: Parameter #1 $i of function prtlint expects int, string given.
prtlint(Role::manager()); // PHPStan Error: Parameter #1 $i of function prtlint expects int, string given.
prtlint(Suits::clubs->value); // PHPStan Error: Parameter #1 $i of function prtlint expects int, string given.
prtlint(Suits::diamonds()); // PHPStan Error: Parameter #1 $i of function prtlint expects int, string given.
prtlint(Status::created->value); // 0
prtlint(Status::running()); // 1

$staff = Role::staff;
$hearts = Suits::hearts;
$done = Status::done;

// Hacky method for getting type-hinting to pick up
/** @var callable():string $spades */
$spades = Suits::spades;

prtl($staff()); // staff
prtl($hearts()); // H
prtlint($spades()); // PHPStan Error: Parameter #1 $i of function prtlint expects int, string given.
prtl($done()); // Uncaught TypeError: prtl(): Argument #1 ($s) must be of type string, int given

Essentially PHPStan believes that all callables return mixed unless explicitly defined. It also struggles with the __invoke() method, because it doesn't see invoking a case as calling __invoke() on an enum case. It sees it as just a callable.

@stancl
Copy link
Member

stancl commented Aug 24, 2022

because it doesn't see invoking a case as calling __invoke() on an enum case. It sees it as just a callable.

Is this specific to enums? I.e. for normal class objects, it would understand __invoke(), but for enums it doesn't?

If that'd be the case then we could open an issue in the phpstan repo.

@samlev
Copy link
Contributor Author

samlev commented Aug 24, 2022

It's known behaviour (I'm not sure if it's intentional or not). I found this blog post talking about building a rule to detect callables without an explicit return type, and force you to document their return type.

I attempted to add a Dynamic Return Type Extension for UnitEnum when it used the InvokableCases trait, but the extension was never called because $enum() doesn't get picked up as calling __invoke() on an enum - it gets treated as a callable and the extension code never runs. I also couldn't get it to work as a DynamicFunctionReturnTypeExtension. The __invoke method also never gets passed to the other ReflectionExtension in hasMethod.

I also attempted to type-hint the return type of the __invoke method using conditional return types which showed promise, but still ran into an issue where I couldn't use $this, self, or static in any form to detect the backing type (if any) - it just threw errors about int not being declared in a @template tag, and @template tags only seem to work if there's a @param (which there isn't for this implementation of __invoke()).

It's possible that there is a bug in PHPStan around the conditional return types - it's fairly new, after all - and resolving that would allow us to put the return type hint in one place and resolve the issue. I'll probably revisit it later, but for now I've already burnt too much time chasing this issue.

e: For reference, the dynamic return type hint that I tried looked something like this:

/**
 * Return the enum's value when it's $invoked().
 *
 * @return (value-of<$this> is int ? int : string)
 */
public function __invoke()
{
    return $this instanceof BackedEnum ? $this->value : $this->name;
}

@stancl
Copy link
Member

stancl commented Aug 24, 2022

I see, thanks for looking into that in so much depth!

Should I merge the current version of the PR, or do you want to revisit and complete this later?

@samlev
Copy link
Contributor Author

samlev commented Aug 24, 2022

I'd be happy if you merged this now - it's still an improvement and I don't know when I'll have time to look into the __invoke() case.

@stancl
Copy link
Member

stancl commented Aug 24, 2022

So just to be sure: there's no "incomplete code" here, right? It just adds support for some things, even if not __invoke() yet.

@samlev
Copy link
Contributor Author

samlev commented Aug 24, 2022

No, nothing incomplete.

@stancl
Copy link
Member

stancl commented Aug 24, 2022

Awesome, thanks a lot for the PR!

@stancl stancl merged commit 373a86a into archtechx:master Aug 24, 2022
@samlev samlev deleted the feature/phpstanInvokableCases branch December 12, 2023 10:52
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.

None yet

2 participants