-
Notifications
You must be signed in to change notification settings - Fork 508
Only set return native type from null return type when declaring class is built in on __toString() #3978
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
Conversation
@ondrejmirtes ready for review, not sure about the php 7.4 notice:
https://github.com/phpstan/phpstan-src/actions/runs/14802288573/job/41563600230?pr=3978#step:6:226 since other php versions green, could you give me hint to make it green? Thank you. |
@@ -330,7 +330,13 @@ private function getNativeReturnType(): Type | |||
return $this->nativeReturnType = new VoidType(); | |||
} | |||
if ($name === '__tostring') { | |||
return $this->nativeReturnType = new StringType(); | |||
return $this->nativeReturnType = $this->declaringClass->isBuiltin() |
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.
- How is
__toString()
different from other methods handled by this method? - Personally I'd like a test + revert of the commit that changed this (3854cbc) instead of this change.
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.
- we found by case, for current case is
__toString()
so don't know what other needs - yes, I think revert is better for this case, as native is native on overridden method.
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.
Start from PHPStan 2.1.14, in
Rector
, we got notice when downgrade code:This add
string
return toWithParentMixedReturn::__toString()
see https://3v4l.org/IrE2q#v7.4.33The issue is when code consumed by user: https://3v4l.org/kdcEh#v7.4.33 it got error:
On our rector-src code, I patch there with use our AstResolver to specify on
__toString
when no return and non-built into return
MixedType
@TomasVotruba if this accepted, we may can rollback to original implementation 👍