Skip to content
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

False-positive: Access to an uninitialized readonly property #10523

Closed
staabm opened this issue Feb 1, 2024 · 10 comments
Closed

False-positive: Access to an uninitialized readonly property #10523

staabm opened this issue Feb 1, 2024 · 10 comments
Labels

Comments

@staabm
Copy link
Contributor

staabm commented Feb 1, 2024

Bug report

I have a class

<?php

final class CheckoutController11 extends Zend_Controller_Action
{
    private readonly B $userAccount;

    public function __construct(Zend_Controller_Request_Abstract $request, Zend_Controller_Response_Abstract $response, array $invokeArgs = [])
    {
        $this->userAccount = new B();

        parent::__construct($request, $response, $invokeArgs);
    }

    public function init(): void
    {
        $this->redirectIfNkdeCheckoutNotAllowed();
        $this->redirectIfNoShoppingBasketPresent();
    }

    private function redirectIfNkdeCheckoutNotAllowed(): void
    {

    }

    private function redirectIfNoShoppingBasketPresent(): void
    {
        $x = $this->userAccount;
    }

}

class B {}

and PHPStan configured with additionalConstructors:

parameters:
    additionalConstructors:
        - Zend_Controller_Action::init 

and I get the following errors:

$ vendor/bin/phpstan analyse -c application/mbw_desktopRocket/phpstan.neon.dist application/mbw_desktopRocket/controllers/CheckoutController.php  --debug

C:\dvl\Workspace\mbw\application\mbw_desktopRocket\controllers\CheckoutController.php
 ------ ---------------------------------------------------------------------------------------------------------------------------
  Line   CheckoutController.php
 ------ ---------------------------------------------------------------------------------------------------------------------------
  7      Method CheckoutController11::__construct() has parameter $invokeArgs with no value type specified in iterable type array.
         💡 See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
  27     Access to an uninitialized readonly property CheckoutController11::$userAccount.
 ------ --------------------------------------------------------------------------------------------------------------------------

I am wondering about the 27 Access to an uninitialized readonly property CheckoutController11::$userAccount. error.

I was not yet able to reproduce the problem in a small file outside of my project.

Code snippet that reproduces the problem

https://phpstan.org/r/dc8caf8e-bc01-4d08-9f65-172bb9235bb2

Expected output

no error on line 27, because of the initialization in __construct

Did PHPStan help you today? Did it make you happy in any way?

No response

@ondrejmirtes
Copy link
Member

Yeah, the reproduction would be useful, ideally as a rule test case in phpstan-src.

@staabm
Copy link
Contributor Author

staabm commented Feb 1, 2024

I managed to reproduce it in phpstan/phpstan-src#2897 - see https://github.com/phpstan/phpstan-src/actions/runs/7744684608/job/21118995083?pr=2897

could you have a look into a possible fix? I had a look already, but this stuff looks pretty complicated and its a new area for me atm

@ondrejmirtes
Copy link
Member

I was touching this at some point last summer. This touches a similar issue phpstan/phpstan-src@218aad0

This was quite a substantial rewrite of the feature phpstan/phpstan-src@348debc

@staabm
Copy link
Contributor Author

staabm commented Feb 2, 2024

after hours of investigation the only way I see this could be fixed - without regressing existing expected errors - would be to gather all property-fetches of __construct() until the first method call happens.

that way we would know which properties will always exist and at the same time don't mix it up with inter-method-call assignments which can have various states - see e.g. https://github.com/phpstan/phpstan-src/blob/29a68b75ce9fa885886395df1acd23c06cff2a14/tests/PHPStan/Rules/Properties/data/missing-readonly-property-assign.php#L231-L246

to achieve this I would need to add these statements as a arg of ClassPropertiesNode and pass them in from NodeScopeResolver via ClassStatementsGatherer.

wdyt?

@ondrejmirtes
Copy link
Member

I feel like very similar situations in code are already well understood without false positives so this should be an easy fix. But I'd have to look into this myself to verify that.

@staabm
Copy link
Contributor Author

staabm commented Feb 2, 2024

I think the current test-cases like e.g.

https://github.com/phpstan/phpstan-src/blob/29a68b75ce9fa885886395df1acd23c06cff2a14/tests/PHPStan/Rules/Properties/data/missing-readonly-property-assign.php#L231-L246

work because the methods are called directly from __construct.

the difference in my repro-case is, that the caller is a "additionalConstructor" and the scope of this additionalConstructor does not know about the scope of __construct

@staabm
Copy link
Contributor Author

staabm commented Feb 2, 2024

after a lunch walk I got an idea and implemented a fix in phpstan/phpstan-src#2897

@phpstan-bot
Copy link
Contributor

@staabm After the latest push in 1.11.x, PHPStan now reports different result with your code snippet:

@@ @@
-PHP 8.1 – 8.3 (2 errors)
+PHP 8.1 – 8.3 (5 errors)
 ==========
 
- 3: Class CheckoutController11 extends unknown class Zend_Controller_Action.
-14: Call to an undefined method CheckoutController11::redirectIfNkdeCheckoutNotAllowed().
+-1: Reflection error: Zend_Controller_Action not found.
+ 3: Reflection error: Zend_Controller_Action not found.
+ 3: Reflection error: Zend_Controller_Action not found.
+ 3: Reflection error: Zend_Controller_Action not found.
+ 3: Reflection error: Zend_Controller_Action not found.
 
 PHP 7.2 – 8.0 (1 error)
 ==========
 
  5: Syntax error, unexpected T_STRING, expecting T_VARIABLE on line 5
Full report

PHP 8.1 – 8.3 (5 errors)

Line Error
-1 Reflection error: Zend_Controller_Action not found.
3 Reflection error: Zend_Controller_Action not found.
3 Reflection error: Zend_Controller_Action not found.
3 Reflection error: Zend_Controller_Action not found.
3 Reflection error: Zend_Controller_Action not found.

PHP 7.2 – 8.0 (1 error)

Line Error
5 Syntax error, unexpected T_STRING, expecting T_VARIABLE on line 5

@ondrejmirtes
Copy link
Member

Fixed phpstan/phpstan-src#2897

Copy link

github-actions bot commented May 6, 2024

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants