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

Add new ArrayUnpackingRule #856

Merged

Conversation

canvural
Copy link
Contributor

Before PHP 8.1, unpacking an array that contains string keys results in fatal error: https://3v4l.org/A4gqa

This PR adds a rule that can check this.

Not sure if this rule would be too annoying or not. All the mixed or array-key keyed arrays that being unpacked would error now. Which sounds in line with the runtime though.

@ondrejmirtes
Copy link
Member

Hi, I think we have to acknowledge multiple things here:

  1. Unpacking syntax can be used in literal arrays and also in call arguments.
  2. We should also check for other invalid unpacks like this: https://3v4l.org/0OJA5
  3. New rules can only be added for bleeding edge: https://phpstan.org/user-guide/backward-compatibility-promise

So I suggest this:

  1. Add some logic to https://github.com/phpstan/phpstan-src/blob/master/src/Rules/FunctionCallParametersCheck.php that checks this for all the rules that call this class - it covers new, function calls, instance method calls, static method calls, callables calls...
  2. Add a new rule called UnpackingRule that checks all possible invalid unpacks.

If you're unsure about bleeding edge, you can code the rule as normal and I'll configure it in a later commit :)

Thank you!

@ondrejmirtes
Copy link
Member

@canvural If you ever feel like finishing this, I'm looking forward to it! :)

@canvural
Copy link
Contributor Author

canvural commented Feb 8, 2022

@canvural If you ever feel like finishing this, I'm looking forward to it! :)

Yes, in my to-do list. I'll definitely do something about this when I have the time. Hope it's ok if this stays in draft.

@canvural canvural force-pushed the string-keyed-array-unpacking-rule branch from 8adfab3 to a582a5d Compare March 28, 2022 10:52
@canvural
Copy link
Contributor Author

@ondrejmirtes It looks like there is already an error reported for all the things FunctionCallParametersCheck does: https://phpstan.org/r/1d920739-b94e-421d-8afe-9e86e509f196 (Named arguments are supported only on PHP 8.0 and later.) I think that's enough.

Also the invalid usage (strpos(...1)) is reported correctly.

So the current rule in this PR seems sufficient?

@ondrejmirtes
Copy link
Member

Named arguments are supported only on PHP 8.0 and later.

Yeah, but on PHP 8.0 PHPStan should complain about array with string keys being unpacked. Am I right?

@@ -3,6 +3,7 @@ includes:

rules:
- PHPStan\Rules\Arrays\ArrayDestructuringRule
- PHPStan\Rules\Arrays\ArrayUnpackingRule
Copy link
Member

Choose a reason for hiding this comment

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

Can you please adapt this similarly to this commit? 7627b52

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@ondrejmirtes
Copy link
Member

 PHPStan should complain about array with string keys being unpacked. Am I right?

Oh right, that's what this PR does. I need coffee.

@canvural
Copy link
Contributor Author

canvural commented Mar 28, 2022

Hmm. Looks like no: https://3v4l.org/7lTWP Only for PHP < 8

@canvural canvural force-pushed the string-keyed-array-unpacking-rule branch from a582a5d to 9edb368 Compare March 28, 2022 12:10
@canvural canvural marked this pull request as ready for review March 28, 2022 12:10
@canvural canvural force-pushed the string-keyed-array-unpacking-rule branch from 9edb368 to 0b2cc87 Compare March 28, 2022 12:23
@ondrejmirtes ondrejmirtes changed the base branch from 1.5.x to 1.6.x March 28, 2022 15:52
@ondrejmirtes ondrejmirtes force-pushed the string-keyed-array-unpacking-rule branch from 0b2cc87 to 4373952 Compare March 28, 2022 15:57
@ondrejmirtes
Copy link
Member

Did a couple more adjustments: https://github.com/phpstan/phpstan-src/pull/856/commits

@ondrejmirtes ondrejmirtes merged commit 0f5b4bf into phpstan:1.6.x Mar 28, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@staabm
Copy link
Contributor

staabm commented Mar 28, 2022

not sure why yet, but the test from this PR errors on macOS with php 8.1.2

There was 1 error:

1) PHPStan\Rules\Arrays\ArrayUnpackingRuleTest::testRuleOnPHP81
Error: Typed property PHPStan\Rules\Arrays\ArrayUnpackingRuleTest::$checkUnions must not be accessed before initialization

/Users/staabm/workspace/phpstan-src/tests/PHPStan/Rules/Arrays/ArrayUnpackingRuleTest.php:23
/Users/staabm/workspace/phpstan-src/src/Testing/RuleTestCase.php:47
/Users/staabm/workspace/phpstan-src/src/Testing/RuleTestCase.php:95
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Rules/Arrays/ArrayUnpackingRuleTest.php:91

FAILURES!                                                
Tests: 12850, Assertions: 83007, Errors: 1, Skipped: 142.
make: *** [tests] Error 2

php -v
PHP 8.1.2 (cli) (built: Jan 21 2022 04:34:05) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.1.2, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.2, Copyright (c), by Zend Technologies

@ondrejmirtes
Copy link
Member

Fixed: e131e35

@ondrejmirtes
Copy link
Member

Turns out there was a feature request for this :) phpstan/phpstan#5764

@canvural canvural deleted the string-keyed-array-unpacking-rule branch June 2, 2022 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants