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

Ensure we recognize inherited static methods for the first-class callables #8370

Merged
merged 2 commits into from Aug 18, 2022

Conversation

someniatko
Copy link
Contributor

@someniatko someniatko commented Aug 4, 2022

fixes #8363

I have a strong feeling it's not the correct place where this functionality should belong.

For now first-class callables are implemented in the StaticCallAnalyzer just because they look like a static call syntax-wise (and are such from the perspective of nikic/php-parser). However, I am not sure where they should really belong: ClosureAnalyzer also seems odd here.

@someniatko
Copy link
Contributor Author

someniatko commented Aug 4, 2022

The original example from the issue still doesn't work: it says

Psalm\Exception\CodeException : InvalidArgument - src/somefile.php:27:38 - 
Argument 1 of takesIntToHolder expects Closure(int):Holder<C>, but impure-Closure(int):Holder<static> provided

@psalm-github-bot
Copy link

psalm-github-bot bot commented Aug 4, 2022

I found these snippets:

https://psalm.dev/r/0a4421aa23
<?php

/** @template T */
class Holder
{
    /** @param T $value */
    public function __construct(public $value) {}
}

abstract class A
{
    final public function __construct(public int $i) {}
    
    /** @return Holder<static> */
    public static function create(int $i): Holder
    {
        return new Holder(new static($i));
    }
}

class C extends A
{
}

/** @param \Closure(int):Holder<C> $_ */
function takesIntToHolder(\Closure $_): void {}

takesIntToHolder(C::create(...));
Psalm output (using commit 24f7920):

INFO: MixedArgumentTypeCoercion - 28:18 - Argument 1 of takesIntToHolder expects Closure(int):Holder<C>, parent type Closure provided

@someniatko
Copy link
Contributor Author

someniatko commented Aug 4, 2022

Okay, so I managed to fix my exact case in the last commit, however it still feels like a hack and duplicated efforts.

Needless to say it will suffer from other static problems which were fixed or maybe will be fixed in future for other features of the language.

Perhaps the proper way to implement the first-class callables is first to desugar them into a closure, or maybe a Closure::fromCallable(<xxx>) expression as per RFC?

@orklah
Copy link
Collaborator

orklah commented Aug 4, 2022

It's one of the feature that was implemented after Matt departed from the project, so sadly, it's possible it was added without a "big picture" mindset at the time.
I didn't work very much on first class callable either so I wouldn't be a good adviser on that.

Frankly, if you feel refactoring it would be best and you're up for it, go ahead! Now is the best moment to do so, Psalm 5 was not yet released so implementation changes won't bother anyone depending on it

@someniatko
Copy link
Contributor Author

Ok, however obviously I have even less vision than you do, by a lot.

Each such fix for me is a debugging exercise, and frankly I only fix those problems which I encounter myself, to solve my immediate problem :P

I suggest to merge this one then, but with the proper refactoring kept in mind, WDYT?

@orklah
Copy link
Collaborator

orklah commented Aug 4, 2022

Each such fix for me is a debugging exercise

There's a few modules in Psalm I know and I can pinpoint some issues, but aside from that, I'm really the same. For example, I'm pretty sure you know more than me about static issues because I never went check how it works and it would take me a lot of time too :)

I'm up to merge this :)

@someniatko someniatko marked this pull request as ready for review August 4, 2022 19:23
@someniatko
Copy link
Contributor Author

I'm up to merge this :)

@orklah pingie 👉👈

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Aug 18, 2022
@orklah orklah merged commit 7ee3a81 into vimeo:4.x Aug 18, 2022
@orklah
Copy link
Collaborator

orklah commented Aug 18, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

first-class callables type is not inferred if it is an inherited static method
2 participants