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

Simulate issset for null coalesce operator #1104

Closed
wants to merge 8 commits into from

Conversation

rajyan
Copy link
Contributor

@rajyan rajyan commented Mar 22, 2022

@rajyan
Copy link
Contributor Author

rajyan commented Mar 22, 2022

Several thing to consider and fix.
The test fails because the behaviour for coalesce and isset ternary is slightly different with undefined variables.
Although

isset($a) ? $a : null;
isset($a->b) ? $a->b : null;

$a ?? null;
$a->b ?? null;

are all valid (no errors, but kind of buggy), currently PHPStan throws an error Undefined variable: $a for isset ternary while coalesce dosen't.

Also, it may be misleading, if IssetRule errors occur by coalesce.

@rajyan rajyan mentioned this pull request Mar 22, 2022
@rajyan
Copy link
Contributor Author

rajyan commented Mar 22, 2022

I should separate the PR for 6870

@rajyan
Copy link
Contributor Author

rajyan commented Mar 22, 2022

@rajyan rajyan changed the title Fix/issue 6870 Simulate issset ternary for null coalesce operator Mar 23, 2022
@rajyan rajyan force-pushed the fix/issue-6870 branch 2 times, most recently from 1562e4b to 148f35e Compare March 24, 2022 11:20
@rajyan
Copy link
Contributor Author

rajyan commented Mar 24, 2022

@rajyan rajyan changed the title Simulate issset ternary for null coalesce operator Simulate issset for null coalesce operator Mar 24, 2022
@rajyan
Copy link
Contributor Author

rajyan commented Mar 25, 2022

The failures in the tests looks good to me, but it's definitely a breaking change.
Two differences of Coalesce and Isset_ now (before this PR)

  1. PropertyFetch in isset is not considered as read
    class Foo
    {
    private string $field;
    public function __construct()
    {
    if (isset($this->field)) {}
    }
    }

    This reports Property Bug5337\Foo::$field is unused for isset, but something like $this->field ?? false would say only read.

By the way, there is a bug in coalesce too, reported in phpstan/phpstan#5971 (comment)
which

$this->field->prop ?? false

would not be recognized as read for property $field.

  1. Dynamic property in isset is not reporting undefined property

As explained in phpstan/phpstan#6809 (comment)

This commit cf0059e
would solve the first problem (leaving the bug phpstan/phpstan#5971 (comment)), and report undefined property for dynamic property for the second one (which makes it more strict, but maybe too strict for some usecases?).

@rajyan
Copy link
Contributor Author

rajyan commented Mar 25, 2022

I think first one should be fixed, but not sure about the second one.
As shown in
https://phpstan.org/r/56de90d5-b01b-42fd-ab31-912e2a3f6d79
dynamic property errors in level 0 anyway, so maybe in reporting in isset too is applicable.

@ondrejmirtes
Copy link
Member

Please see my thinking about this: phpstan/phpstan#6809 (comment)

I don't want BC breaks around this for analysis on PHP <= 8.1. We can do something more ambitious and strict for PHP 8.2 and objects that do not allow dynamic properties, but right now, there are too many test failures :)

@rajyan
Copy link
Contributor Author

rajyan commented Mar 28, 2022

@ondrejmirtes
Thank you for your comment. I'm working on now to solve the first problem (which is a bug I think), while leaving the dynamic property behavior as is. I'll mark this PR ready after the tests are passed!

@ondrejmirtes
Copy link
Member

Superseded by #1192

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants