From b03f922e61d46eba2893a6d9109b6fb06bedbea7 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 16 Feb 2025 06:56:38 +0100 Subject: [PATCH 1/2] Squiz/MemberVarSpacing: rename test case file ... to allow for adding additional test case files for code containing parse errors. --- ...est.inc => MemberVarSpacingUnitTest.1.inc} | 0 ...d => MemberVarSpacingUnitTest.1.inc.fixed} | 0 .../WhiteSpace/MemberVarSpacingUnitTest.php | 94 ++++++++++--------- 3 files changed, 51 insertions(+), 43 deletions(-) rename src/Standards/Squiz/Tests/WhiteSpace/{MemberVarSpacingUnitTest.inc => MemberVarSpacingUnitTest.1.inc} (100%) rename src/Standards/Squiz/Tests/WhiteSpace/{MemberVarSpacingUnitTest.inc.fixed => MemberVarSpacingUnitTest.1.inc.fixed} (100%) diff --git a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.inc b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc similarity index 100% rename from src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.inc rename to src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc diff --git a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.inc.fixed b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed similarity index 100% rename from src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.inc.fixed rename to src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed diff --git a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php index 671ed51f85..ffea9c4c1d 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php +++ b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.php @@ -26,52 +26,60 @@ final class MemberVarSpacingUnitTest extends AbstractSniffUnitTest * The key of the array should represent the line number and the value * should represent the number of errors that should occur on that line. * + * @param string $testFile The name of the file being tested. + * * @return array */ - public function getErrorList() + public function getErrorList($testFile='') { - return [ - 4 => 1, - 7 => 1, - 20 => 1, - 30 => 1, - 35 => 1, - 44 => 1, - 50 => 1, - 73 => 1, - 86 => 1, - 106 => 1, - 115 => 1, - 150 => 1, - 160 => 1, - 165 => 1, - 177 => 1, - 186 => 1, - 200 => 1, - 209 => 1, - 211 => 1, - 224 => 1, - 229 => 1, - 241 => 1, - 246 => 1, - 252 => 1, - 254 => 1, - 261 => 1, - 275 => 1, - 276 => 1, - 288 => 1, - 292 => 1, - 333 => 1, - 342 => 1, - 346 => 1, - 353 => 1, - 357 => 1, - 366 => 1, - 377 => 1, - 378 => 1, - 379 => 1, - 380 => 1, - ]; + switch ($testFile) { + case 'MemberVarSpacingUnitTest.1.inc': + return [ + 4 => 1, + 7 => 1, + 20 => 1, + 30 => 1, + 35 => 1, + 44 => 1, + 50 => 1, + 73 => 1, + 86 => 1, + 106 => 1, + 115 => 1, + 150 => 1, + 160 => 1, + 165 => 1, + 177 => 1, + 186 => 1, + 200 => 1, + 209 => 1, + 211 => 1, + 224 => 1, + 229 => 1, + 241 => 1, + 246 => 1, + 252 => 1, + 254 => 1, + 261 => 1, + 275 => 1, + 276 => 1, + 288 => 1, + 292 => 1, + 333 => 1, + 342 => 1, + 346 => 1, + 353 => 1, + 357 => 1, + 366 => 1, + 377 => 1, + 378 => 1, + 379 => 1, + 380 => 1, + ]; + + default: + return []; + }//end switch }//end getErrorList() From 98f7cff049fbd7352956894a3448869a7ad8b783 Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sun, 16 Feb 2025 06:59:04 +0100 Subject: [PATCH 2/2] Squiz/MemberVarSpacing: bug fix / improve parse error handling The `Squiz.WhiteSpace.MemberVarSpacing` sniff checks the number of blank lines before a property declaration. To determine the number of blank lines before a property, it tries to find the start of the statement by: * First finding the first modifier keyword before the variable (to skip over a potential type declaration); * And then walking over the other modifiers until it finds the first one for the statement; * After that, it checks for potential docblocks and attributes and skips over those. Only after all that it checks the number of blank lines. The first step however leads to problems when, during live coding, a property would be declared without a modifier keyword. In that case, the sniff could walk back much further than it should, potentially misidentifying a modifier keyword for a function for the modifier keyword for the property. While this is an edge-case as it is not customary for properties to be declared _after_ functions, the sniff should still handle this situation correctly. Fixed by changing the logic of the sniff to stop searching earlier. Includes new test case files, both of which demonstrate the bug. Additionally, the test in the `1` file safeguards that the current behaviour of the sniff for multi-property declarations is not aversely affected by the fix. --- .../Sniffs/WhiteSpace/MemberVarSpacingSniff.php | 16 +++++++++++----- .../WhiteSpace/MemberVarSpacingUnitTest.1.inc | 6 ++++++ .../MemberVarSpacingUnitTest.1.inc.fixed | 7 +++++++ .../WhiteSpace/MemberVarSpacingUnitTest.2.inc | 10 ++++++++++ .../WhiteSpace/MemberVarSpacingUnitTest.3.inc | 14 ++++++++++++++ .../WhiteSpace/MemberVarSpacingUnitTest.php | 1 + 6 files changed, 49 insertions(+), 5 deletions(-) create mode 100644 src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.2.inc create mode 100644 src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.3.inc diff --git a/src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php b/src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php index 847b43b8c5..96cfd7aa95 100644 --- a/src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php +++ b/src/Standards/Squiz/Sniffs/WhiteSpace/MemberVarSpacingSniff.php @@ -45,23 +45,29 @@ protected function processMemberVar(File $phpcsFile, $stackPtr) { $tokens = $phpcsFile->getTokens(); + $stopPoints = [ + T_SEMICOLON, + T_OPEN_CURLY_BRACKET, + T_CLOSE_CURLY_BRACKET, + ]; + + $endOfPreviousStatement = $phpcsFile->findPrevious($stopPoints, ($stackPtr - 1), null, false, null, true); + $validPrefixes = Tokens::$methodPrefixes; $validPrefixes[] = T_VAR; $validPrefixes[] = T_READONLY; - $startOfStatement = $phpcsFile->findPrevious($validPrefixes, ($stackPtr - 1), null, false, null, true); + $startOfStatement = $phpcsFile->findNext($validPrefixes, ($endOfPreviousStatement + 1), $stackPtr, false, null, true); if ($startOfStatement === false) { + // Parse error/live coding - property without modifier. Bow out. return; } $endOfStatement = $phpcsFile->findNext(T_SEMICOLON, ($stackPtr + 1), null, false, null, true); - $ignore = $validPrefixes; - $ignore[T_WHITESPACE] = T_WHITESPACE; - $start = $startOfStatement; for ($prev = ($startOfStatement - 1); $prev >= 0; $prev--) { - if (isset($ignore[$tokens[$prev]['code']]) === true) { + if ($tokens[$prev]['code'] === T_WHITESPACE) { continue; } diff --git a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc index 8072ece66b..b2e9db886e 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc +++ b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc @@ -379,3 +379,9 @@ class SupportReadonlyProperties { readonly bool $readonlyB; readonly private bool $readonlyPrivate; } + +class NoPreambleMultilineDeclaration { + public + static + int $prop = 1; +} diff --git a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed index 12953c21a3..86835dfc6d 100644 --- a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed +++ b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.1.inc.fixed @@ -368,3 +368,10 @@ class SupportReadonlyProperties { readonly private bool $readonlyPrivate; } + +class NoPreambleMultilineDeclaration { + + public + static + int $prop = 1; +} diff --git a/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.2.inc b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.2.inc new file mode 100644 index 0000000000..2aab7c0591 --- /dev/null +++ b/src/Standards/Squiz/Tests/WhiteSpace/MemberVarSpacingUnitTest.2.inc @@ -0,0 +1,10 @@ + 1, 379 => 1, 380 => 1, + 384 => 1, ]; default: