Skip to content

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

Closed
wants to merge 8 commits into from

Conversation

samsonasik
Copy link
Contributor

@samsonasik samsonasik commented May 2, 2025

Start from PHPStan 2.1.14, in Rector, we got notice when downgrade code:

class ParentClassWithToStringMixedReturn
{
    public function __toString()
    {
        return 'a';
    }
}

class WithParentMixedReturn extends ParentClassWithToStringMixedReturn
{
    public function __toString()
    {
        return 'value';
    }
}

echo (new WithParentMixedReturn());

This add string return to WithParentMixedReturn::__toString() see https://3v4l.org/IrE2q#v7.4.33

The issue is when code consumed by user: https://3v4l.org/kdcEh#v7.4.33 it got error:

Fatal error: Declaration of Consumer::__toString() must be compatible with WithParentMixedReturn::__toString(): string in /in/kdcEh on line 21

On our rector-src code, I patch there with use our AstResolver to specify on __toString when no return and non-built in

to return MixedType

@TomasVotruba if this accepted, we may can rollback to original implementation 👍

@samsonasik samsonasik changed the title Only set return native type from null return type when declaring class is built in Only set return native type from null return type when declaring class is built in on __toString() May 2, 2025
@samsonasik
Copy link
Contributor Author

samsonasik commented May 2, 2025

@ondrejmirtes ready for review, not sure about the php 7.4 notice:

There was 1 failure:

1) PHPStan\Analyser\NodeScopeResolverTest::testFile with data set "tests/PHPStan/Analyser/nsrt/bug-2471.php" ('/home/runner/work/phpstan-src...71.php')
Failed assertions in /home/runner/work/phpstan-src/phpstan-src/tests/PHPStan/Analyser/nsrt/bug-2471.php:

Line 26:
Expected: array<string, null>
Actual:   array<null>

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. How is __toString() different from other methods handled by this method?
  2. Personally I'd like a test + revert of the commit that changed this (3854cbc) instead of this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. we found by case, for current case is __toString() so don't know what other needs
  2. yes, I think revert is better for this case, as native is native on overridden method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Successfully merging this pull request may close these issues.

2 participants