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

function (Model $x) {} passed to Closure(object): void is not reported in level <=6 #7342

Open
mvorisek opened this issue May 29, 2022 · 8 comments
Labels
Milestone

Comments

@mvorisek
Copy link
Contributor

mvorisek commented May 29, 2022

Bug report

Currently, with level 3 - 6, Closure(object) accepts Closure(X extends Cl), but Closure(Cl) does not. This is inconsistent as object is a weaker type.

With #8964 implemented, this issue should actually aim to not accept function (Model $x) {} into Closure(object): void property/param.

Code snippet that reproduces the problem

repro: https://3v4l.org/gIfXB (TypeErrorException)
https://phpstan.org/r/9e11c8b2-622b-4520-88c1-10ec04c86378 (no phpstan error for level <= 6)

Expected output

phpstan type error

@mvorisek mvorisek changed the title Closure(Cl) should accept Closure(X extends Cl) as Closure(object) does on level 3 Closure(Cl) should accept Closure(X extends Cl) as Closure(object) does on level 3 May 29, 2022
@ondrejmirtes ondrejmirtes added this to the Easy fixes milestone May 30, 2022
@phpstan-bot
Copy link
Contributor

@mvorisek After the latest push in 1.11.x, PHPStan now reports different result with your code snippet:

@@ @@
-25: Property UserAction::$enabled2 (bool|(Closure(Model): bool)) does not accept Closure(X): false.
+No errors

@ondrejmirtes
Copy link
Member

Changed: phpstan/phpstan-src@eceb9f6

@ondrejmirtes
Copy link
Member

Decided to revert the fix.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jun 22, 2023

Allowing Closure(Cl) to accept Closure(X extends Cl) is on the other side quite dangerous as it leads to a TypeError.

Example:

class Cl {}
class X extends Cl {}

class Foo {
    /** @var \Closure(Cl): void */
    public $fx;

    public function run(): void {
        ($this->fx)(new Cl());
    }
}

$foo = new Foo();
$foo->fx = function (X $obj): void { var_dump($obj); };
$foo->run();

https://3v4l.org/ddhWL

Maybe this should be solved with something like type projection like in phpstan/phpdoc-parser#138 as "\Closure(xxx) is more or less generic Closure, only the syntax uses ( and not <".

So the clean way to allow Closure with subclass params acceptance should be like \Closure(object of Cl).

@jiripudil what do you think


real world example:

https://github.com/atk4/ui/blob/67cfdbd00d/src/Form/Control/Lookup.php#L57

the property must accept function (X $fx, array $arr) {} where X extends Model as X is needed to hind/typecheck the passed model, but the Control class cannot be made generic.

@ondrejmirtes
Copy link
Member

This is correctly reported https://phpstan.org/r/94ae1e00-8bb6-47dd-aa9e-06fad4a45880

@mvorisek
Copy link
Contributor Author

That is correct. But currently, phpdoc cannot be used to specify the Closure should accept class that is or extends Model as the 1st param. This issue is to allow such requirement properly documented better than object.

@jiripudil
Copy link
Contributor

Type projections wouldn't help you, even if we implemented them for Closures; you'd need a covariant projection, but Closure/callable parameters are contravariant, as you've demonstrated. That's an impossible combination.

@mvorisek
Copy link
Contributor Author

mvorisek commented Jun 22, 2023

You are both right. With #8964 implemented... I have updated the issue title and the description.

@mvorisek mvorisek changed the title Closure(Cl) should accept Closure(X extends Cl) as Closure(object) does on level 3 function (Model $x) {} passed to Closure(object): void is not reported in level <=6 Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants