From 856219c8b4c512f36224019bdd97b3488ef26030 Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Tue, 25 Jun 2024 15:32:54 -0300 Subject: [PATCH 1/2] Squiz/ArrayDeclaration: improves test coverage This commit adds tests that were previously missing. There are still some uncovered lines in this sniff, those lines might be unreachable, but I did not investigate this at this time. There is also potentially some more relevant tests that could be added for lines that are already covered. This is also not addressed in this commit. --- .../Tests/Arrays/ArrayDeclarationUnitTest.1.inc | 13 ++++++++++++- .../Arrays/ArrayDeclarationUnitTest.1.inc.fixed | 10 ++++++++++ .../Tests/Arrays/ArrayDeclarationUnitTest.2.inc | 11 +++++++++++ .../Arrays/ArrayDeclarationUnitTest.2.inc.fixed | 10 ++++++++++ .../Tests/Arrays/ArrayDeclarationUnitTest.3.inc | 7 +++++++ .../Squiz/Tests/Arrays/ArrayDeclarationUnitTest.php | 4 ++++ 6 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.3.inc diff --git a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.1.inc b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.1.inc index cdbfff6c33..6a25ab9072 100644 --- a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.1.inc @@ -19,7 +19,7 @@ class TestClass 'height' => '', ); - private $_bad = Array( + private $_bad = ARRAY( 'width' => '', 'height' => '' ); @@ -547,3 +547,14 @@ $x = array( 1, static::helloWorld(), $class instanceof static, 2, ); + +$noSpaceBeforeDoubleArrow = array( + 'width'=> '', + 'height' => '', + ); + +$newlineAfterDoubleArrow = array( + 'width' => + '', + 'height' => '', + ); diff --git a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.1.inc.fixed b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.1.inc.fixed index 6f8fe216a3..048f898c8d 100644 --- a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.1.inc.fixed +++ b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.1.inc.fixed @@ -586,3 +586,13 @@ $x = array( $class instanceof static, 2, ); + +$noSpaceBeforeDoubleArrow = array( + 'width' => '', + 'height' => '', + ); + +$newlineAfterDoubleArrow = array( + 'width' => '', + 'height' => '', + ); diff --git a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc index 90b026f023..f5f4c84234 100644 --- a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc +++ b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc @@ -536,3 +536,14 @@ $x = [ 1, static::helloWorld(), $class instanceof static, 2, ]; + +$noSpaceBeforeDoubleArrow = [ + 'width'=> '', + 'height' => '', + ]; + +$newlineAfterDoubleArrow = [ + 'width' => + '', + 'height' => '', + ]; diff --git a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc.fixed b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc.fixed index 533be16a65..6ef97d6434 100644 --- a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc.fixed +++ b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc.fixed @@ -573,3 +573,13 @@ $x = [ $class instanceof static, 2, ]; + +$noSpaceBeforeDoubleArrow = [ + 'width' => '', + 'height' => '', + ]; + +$newlineAfterDoubleArrow = [ + 'width' => '', + 'height' => '', + ]; diff --git a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.3.inc b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.3.inc new file mode 100644 index 0000000000..beb5ae1aec --- /dev/null +++ b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.3.inc @@ -0,0 +1,7 @@ + 1, 540 => 1, 547 => 2, + 552 => 1, + 557 => 1, ]; case 'ArrayDeclarationUnitTest.2.inc': return [ @@ -229,6 +231,8 @@ public function getErrorList($testFile='') 526 => 1, 529 => 1, 536 => 2, + 541 => 1, + 546 => 1, ]; default: return []; From 8e0074ace15f690f20b46c37be3ef9dfb94f4008 Mon Sep 17 00:00:00 2001 From: Rodrigo Primo Date: Wed, 20 Nov 2024 12:05:58 -0300 Subject: [PATCH 2/2] Squiz/ArrayDeclaration: improve handling of short lists inside a foreach This commit improves how the `Squiz.Arrays.ArrayDeclaration` sniff handles short lists inside a foreach and fixes a false positive. See https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/527 --- .../Sniffs/Arrays/ArrayDeclarationSniff.php | 20 +++++++++++++++++++ .../Arrays/ArrayDeclarationUnitTest.2.inc | 6 ++++++ .../ArrayDeclarationUnitTest.2.inc.fixed | 6 ++++++ .../Arrays/ArrayDeclarationUnitTest.4.inc | 8 ++++++++ .../ArrayDeclarationUnitTest.4.inc.fixed | 8 ++++++++ .../Tests/Arrays/ArrayDeclarationUnitTest.php | 3 +++ 6 files changed, 51 insertions(+) create mode 100644 src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.4.inc create mode 100644 src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.4.inc.fixed diff --git a/src/Standards/Squiz/Sniffs/Arrays/ArrayDeclarationSniff.php b/src/Standards/Squiz/Sniffs/Arrays/ArrayDeclarationSniff.php index 89cd7bd57f..efad97e4b3 100644 --- a/src/Standards/Squiz/Sniffs/Arrays/ArrayDeclarationSniff.php +++ b/src/Standards/Squiz/Sniffs/Arrays/ArrayDeclarationSniff.php @@ -45,6 +45,26 @@ public function process(File $phpcsFile, $stackPtr) { $tokens = $phpcsFile->getTokens(); + // Prevent acting on short lists inside a foreach (see + // https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/527). + if ($tokens[$stackPtr]['code'] === T_OPEN_SHORT_ARRAY + && isset($tokens[$stackPtr]['nested_parenthesis']) === true + ) { + $nestedParens = $tokens[$stackPtr]['nested_parenthesis']; + $lastParenthesisCloser = end($nestedParens); + $lastParenthesisOpener = key($nestedParens); + + if (isset($tokens[$lastParenthesisCloser]['parenthesis_owner']) === true + && $tokens[$tokens[$lastParenthesisCloser]['parenthesis_owner']]['code'] === T_FOREACH + ) { + $asKeyword = $phpcsFile->findNext(T_AS, ($lastParenthesisOpener + 1), $lastParenthesisCloser); + + if ($asKeyword !== false && $asKeyword < $stackPtr) { + return; + } + } + } + if ($tokens[$stackPtr]['code'] === T_ARRAY) { $phpcsFile->recordMetric($stackPtr, 'Short array syntax used', 'no'); diff --git a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc index f5f4c84234..415042d89e 100644 --- a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc +++ b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc @@ -547,3 +547,9 @@ $newlineAfterDoubleArrow = [ '', 'height' => '', ]; + +// Sniff should ignore short lists when inside a foreach. +// https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/527 +foreach ($data as [, , $value]) {} +foreach ($array as $k => [$v1, , $v3]) {} +foreach ([$a ,$b] as $c) {} // Not a short list. Sniff should handle it. diff --git a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc.fixed b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc.fixed index 6ef97d6434..d835064ba1 100644 --- a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc.fixed +++ b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.2.inc.fixed @@ -583,3 +583,9 @@ $newlineAfterDoubleArrow = [ 'width' => '', 'height' => '', ]; + +// Sniff should ignore short lists when inside a foreach. +// https://github.com/PHPCSStandards/PHP_CodeSniffer/issues/527 +foreach ($data as [, , $value]) {} +foreach ($array as $k => [$v1, , $v3]) {} +foreach ([$a, $b] as $c) {} // Not a short list. Sniff should handle it. diff --git a/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.4.inc b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.4.inc new file mode 100644 index 0000000000..9d87a6e7fe --- /dev/null +++ b/src/Standards/Squiz/Tests/Arrays/ArrayDeclarationUnitTest.4.inc @@ -0,0 +1,8 @@ + 2, 541 => 1, 546 => 1, + 555 => 2, ]; + case 'ArrayDeclarationUnitTest.4.inc': + return [8 => 1]; default: return []; }//end switch