Skip to content

Conversation

@jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 10, 2025

Description

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 (Squiz.ControlStructures.SwitchDeclaration: thorough review of the sniff code #1314) as a reminder to address these later.
    While addressing those, the current changes may be made more efficient too.

Includes tests.

Suggested changelog entry

Fixed: Squiz.ControlStructures.SwitchDeclaration: a number of the fixers would get into fixer conflicts with each other if the code under scan contained multiple statements on a line within a switch.
- To fix this, the sniff will now forbid - and auto-fix - multiple statements on one line for case/default and "case breaking" statements.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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.
@jrfnl jrfnl enabled auto-merge (squash) November 10, 2025 00:40
@jrfnl jrfnl merged commit 9397930 into 4.x Nov 10, 2025
67 checks passed
@jrfnl jrfnl deleted the feature/squiz-switchdeclaration-various-fixes branch November 10, 2025 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants