Skip to content

Commit 9397930

Browse files
jrfnljrfnl
authored
Squiz/SwitchDeclaration: bug fix - fixer conflict for single line code (#1315)
While working on something else, I realized that the `Squiz.ControlStructures.SwitchDeclaration` sniff did not take into account that the "code under scan" could potentially be written to have various "parts" of a switch case/default structure on a single line. This meant that, in that case, the sniff would have a fixer conflict with itself. It would basically keep adding indentation whitespace for case/default/breaking statements not having the right indentation, but as it never added a new line _before_ the indentation, the indentation would still be incorrect in the next loop. Fixed now by adding a number of extra checks to the sniff: * `'ContentBefore' .$type` - which checks if there is anything but indentation whitespace, before a `case` or `default` keyword. * `'ContentBeforeBreak'` - which checks if there is anything but indentation whitespace, before a `break/return` (etc) keyword. * `'ContentAfter' .$type` - which checks if there is anything but a trailing comment, after a `case` or `default` statement. The sniff also contains a couple of checks which verify the number of blank lines above/below certain statements. Those also didn't take into account that the count of the "lines between" may be zero, i.e. the code "after" being on the same line as the starting point for the check. Those situations should now largely be fixed with the new lines being added via the `Content[Before|After]*` error codes. For all other cases, the fixer code for the "number of blank lines" checks has been updated to handle trailing comments better, not leave trailing whitespace behind and not get into a fixer conflict with the new checks. Notes: * All fixers added/updated have been set up to prevent leaving trailing whitespace behind. * The "fixed" code for one pre-existing test with a trailing comment has changed. In my opinion, the new "fixed" version is more correct, because: 1. This sniff is not about disallowing trailing comments, so this sniff should not act as if it has an opinion on that. 2. When moving (trailing) comments would be allowed, it could also result in tooling annotations being moved, which changes the meaning of the annotation in most cases. * While working on this, I noticed various other fixes which are needed for this sniff. I've opened a ticket (1314) as a reminder to address these later. While addressing those, the current changes may be made more efficient too. Includes tests.
1 parent 2c13a90 commit 9397930

File tree

4 files changed

+210
-37
lines changed

4 files changed

+210
-37
lines changed

‎src/Standards/Squiz/Sniffs/ControlStructures/SwitchDeclarationSniff.php‎

Lines changed: 113 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -91,22 +91,6 @@ public function process(File $phpcsFile, int $stackPtr)
9191
}
9292
}
9393

94-
if ($tokens[$nextCase]['column'] !== $caseAlignment) {
95-
$error = strtoupper($type) . ' keyword must be indented ' . $this->indent . ' spaces from SWITCH keyword';
96-
$fix = $phpcsFile->addFixableError($error, $nextCase, $type . 'Indent');
97-
98-
if ($fix === true) {
99-
$padding = str_repeat(' ', ($caseAlignment - 1));
100-
if ($tokens[$nextCase]['column'] === 1
101-
|| $tokens[($nextCase - 1)]['code'] !== T_WHITESPACE
102-
) {
103-
$phpcsFile->fixer->addContentBefore($nextCase, $padding);
104-
} else {
105-
$phpcsFile->fixer->replaceToken(($nextCase - 1), $padding);
106-
}
107-
}
108-
}
109-
11094
if ($type === 'Case'
11195
&& ($tokens[($nextCase + 1)]['type'] !== 'T_WHITESPACE'
11296
|| $tokens[($nextCase + 1)]['content'] !== ' ')
@@ -122,6 +106,35 @@ public function process(File $phpcsFile, int $stackPtr)
122106
}
123107
}
124108

109+
$beforeCase = $phpcsFile->findPrevious(T_WHITESPACE, ($nextCase - 1), null, true);
110+
if ($tokens[$beforeCase]['line'] === $tokens[$nextCase]['line']) {
111+
$error = '%s statement must be on a line by itself. Found content before';
112+
$fix = $phpcsFile->addFixableError($error, $nextCase, 'ContentBefore' . $type, [strtoupper($type)]);
113+
114+
if ($fix === true) {
115+
$padding = str_repeat(' ', ($caseAlignment - 1));
116+
if ($tokens[($beforeCase + 1)]['code'] === T_WHITESPACE) {
117+
$phpcsFile->fixer->replaceToken(($beforeCase + 1), $phpcsFile->eolChar . $padding);
118+
} else {
119+
$phpcsFile->fixer->addContent($beforeCase, $phpcsFile->eolChar . $padding);
120+
}
121+
}
122+
} elseif ($tokens[$nextCase]['column'] !== $caseAlignment) {
123+
$error = strtoupper($type) . ' keyword must be indented ' . $this->indent . ' spaces from SWITCH keyword';
124+
$fix = $phpcsFile->addFixableError($error, $nextCase, $type . 'Indent');
125+
126+
if ($fix === true) {
127+
$padding = str_repeat(' ', ($caseAlignment - 1));
128+
if ($tokens[$nextCase]['column'] === 1
129+
|| $tokens[($nextCase - 1)]['code'] !== T_WHITESPACE
130+
) {
131+
$phpcsFile->fixer->addContentBefore($nextCase, $padding);
132+
} else {
133+
$phpcsFile->fixer->replaceToken(($nextCase - 1), $padding);
134+
}
135+
}
136+
}
137+
125138
if (isset($tokens[$nextCase]['scope_opener']) === false) {
126139
// Parse error or live coding.
127140
continue;
@@ -148,7 +161,20 @@ public function process(File $phpcsFile, int $stackPtr)
148161
// Only need to check a couple of things once, even if the
149162
// break is shared between multiple case statements, or even
150163
// the default case.
151-
if ($tokens[$nextBreak]['column'] !== $caseAlignment) {
164+
$beforeBreak = $phpcsFile->findPrevious(T_WHITESPACE, ($nextBreak - 1), null, true);
165+
if ($tokens[$beforeBreak]['line'] === $tokens[$nextBreak]['line']) {
166+
$error = 'Case breaking statement must be on a line by itself. Found content before';
167+
$fix = $phpcsFile->addFixableError($error, $nextBreak, 'ContentBeforeBreak');
168+
169+
if ($fix === true) {
170+
$padding = str_repeat(' ', ($caseAlignment - 1));
171+
if ($tokens[($beforeBreak + 1)]['code'] === T_WHITESPACE) {
172+
$phpcsFile->fixer->replaceToken(($beforeBreak + 1), $phpcsFile->eolChar . $padding);
173+
} else {
174+
$phpcsFile->fixer->addContent($beforeBreak, $phpcsFile->eolChar . $padding);
175+
}
176+
}
177+
} elseif ($tokens[$nextBreak]['column'] !== $caseAlignment) {
152178
$error = 'Case breaking statement must be indented ' . $this->indent . ' spaces from SWITCH keyword';
153179
$fix = $phpcsFile->addFixableError($error, $nextBreak, 'BreakIndent');
154180

@@ -164,63 +190,115 @@ public function process(File $phpcsFile, int $stackPtr)
164190
}
165191
}
166192

167-
$prev = $phpcsFile->findPrevious(T_WHITESPACE, ($nextBreak - 1), $stackPtr, true);
168-
if ($tokens[$prev]['line'] !== ($tokens[$nextBreak]['line'] - 1)) {
193+
if (($tokens[$nextBreak]['line'] - $tokens[$beforeBreak]['line']) > 1) {
169194
$error = 'Blank lines are not allowed before case breaking statements';
170195
$phpcsFile->addError($error, $nextBreak, 'SpacingBeforeBreak');
171196
}
172197

198+
// Figure out the relevant "next" line.
199+
// Either the line containing the first non-empty content after the "break" statement
200+
// (which may be the current line), or a line containing a comment,
201+
// as long as it is not a trailing comment on the case statement line.
173202
$nextLine = $tokens[$tokens[$stackPtr]['scope_closer']]['line'];
174203
$semicolon = $phpcsFile->findEndOfStatement($nextBreak);
175-
for ($i = ($semicolon + 1); $i < $tokens[$stackPtr]['scope_closer']; $i++) {
176-
if ($tokens[$i]['type'] !== 'T_WHITESPACE') {
177-
$nextLine = $tokens[$i]['line'];
204+
for ($nextRelevant = ($semicolon + 1); $nextRelevant < $tokens[$stackPtr]['scope_closer']; $nextRelevant++) {
205+
if (isset(Tokens::EMPTY_TOKENS[$tokens[$nextRelevant]['code']]) === false) {
206+
$nextLine = $tokens[$nextRelevant]['line'];
207+
break;
208+
}
209+
210+
if ($tokens[$nextRelevant]['code'] !== T_WHITESPACE
211+
&& $tokens[$nextBreak]['line'] !== $tokens[$nextRelevant]['line']
212+
) {
213+
$nextLine = $tokens[$nextRelevant]['line'];
178214
break;
179215
}
180216
}
181217

182218
if ($type === 'Case') {
183219
// Ensure the BREAK statement is followed by
184220
// a single blank line, or the end switch brace.
185-
if ($nextLine !== ($tokens[$semicolon]['line'] + 2) && $i !== $tokens[$stackPtr]['scope_closer']) {
221+
if ($nextLine !== ($tokens[$semicolon]['line'] + 2)
222+
&& $nextRelevant !== $tokens[$stackPtr]['scope_closer']
223+
) {
186224
$error = 'Case breaking statements must be followed by a single blank line';
187225
$fix = $phpcsFile->addFixableError($error, $nextBreak, 'SpacingAfterBreak');
188226
if ($fix === true) {
227+
$padding = str_repeat(' ', ($caseAlignment - 1));
228+
189229
$phpcsFile->fixer->beginChangeset();
190-
for ($i = ($semicolon + 1); $i <= $tokens[$stackPtr]['scope_closer']; $i++) {
191-
if ($tokens[$i]['line'] === $nextLine) {
192-
$phpcsFile->fixer->addNewlineBefore($i);
193-
break;
230+
if ($nextLine === $tokens[$semicolon]['line']) {
231+
// Missing new line.
232+
$replacement = $phpcsFile->eolChar . $phpcsFile->eolChar . $padding;
233+
if ($tokens[($nextRelevant - 1)]['code'] === T_WHITESPACE) {
234+
$phpcsFile->fixer->replaceToken(($nextRelevant - 1), $replacement);
235+
} else {
236+
$phpcsFile->fixer->addContentBefore($nextRelevant, $replacement);
194237
}
195-
196-
if ($tokens[$i]['line'] === $tokens[$semicolon]['line']) {
197-
continue;
238+
} else {
239+
// There is/are new line(s), just not the right number of them.
240+
for ($i = ($semicolon + 1); $i < $nextRelevant; $i++) {
241+
if ($tokens[$semicolon]['line'] === $tokens[$i]['line']) {
242+
if ($tokens[$i]['line'] !== $tokens[($i + 1)]['line']) {
243+
// Add extra new line at end of break line.
244+
$phpcsFile->fixer->addNewline($i);
245+
}
246+
247+
continue;
248+
}
249+
250+
if ($tokens[$i]['line'] === $tokens[$nextRelevant]['line']) {
251+
// Don't remove indentation on the line of the "next" content.
252+
break;
253+
}
254+
255+
// In all other cases, remove whitespace.
256+
$phpcsFile->fixer->replaceToken($i, '');
198257
}
199-
200-
$phpcsFile->fixer->replaceToken($i, '');
201258
}
202259

203260
$phpcsFile->fixer->endChangeset();
204261
}
205262
}
206263
} else {
207264
// Ensure the BREAK statement is not followed by a blank line.
208-
if ($nextLine !== ($tokens[$semicolon]['line'] + 1)) {
265+
if (($nextLine - $tokens[$semicolon]['line']) > 1) {
209266
$error = 'Blank lines are not allowed after the DEFAULT case\'s breaking statement';
210267
$phpcsFile->addError($error, $nextBreak, 'SpacingAfterDefaultBreak');
211268
}
212269
}
213270

271+
// Figure out the relevant "next" line.
272+
// Either the line containing the first non-empty content (which may be the current line),
273+
// or a line containing a comment, as long as it is not a trailing comment on the case statement line.
214274
$caseLine = $tokens[$nextCase]['line'];
215275
$nextLine = $tokens[$nextBreak]['line'];
216276
for ($i = ($opener + 1); $i < $nextBreak; $i++) {
217-
if ($tokens[$i]['type'] !== 'T_WHITESPACE') {
277+
if (isset(Tokens::EMPTY_TOKENS[$tokens[$i]['code']]) === false) {
278+
$nextLine = $tokens[$i]['line'];
279+
break;
280+
}
281+
282+
if ($tokens[$i]['code'] !== T_WHITESPACE
283+
&& $tokens[$opener]['line'] !== $tokens[$i]['line']
284+
) {
218285
$nextLine = $tokens[$i]['line'];
219286
break;
220287
}
221288
}
222289

223-
if ($nextLine !== ($caseLine + 1)) {
290+
if ($nextLine === $caseLine) {
291+
$error = '%s statement must be on a line by itself. Found content after';
292+
$fix = $phpcsFile->addFixableError($error, $opener, 'ContentAfter' . $type, [strtoupper($type)]);
293+
if ($fix === true) {
294+
$padding = str_repeat(' ', ($caseAlignment - 1 + $this->indent));
295+
if ($tokens[($i - 1)]['code'] === T_WHITESPACE) {
296+
$phpcsFile->fixer->replaceToken(($i - 1), $phpcsFile->eolChar . $padding);
297+
} else {
298+
$phpcsFile->fixer->addContentBefore($i, $phpcsFile->eolChar . $padding);
299+
}
300+
}
301+
} elseif (($nextLine - $caseLine) > 1) {
224302
$error = 'Blank lines are not allowed after ' . strtoupper($type) . ' statements';
225303
$phpcsFile->addError($error, $nextCase, 'SpacingAfter' . $type);
226304
}

‎src/Standards/Squiz/Tests/ControlStructures/SwitchDeclarationUnitTest.inc‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,3 +344,39 @@ switch ( $a ) {
344344

345345
jumpOut:
346346
doSomething();
347+
348+
// Bug: the below should not throw "Blank lines are not allowed before/after..." errors (as there are no blank lines).
349+
// Instead the sniff should flag case/default/break statements not being on their own line.
350+
switch ($condition) {case 'string': $varStr = 'one line'; break; case 'bool':$varStr = 'one line';return $foo;default: $varStr = 'test'; break;}
351+
352+
switch ($condition) {
353+
case 'string': /*test comment handling when adding new lines*/$varStr = 'one line';/*test comment handling when adding new lines*/ break;
354+
default:/*test comment handling when adding new lines*/ $varStr = 'test'; /*test comment handling when adding new lines*/break;
355+
}
356+
357+
switch ($condition) {
358+
case 'too many blank lines':
359+
360+
361+
// This comment should be used for the "blank line after case" determination.
362+
$varStr = 'test';
363+
364+
break;
365+
366+
367+
368+
// Comment to use for measuring blank lines between the above break and here.
369+
case 'this is fine': // This comment should be ignored and $varStr should be used instead.
370+
371+
372+
$varStr = 'test';
373+
374+
375+
break; // Trailing comment to ignore.
376+
// This comment should be used for the "blank line after break" determination.
377+
default: // This comment should be ignored and $varStr should be used instead.
378+
379+
$varStr = 'test';
380+
break;
381+
382+
}

‎src/Standards/Squiz/Tests/ControlStructures/SwitchDeclarationUnitTest.inc.fixed‎

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -288,9 +288,8 @@ switch ($something) {
288288

289289
switch ($foo) {
290290
case '1':
291-
return;
291+
return; // comment
292292

293-
// comment
294293
break;
295294

296295
}
@@ -354,3 +353,54 @@ switch ( $a ) {
354353

355354
jumpOut:
356355
doSomething();
356+
357+
// Bug: the below should not throw "Blank lines are not allowed before/after..." errors (as there are no blank lines).
358+
// Instead the sniff should flag case/default/break statements not being on their own line.
359+
switch ($condition) {
360+
case 'string':
361+
$varStr = 'one line';
362+
break;
363+
364+
case 'bool':
365+
$varStr = 'one line';
366+
return $foo;
367+
368+
default:
369+
$varStr = 'test';
370+
break;}
371+
372+
switch ($condition) {
373+
case 'string': /*test comment handling when adding new lines*/
374+
$varStr = 'one line';/*test comment handling when adding new lines*/
375+
break;
376+
377+
default:/*test comment handling when adding new lines*/
378+
$varStr = 'test'; /*test comment handling when adding new lines*/
379+
break;
380+
}
381+
382+
switch ($condition) {
383+
case 'too many blank lines':
384+
385+
386+
// This comment should be used for the "blank line after case" determination.
387+
$varStr = 'test';
388+
389+
break;
390+
391+
// Comment to use for measuring blank lines between the above break and here.
392+
case 'this is fine': // This comment should be ignored and $varStr should be used instead.
393+
394+
395+
$varStr = 'test';
396+
397+
398+
break; // Trailing comment to ignore.
399+
400+
// This comment should be used for the "blank line after break" determination.
401+
default: // This comment should be ignored and $varStr should be used instead.
402+
403+
$varStr = 'test';
404+
break;
405+
406+
}

‎src/Standards/Squiz/Tests/ControlStructures/SwitchDeclarationUnitTest.php‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,15 @@ public function getErrorList()
7979
330 => 1,
8080
339 => 2,
8181
342 => 1,
82+
350 => 12,
83+
353 => 4,
84+
354 => 3,
85+
358 => 1,
86+
364 => 2,
87+
369 => 1,
88+
375 => 2,
89+
377 => 1,
90+
380 => 1,
8291
];
8392
}
8493

0 commit comments

Comments
 (0)