Skip to content

Fix constant array comparison for optional keys#1324

Merged
ondrejmirtes merged 5 commits into
phpstan:1.7.xfrom
rvanvelzen:constant-array-supertype
May 18, 2022
Merged

Fix constant array comparison for optional keys#1324
ondrejmirtes merged 5 commits into
phpstan:1.7.xfrom
rvanvelzen:constant-array-supertype

Conversation

@rvanvelzen

Copy link
Copy Markdown
Contributor

Fixes phpstan/phpstan#7248. When optional keys are involved it's never certain whether to values are equal.

Comment thread tests/PHPStan/Analyser/data/constant-array-type-identical.php Outdated
Comment thread src/Analyser/MutatingScope.php Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can tell when it's always false even when optional keys are involved. This code doesn't seem right.

if I have array{a?: int} and array{b? int}, it can be true because both arrays can be empty.

But if I have array{c, int, d?: int}, array{e: int, f?: int}, we can be sure it's always false.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That is already handled by the supertype check in resolveIdenticalType. I've added these as test cases.

@rvanvelzen rvanvelzen marked this pull request as draft May 17, 2022 11:46
@ondrejmirtes

Copy link
Copy Markdown
Member

I can merge this if it's ready :)

@rvanvelzen rvanvelzen force-pushed the constant-array-supertype branch from 4f1079c to c570bc7 Compare May 18, 2022 15:03
@rvanvelzen

Copy link
Copy Markdown
Contributor Author

It didn't work for the == case, but it should cover all cases now :)

@rvanvelzen rvanvelzen marked this pull request as ready for review May 18, 2022 15:04
@ondrejmirtes ondrejmirtes merged commit 40ae66b into phpstan:1.7.x May 18, 2022
@ondrejmirtes

Copy link
Copy Markdown
Member

Thank you!

@rvanvelzen rvanvelzen deleted the constant-array-supertype branch June 13, 2022 06:29
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.

False positive same comparison

2 participants