Problem with NormalizedArrays.Arrays.CommaAfterLast in some cases

[stronk7]

Bug Description

The NormalizedArrays.Arrays.CommaAfterLast does its job pretty well. But there is a case that is hitting us a lot (because of our own coding standard explicitly allowing it), and that I think that can be hitting others out there too.

Right now we support all these arrays as valid:

$a = [1, 2, 3, 4]; // Normal mono-line.
$a = [
    1,
    2,
    3,
    4,
]; // Normal multi-line
$a = [1, 2,
    3, 4]; // Multi-multi line with same line closer (to call it some way).
$a = [
    1, 2,
    3, 4,
]; // Multi-multi line with closer apart (also to call it some way).

And the Sniff is working ok for all the cases but the 3rd one. And it seems logic not to require a comma there, because, if there are new items to be added to the array later, the line is going to change, yes or yes (the closer will need to be moved.

Given the following reproduction Scenario

Ensure that "Multi-multi line with same line closer " cases don't require a comma.

The issue happens when running this command:

vendor/bin/phpcs --standard=NormalizedArrays --sniffs=NormalizedArrays.Arrays.CommaAfterLast test.php

... over a file containing this code:

<?php

$a = [1, 2,
    3, 4];

$a = [1 => 1, 2 => 2,
    3 => 3, 4 => 4];

$a = [1 => [1 => 1,
    2 => 2,
    3 => 3,
    4 => 4,
], 5 => 5];

$a = [1 => [1 => 1,
    2 => 2,
    3 => 3,
    4 => 4],
5 => 5];

I'd expect the following behaviour

No errors are reported.

Instead this happened

Five errors are reported.

-------------------------------------------------------------------------------------------
FOUND 5 ERRORS AFFECTING 5 LINES
-------------------------------------------------------------------------------------------
  4 | ERROR | [x] There should be a comma after the last array item in a multi-line array.
  7 | ERROR | [x] There should be a comma after the last array item in a multi-line array.
 13 | ERROR | [x] There should be a comma after the last array item in a multi-line array.
 18 | ERROR | [x] There should be a comma after the last array item in a multi-line array.
 19 | ERROR | [x] There should be a comma after the last array item in a multi-line array.
-------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 5 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------

Environment

Environment	Answer
PHP version	Any
PHP_CodeSniffer version	3.7.2
PHPCSExtra version	1.1.1
PHPCSUtils version	1.0.8
Install type	Normal composer, used by our standard

Additional Context (optional)

This was originally reported for us @ moodlehq/moodle-cs#76
And this change seems to fix the problem:

diff --git a/NormalizedArrays/Sniffs/Arrays/CommaAfterLastSniff.php b/NormalizedArrays/Sniffs/Arrays/CommaAfterLastSniff.php
index d03d1ff..6472e5c 100644
--- a/NormalizedArrays/Sniffs/Arrays/CommaAfterLastSniff.php
+++ b/NormalizedArrays/Sniffs/Arrays/CommaAfterLastSniff.php
@@ -159,6 +159,12 @@ final class CommaAfterLastSniff implements Sniff
                     return;
                 }
 
+                // If the line of the last non-empty token is the same as the closer, we're not
+                // going to require a comma. TODO: Put this under some Sniff configuration setting.
+                if ($tokens[$lastNonEmpty]['line'] === $tokens[$closer]['line']) {
+                    return;
+                }
+
                 $error     = 'There should be a comma after the last array item in a %s array.';
                 $errorCode = 'Missing' . $errorCode;
                 $data      = [$phrase];

Note the TODO that I've put there. IMO there are 4 alternatives:

• We make that the default behaviour. After all, it doesn't make sense to force commas when the closer is in the same line.
• We make it a new $validValues, say: enforce_but_when_sameline_closer.
• We make it a new, apart sniff setting: enforce_skipped_when_sameline_closer.
• The change is not acceptable (so we'll have to duplicate the sniff into our standard and apply the patch there.

If there is any insight/preference about any of the 1-3 alternatives above... I'm happy preparing a PR with tests and so on...

Ciao :-)

Tested Against develop branch?

• I have verified the issue still exists in the develop branch of PHPCSExtra.

[stronk7]

BTW, note that the first alternative above:

• We make that the default behaviour. After all, it doesn't make sense to force commas when the closer is in the same line.

In practice, render inexistent any difference between mono-line and multi-line, because the former always has the closer in the same/unique (mono) line.

[fredden]

Making this default behaviour seems to make sense to me. If people want to assert that the closer of a multi-line array is on a different line, one can use another sniff like Squiz.Arrays.ArrayDeclaration.CloseBraceNewLine.

[jrfnl]

@stronk7 Thanks for bringing this to my attention.

Changing the default behaviour would be a breaking change, so I'm not keen on that, but I'm missing another alternative solution:

• Introduce new error codes for this specific situation, something along the lines of MissingMultilineCloserSameLine, FoundMultilineCloserSameLine.

Having the separate error codes would allow for excluding the applicable error code, while retaining the default behaviour as is.

The extra error code could also be considered a breaking change, but one with a smaller impact than changing the default behaviour.

What do you think ?

[stronk7]

Making this default behaviour seems to make sense to me. If people want to assert that the closer of a multi-line array is on a different line, one can use another sniff like Squiz.Arrays.ArrayDeclaration.CloseBraceNewLine.

Yeah, or, also, PHPCSExtra's NormalizedArrays.Arrays.ArrayBraceSpacing available here, that is about enforcing newlines and so on (obviously we don't use that one, because we allow the cases commented above.

[stronk7]

• Introduce new error codes for this specific situation, something along the lines of MissingMultilineCloserSameLine, FoundMultilineCloserSameLine.

Aha, that's a good 5th alternative, had not thought about it! For sure it will be enough for us to be able to ignore those error codes.

[jrfnl]

• Introduce new error codes for this specific situation, something along the lines of MissingMultilineCloserSameLine, FoundMultilineCloserSameLine.

Aha, that's a good 5th alternative, had not thought about it! For sure it will be enough for us to be able to ignore those error codes.

In that case, I'm open to a PR to make that change.

[stronk7]

Sup, will give a try to it soon, thanks!