Skip to content

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.

}
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