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

polymorphic string based checks #1330

Closed
exussum12 opened this issue Aug 4, 2018 · 8 comments
Closed

polymorphic string based checks #1330

exussum12 opened this issue Aug 4, 2018 · 8 comments

Comments

@exussum12
Copy link

I'm not sure if this is an error, as the code should check in a better way but I have come across the following

https://phpstan.org/r/051b7ab22cbb5241e32200e723a0bf3c

This should use instanceof ideally which resolves the issue, just wondering if code like this should also verify ?

Using something like

switch ($object->getType()) {
     case something::class :
}

vs

switch (true) {
     case $object instanceof something :
}
@ondrejmirtes
Copy link
Member

ondrejmirtes commented Aug 4, 2018 via email

@JanTvrdik
Copy link
Contributor

JanTvrdik commented Aug 4, 2018

I think that the idea is that when $obj->getType() return value of type static<$obj> then $obj->getType() === Foo::class should be resolved the same way as $obj instanceof Foo which would require understanding that static<$obj> depends on type of $obj.

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Aug 4, 2018

This might be doable with a custom type-specifying extension, you can find out about them in the README and there's plenty of examples in the codebase:

function test(contract $c) {
	if ($c->getType() === concrete::class) {
		concrete($c);	
	}
}

Supporting it in a switch statement is a completely different beast and it's not possible right now. Only these two forms are now supported in switch:

switch (true) {
    case $foo instanceof Foo:
}

And:

switch (get_class($foo)) {
    case Foo::class:
}

Waiting for confirmation from @exussum12 that this is solved so I can close this.

@exussum12
Copy link
Author

exussum12 commented Aug 4, 2018

Sorry, reading it back its not too clear

https://phpstan.org/r/25cbd9a4d5f1e16972e503079d3819b7

The result of the two loops are really identical, though the top loop checks by comparing strings and bottom loop checks by using instanceof.

Not sure if this should be a bug, as although its type safe - knowing that by comparing strings doesn't feel right.

This is only an issue with late static bindings.

Hopefully this example makes the issue more clear.

The code can 100% be improved here, its just as an example

@ondrejmirtes
Copy link
Member

Yeah, PHPStan has no way of understanding the connection between $a->getType() string and $a type without a type-specifying extension. And it still won't work with switch statements because it wasn't thought of during implementation.

@exussum12
Copy link
Author

ok so a custom type check would be fine if the switch was converted to a few ifs ?

@ondrejmirtes
Copy link
Member

I'm now realizing that you can't write an extension for $a->getType === Foo::class because it's not a function/method. You'd have to rewrite your code to something like if (isType($a->getType(), Foo::class)), then it would work (you can write type-specifying extensions for functions, instance methods and static methods).

What are you implementing using the getType() method that you cannot use instanceof?

@exussum12
Copy link
Author

Im not implementing this is in older code.

I have moved this for now to the switch (true) method, I just raised the bug report as technically the code was correct before also. Though I understand why it wouldnt be picked up as being right

@lock lock bot locked as resolved and limited conversation to collaborators Dec 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants