Improvements and fixes #19
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ondrejmirtes
force-pushed
the
allow-using-attributes
branch
from
17a4a0d to
80f0f11
Compare
|
@ondrejmirtes I'll review it in a bit and let you know. Thanks! |
|
I just realized that although a working proof of concept, the approach of extending TypeNodeResolver isn't viable in the longterm. Because multiple extensions extending it would clash and they wouldn't be usable all at the same time. I'll need to rework it with some sort of extension system. Stay tuned :) |
|
Thanks @ondrejmirtes! I have to admit that my prev PR was done by trial & error, so I was expecting some mistakes in there... |
|
@ondrejmirtes I was testing it locally in my project and it didn't really work (lots of |
|
Make sure you delete PHPStan’s cache.
On Tue, 24 Jul 2018 at 00:36, Luís Cobucci ***@***.***> wrote:
@ondrejmirtes <https://github.com/ondrejmirtes> I was testing it locally
in my project and it didn't really work (lots of Call to an undefined
method $CLASSNAME::reveal()). Will do more tests tomorrow
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGZuMSkqbqWwHjRKyxiNDuBWasDovimks5uJk_agaJpZM4VbiPN>
.
--
Ondřej Mirtes
|
|
@ondrejmirtes my bad, I didn't read your DM when I was doing the tests. Apart from having to remove the cache all the time, it works fine for most cases. The one that didn't work was this: final class BlahTest
{
/** @var MyClass[]|ObjectProphecy[] */
private $prophecies;
// same setup as before but in an array instead...
}This is an edge-case that doesn't need to be supported IMO, I can just reorganise the tests. Thanks for your help! |
lcobucci
reviewed
Jul 24, 2018
| @@ -12,8 +12,10 @@ | |||
| "php": "^7.1", | |||
| "phpspec/prophecy": "^1.7", | |||
| "phpunit/phpunit": "^6.0||^7.0", | |||
| "phpstan/phpstan": "^0.10" | |||
| "phpstan/phpstan": "^0.10.3" | |||
There was a problem hiding this comment.
I'd say that using ^0.10.3@dev would be more appropriated for testing, just to avoid issues with other deps.
src/PhpDoc/TypeNodeResolver.php
Outdated
|
|
||
| protected function resolveUnionTypeNode(UnionTypeNode $typeNode, NameScope $nameScope): Type | ||
| { | ||
| if (count($typeNode->types) === 2) { |
There was a problem hiding this comment.
We can refactor this a bit to make things a bit more readable, my suggestion:
protected function resolveUnionTypeNode(UnionTypeNode $typeNode, NameScope $nameScope): Type
{
if (count($typeNode->types) !== 2) {
return parent::resolveUnionTypeNode($typeNode, $nameScope);
}
$objectProphecy = $this->getObjectProphecy($typeNode->types[0], $typeNode->types[1], $nameScope);
return $objectProphecy ?? parent::resolveUnionTypeNode($typeNode, $nameScope);
}
private function getObjectProphecy(
TypeNode $argumentOne,
TypeNode $argumentTwo,
NameScope $nameScope
): ?ObjectProphecyType {
$typeOne = $this->resolve($argumentOne, $nameScope);
$typeTwo = $this->resolve($argumentTwo, $nameScope);
if (! $typeOne instanceof TypeWithClassName || ! $typeTwo instanceof TypeWithClassName) {
return null;
}
if ($typeOne->getClassName() === ObjectProphecy::class) {
return new ObjectProphecyType($typeTwo->getClassName());
}
if ($typeTwo->getClassName() === ObjectProphecy::class) {
return new ObjectProphecyType($typeOne->getClassName());
}
return null;
}
src/PhpDoc/TypeNodeResolver.php
Outdated
| use PHPStan\Type\TypeWithClassName; | ||
| use Prophecy\Prophecy\ObjectProphecy; | ||
|
|
||
| class TypeNodeResolver extends \PHPStan\PhpDoc\TypeNodeResolver |
There was a problem hiding this comment.
This class could be final
ondrejmirtes
force-pushed
the
allow-using-attributes
branch
from
80f0f11 to
14e004a
Compare
|
Commit updated with the latest (and hopefully final) extension API: 3953120 Feel free to refactor it, in my opinion the current code is fine 😊 |
Jean85
approved these changes
Jul 27, 2018
@ondrejmirtes it worked fine for me. Did you fix the cache handling issue? I had to manually clean it...
That's up to @Jan0707, my suggestion was just to reduce to cyclomatic complexity of |
|
Yeah, the caching should be solved, try to run the code without the
extension and with it, you should get the corresponding type each time
(union without it and intersection with it) which will mean that the
caching invalidation works.
On Fri, 27 Jul 2018 at 11:17, Luís Cobucci ***@***.***> wrote:
Commit updated with the latest (and hopefully final) extension API:
3953120
<3953120>
@ondrejmirtes <https://github.com/ondrejmirtes> it worked fine for me.
Did you fix the cache handling issue? I had to manually clean it...
Feel free to refactor it, in my opinion the current code is fine 😊
That's up to @Jan0707 <https://github.com/Jan0707>, my suggestion was
just to reduce to cyclomatic complexity of
TypeNodeResolverExtension#resolve(), which is quite high IMO.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGZuLdzip9fg6yKyM8HidMu-WBT7SwMks5uKtq0gaJpZM4VbiPN>
.
--
Ondřej Mirtes
|
|
@ondrejmirtes if I run phpstan twice (with the extension enabled) the first time (clean cache) passes, the sencond doesn't. |
|
The errors are related to the Full list |
|
@lcobucci That's really weird behaviour that I didn't replicate in my testing. Can you debug what's going on? Inside the TypeNodeResolver where the extensions are called (and also possibly inside the |
|
@ondrejmirtes sure, I'll do it tomorrow 👍 |
|
@ondrejmirtes it seems that |
|
@lcobucci Yes, that's what the cache is for 😀 The correct stuff should be already cached in FileTypeMapper (the |
|
@ondrejmirtes exactly, |
|
@ondrejmirtes the result of |
|
For the cache key |
|
So the question is - what gets actually cached under the cache key |
|
@ondrejmirtes I just saw that the map is indeed the same (regardless if fetched from cache or from doc blocks) |
|
The funny thing is that the call |
|
That's super-weird, please investigate more 😊 |
|
@ondrejmirtes I don't have access to the repo anymore, @rdohms could you please take a look at this? |
… no longer part of MethodReflection
ondrejmirtes
force-pushed
the
allow-using-attributes
branch
from
14e004a to
1852454
Compare
|
I just released stable PHPStan 0.10.3 (I'm gonna tweet about it tommorrow) so I updated this PR to use this version. I wasn't able to reproduce the cache problem so we'll see if someone else has more "luck" 😊 |
asgrim
approved these changes
Nov 1, 2018
|
so, what can I do to help this one along? do we still have to dive into the cache issue? |
|
This needs to have the conflicts resolved - then we should be good to go. |
|
I've got it rebased, i'm just trying to figure out how to get that code into this PR 😝 |
Merged
|
Closed in favor of #31 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi,
I'm sending a few improvements, including implementation of @lcobucci's failing test case concerning prophets in class property. It now depends on dev version of PHPStan but after you test it and tell me it's okay, I can release the change.
FYI @Jean85 this commit 80f0f11 fixes your code upgrade to 0.10 - it was a mistake to return
TrivialParametersAcceptorbecause it disabled all checks on the related object - it didn't seeMethodProphecyreturn type butmixed.