-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
[PHPStanStaticTypeMapper] Do not crash on numeric string on ReturnTypeDeclarationRector #1588
Conversation
All checks have passed 🎉 @TomasVotruba it is ready for review. |
<?php | ||
|
||
namespace Rector\Tests\TypeDeclaration\Rector\FunctionLike\ReturnTypeDeclarationRector\FixtureForPhp81; | ||
|
||
class DoNotCrashOnNumericString | ||
{ | ||
public function run(): string | ||
{ | ||
return bcadd('10', '5'); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add test fixture withotu ): string
return type and the other half that adds it? So we're sure this type is completed.
<?php
namespace Rector\Tests\TypeDeclaration\Rector\FunctionLike\ReturnTypeDeclarationRector\FixtureForPhp81;
class DoNotCrashOnNumericString
{
public function run()
{
return bcadd('10', '5');
}
}
?>
-----
<?php
namespace Rector\Tests\TypeDeclaration\Rector\FunctionLike\ReturnTypeDeclarationRector\FixtureForPhp81;
class DoNotCrashOnNumericString
{
public function run(): string
{
return bcadd('10', '5');
}
}
?>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New PHPStan 1.3 just released https://github.com/phpstan/phpstan/releases/tag/1.3.0 and as we use ^1.2
in composer.json, it will automatically updated and #1589 and make error as new PR #1589
I will pin to PHPStan 1.2 and add new fixture for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will pin to PHPStan 1.2 and add new fixture for it.
Thanks 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomasVotruba I applied:
-
temporary pin phpstan to 1.2 as 1.3 cause error like in PR Update to PHPStan ^1.3 #1589 as currently we use
^1.2
-
add new fixture for no return declaration, it seems currently cause different result diff:
- public function run(): string
+ public function run()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, it seems because bcadd()
function not exists, so no return type is expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with add the missing function and add string handling 50a27aa so it can add return type declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 👍
All checks have passed 🎉 @TomasVotruba I think it is ready. |
*/ | ||
public function mapToPHPStanPhpDocTypeNode(Type $type, TypeKind $typeKind): TypeNode | ||
{ | ||
return new IdentifierTypeNode('string'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phpstan also supports numeric-string
.. not sure this would be correct to use here though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PHP doc is also for PHPStorm, that understands only string
type.
Rebase on |
53b3a45
to
237bac1
Compare
237bac1
to
98be33c
Compare
Thank you 👍 |
@samsonasik @TomasVotruba thank you both! |
It previously got the following error:
This PR Fixes rectorphp/rector#6896