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

is_callable is not taken into account #5082

Closed
transistive opened this issue Jan 22, 2021 · 21 comments
Closed

is_callable is not taken into account #5082

transistive opened this issue Jan 22, 2021 · 21 comments

Comments

@transistive
Copy link

Hello,

Psalm is giving me a hard time in a case resembling this snippet: https://psalm.dev/r/583d409fde.

I am trying to accept a lazy-loaded variable which is possibly wrapped around a closure. If I test it with is_callable, it still complains the type might not be a callable. I don't know what causes this.

Kind regards,
Ghlen

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/583d409fde
<?php

/**
 * @psalm-type X = array{test: string}
 * @psalm-type LazyX = callable():(X|null)|X|null
 */

/**
 * @param LazyX $x
 * @return X|null
 */
function read_lazy_x($x)
{
   if (is_callable($x)) {
    	return call_user_func($x); 
   }
   return $x;
}
Psalm output (using commit 28d2795):

ERROR: PossiblyInvalidFunctionCall - 15:13 - Cannot treat type callable-array{test: string} as callable

@weirdan
Copy link
Collaborator

weirdan commented Jan 22, 2021

array{test: string} means something like ["test" => "foobar"] which cannot be callable. You want two element array like this: https://psalm.dev/r/26400469cc

@weirdan weirdan closed this as completed Jan 22, 2021
@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/26400469cc
<?php

/**
 * @psalm-type X = array{string|object,string}
 * @psalm-type LazyX = callable():(X|null)|X|null
 */

/**
 * @param LazyX $x
 * @return X|null
 */
function read_lazy_x($x)
{
   if (is_callable($x)) {
    	return call_user_func($x); 
   }
   return $x;
}
Psalm output (using commit 28d2795):

No issues!

@transistive
Copy link
Author

Hello Weirddan,

Thank you for the swift response! I understand why that is not callable but isn't that what is_callable is for? How can Psalm think it not to be a valid callable, right after I test it to be callable?

Kind regards,

Ghlen

@orklah
Copy link
Collaborator

orklah commented Jan 22, 2021

technically, string(test) could be the supertype of class-string<test> so it may very well be callable. A little unorthodox though...

@transistive
Copy link
Author

I still don't fully understand. If I follow this logic correctly, the key within the array ["test" => "foobar"] is possibly callable, but how does that make the array callable? Furthermore, how does psalm essentially interprets the result of is_callable($x) to be a false positive?

@orklah
Copy link
Collaborator

orklah commented Jan 22, 2021

nope sorry, possible callable would be array{string(test), string}, not array<string(test), string>. I misread your code and I'm not sure I actually understand what's going on

@transistive
Copy link
Author

Thank you. To me, it just looks like a false positive. The example works in simpler cases such as in the second function in this snippet: https://psalm.dev/r/5eece5d7da.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/5eece5d7da
<?php

/**
 * @psalm-type X = array{test: string}
 * @psalm-type LazyX = callable():(X|null)|X|null
 */

/**
 * @param LazyX $x
 * @return X|null
 */
function read_lazy_x($x)
{
   if (is_callable($x)) {
    	return call_user_func($x); 
   }
   return $x;
}

/**
 * @param callable():string|string $x
 */
function read_no_false_positive_example($x): string
{
   if (is_callable($x)) {
    	return call_user_func($x); 
   }
   return $x;
}
Psalm output (using commit d97db04):

ERROR: PossiblyInvalidFunctionCall - 15:13 - Cannot treat type callable-array{test: string} as callable

@orklah
Copy link
Collaborator

orklah commented Jan 22, 2021

Can you explain why the return type of the callable is (X|null)|X|null ?

Psalm is doing weird things around that:
with X instead, it's a redundant is_callable:
https://psalm.dev/r/0ae0ab2322
with X|null, it becomes OK:
https://psalm.dev/r/54ec32ea26
with X|null|(X|null) your issue is back:
https://psalm.dev/r/efea950ba4
But it's not due to parenthesis because X|null|(string) is ok:
https://psalm.dev/r/fefad7a0c4

@weirdan I believe we can reopen this, there's something fishy here

@psalm-github-bot
Copy link

I found these snippets:

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

/**
 * @psalm-type X = array{test: string}
 * @psalm-type LazyX = callable():X
 */

/**
 * @param LazyX $x
 * @return mixed
 */
function read_lazy_x($x)
{
   if (is_callable($x)) {
    	return call_user_func($x); 
   }
   return $x;
}
Psalm output (using commit d97db04):

ERROR: RedundantConditionGivenDocblockType - 14:8 - Docblock-defined type callable():array{test: string} for $x is always callable
https://psalm.dev/r/54ec32ea26
<?php

/**
 * @psalm-type X = array{test: string}
 * @psalm-type LazyX = callable():X|null
 */

/**
 * @param LazyX $x
 * @return mixed
 */
function read_lazy_x($x)
{
   if (is_callable($x)) {
    	return call_user_func($x); 
   }
   return $x;
}
Psalm output (using commit d97db04):

No issues!
https://psalm.dev/r/efea950ba4
<?php

/**
 * @psalm-type X = array{test: string}
 * @psalm-type LazyX = callable():X|null|(X|null)
 */

/**
 * @param LazyX $x
 * @return mixed
 */
function read_lazy_x($x)
{
   if (is_callable($x)) {
    	return call_user_func($x); 
   }
   return $x;
}
Psalm output (using commit d97db04):

ERROR: PossiblyInvalidFunctionCall - 15:13 - Cannot treat type callable-array{test: string} as callable
https://psalm.dev/r/fefad7a0c4
<?php

/**
 * @psalm-type X = array{test: string}
 * @psalm-type LazyX = callable():X|null|(string)
 */

/**
 * @param LazyX $x
 * @return mixed
 */
function read_lazy_x($x)
{
   if (is_callable($x)) {
    	return call_user_func($x); 
   }
   return $x;
}
Psalm output (using commit d97db04):

No issues!

@transistive
Copy link
Author

Hey Orklah,

Thank you for helping. I am using this callable structure for lazy evaluation. On the other hand, the value is also optional, hence the callable():(X|null)|X|null. My library is customisable through injections, but these injections can be quite heavy, so I am using the lazy evaluation pattern. Sometimes it seems a bit like overkill, but for some injections, it is not. So for the sake of consistency, all injections can be lazily injected.

I could have also written it like this: https://psalm.dev/r/df8b0887f4, but as you can see, the result is the same.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/df8b0887f4
<?php

/**
 * @psalm-type X = array{test: string}|null
 * @psalm-type LazyX = callable():X|X
 */

/**
 * @param LazyX $x
 * @return mixed
 */
function read_lazy_x($x)
{
   if (is_callable($x)) {
    	return call_user_func($x); 
   }
   return $x;
}
Psalm output (using commit d97db04):

ERROR: PossiblyInvalidFunctionCall - 15:13 - Cannot treat type callable-array{test: string} as callable

@orklah
Copy link
Collaborator

orklah commented Jan 22, 2021

What I meant precisely is that I don't see the difference between
(X|null)|X|null and simply X|null
Also in your newest example
X|X is strictly equivalent to X

And when you remove this duplication, it seems to work, because your is_callable check is, in fact, really redundant

@transistive
Copy link
Author

But the type isn't (X|null)|X|null, its callable():(X|null)|X|null. If I understand correctly callable():X|null denotes a callable which returns X or null.

@orklah
Copy link
Collaborator

orklah commented Jan 22, 2021

ooooooooh I get it now
I'm not sure if psalms read this as
(callable():(X|null))|X|null
or
callable():((X|null)|X|null)

I'll run some tests

@orklah
Copy link
Collaborator

orklah commented Jan 22, 2021

Ok!
Here's what Psalm does:
https://psalm.dev/r/25d05302f0

When entering is_callable, it will remove null (not callable) and it will transform your array{test: string} as a callable-array[test: string}
so the resulting type in the if is callable():(array{test: string}|null) | callable-array{test: string}
It kinda makes sense and I think Psalm does this to be able to transform an array into a callable-array in some occasions, but I'm not sure this is legit for keyed arrays... This is debatable...

In practice, you now have a dual type where one side is a legit callable and the other side will fail when trying to interpret it in call_user_func.
Psalm could use the code that checks the validity of the callable-array to avoid transforming an array that will fail later... Not sure.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/25d05302f0
<?php

/**
 * @psalm-type X = array{test: string}
 * @psalm-type LazyX = callable():(X|null)|X|null
 */

/**
 * @param LazyX $x
 * @return mixed
 */
function read_lazy_x($x)
{
    /** @psalm-trace $x */;
   if (is_callable($x)) {
        /** @psalm-trace $x */;
    	return call_user_func($x); 
   }
   return $x;
}
Psalm output (using commit d97db04):

INFO: Trace - 14:27 - $x: array{test: string}|callable():(array{test: string}|null)|null

INFO: Trace - 16:31 - $x: callable():(array{test: string}|null)|callable-array{test: string}

ERROR: PossiblyInvalidFunctionCall - 17:13 - Cannot treat type callable-array{test: string} as callable

@transistive
Copy link
Author

Thank you for the detailed explanation! I learned a lot and never knew about psalm-trace, very interesting!

From the way I understand it, the callable-array concept seems a bit over-generalized and deeply integrated into the type system. I hope this will get addressed some time, but I understand it is not high on the priority list.

In any case, there doesn't seem to be a lot I can do in this case except using @psalm-ignore.

@orklah
Copy link
Collaborator

orklah commented Jan 22, 2021

Found an easy solution. Your array has only 1 element so it's easy to exclude it from being transformed into an array-callable.

If the PR is accepted, it should solve your issue

@transistive
Copy link
Author

This was incredibly fast! Thanks orklah :)

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

No branches or pull requests

3 participants