[FR] Trailing commas: enforce/forbid trailing commas in list-like constructs

[rodrigoprimo]

Setting the scene

PER-CS 2.0 section 2.6 defines a single trailing-comma rule for any list-like construct: single-line lists must not have a trailing comma, multi-line lists must. Array literals are already handled by NormalizedArrays.Arrays.CommaAfterLast, and the comma between attributes in an attribute block by Universal.Attributes.TrailingComma. Every other construct the rule applies to is uncovered.

Proposed change

A sniff (or sniffs, see "Open question: structure" below) to enforce this rule.

To address these rules from PER:

If that list is contained on a single line, then the last item MUST NOT have a trailing comma.
If the list is split across multiple lines, then the last item MUST have a trailing comma.

Constructs in scope

Section 2.6's list names a few constructs and ends with "etc." There are some constructs that are not mentioned directly but appear in the section 2.6 example, and some that are not mentioned in section 2.6 but appear in examples in other parts of the spec following the trailing-comma rule. Here is a table of what I could gather from the spec:

Construct	PER-CS basis	Existing sniff
Function/method/new call argument lists	Section 4.7 example shows a trailing comma and "function arguments" named in section 2.6	none
Parameter lists (function, method, closure, arrow function)	Section 4.5 example shows a trailing comma	none
Closure use(...)	named in section 2.6	none
match branches	named + shown in section 2.6	none
Argument list inside an attribute	Section 12.3 cross-ref + section 12.4 example	none
Grouped use imports	Section 3 example	none
list() / [] destructuring	not in the spec, extrapolated from "etc."	none
Array literals	Section 11 cross-ref and named + shown in section 2.6	NormalizedArrays.Arrays.CommaAfterLast
Comma between attributes (#[A, B])	not in the spec, extrapolated from "etc."	Universal.Attributes.TrailingComma

I'm not sure if the list of constructs above is correct or not and if it is missing some constructs. I wonder if it is worth opening a PER-CS clarification issue or there is a clear definition somewhere that I'm missing.

Open question: structure

Two trailing-comma sniffs already exist, both organized per construct (NormalizedArrays.Arrays.CommaAfterLast and Universal.Attributes.TrailingComma), so there is precedent for per-construct sniffs, but the remaining constructs could also be handled together. Three options:

• One sniff per construct (continuing the existing pattern).
• A single sniff for the uncovered constructs.
• A single sniff covering everything, including arrays and attributes.

I don't have a strong preference, and it is not clear to me if there is a reason to continue the existing pattern and create one sniff per construct.

The category of the sniff (or sniffs) will also depend on the structure chosen.

Notes for the implementation

Reuse NormalizedArrays.Arrays.CommaAfterLast's shape: $singleLine (default 'forbid') and $multiLine (default 'enforce') properties accepting 'enforce'/'forbid'/'skip', the same input validation, recordMetric pattern, and error-message wording. The core check is identical everywhere: find the last non-empty token before the closer, decide single- vs multi-line by comparing the construct's opener/closer lines, and add or remove the comma.

Auto-fix works in both directions. The error codes are per construct either way — suffixed on a single sniff, or one code per per-construct sniff — so the exact names depend on the structure chosen.

Describe the solution you'd like

A new sniff (or sniffs) as outlined above.

It should flag and auto-fix the following:

// Function declaration parameter list.
function foo(
    $a,
    $b   // Error: missing trailing comma (multi-line).
) {}

function bar($a, $b,) {}  // Error: trailing comma not allowed (single-line).

// Function/method call argument list.
doSomething(
    $a,
    $b,  // OK.
);

doSomething($a, $b,);     // Error: trailing comma not allowed (single-line).

// Closure use().
$fn = function () use (
    $a,
    $b   // Error: missing trailing comma (multi-line).
) {};

// match branches.
$result = match ($x) {
    1, 2 => 'a',
    default => 'b'   // Error: missing trailing comma (multi-line).
};

// list()/[] destructuring.
[
    $a,
    $b   // Error: missing trailing comma (multi-line).
] = $array;

// Argument list inside an attribute.
#[Route(
    '/path',
    methods: ['GET']   // Error: missing trailing comma (multi-line).
)]

// Grouped use import.
use Vendor\{
    ClassA,
    ClassB   // Error: missing trailing comma (multi-line).
};

Also see the examples outlined in the PER documents (rules + migration guide).

Additional context (optional)

This ticket is the result of a detailed analysis of the rules as outlined in PER-CS 2.0, as well as a critical look at what's still missing rule-wise.

• I intend to create a pull request to implement this feature.

[rodrigoprimo]

@jrfnl, just wondering if you have any thoughts regarding the open question for the sniff suggested in this issue? Thanks!

[jrfnl]

Open question: structure

Two trailing-comma sniffs already exist, both organized per construct (NormalizedArrays.Arrays.CommaAfterLast and Universal.Attributes.TrailingComma), so there is precedent for per-construct sniffs, but the remaining constructs could also be handled together. Three options:

• One sniff per construct (continuing the existing pattern).
• A single sniff for the uncovered constructs.
• A single sniff covering everything, including arrays and attributes.

I don't have a strong preference, and it is not clear to me if there is a reason to continue the existing pattern and create one sniff per construct.

The category of the sniff (or sniffs) will also depend on the structure chosen.

Good question. I'm leaning towards either option 1 or 3, but not 2 (inconsistent).

I'm inclined to say "one sniff per construct" and that each of these should have the $singleLine and $multiLine properties with the enforce/forbid/skip options.
That way each of these sniffs would have predictable properties to set and it would make picking and choosing the sniffs to use highly configurable, which I think is a good thing.

I also believe that the default values should be in line with PERCS, like they already are in the CommaAfterLast sniff.

We may want to take a decision on a consistent name for the new sniffs as well, i.e. either CommaAfterLast or TrailingComma. Probably TrailingComma ?

Construct	PER-CS basis	Existing sniff
Function/method/new call argument lists	Section 4.7 example shows a trailing comma and "function arguments" named in section 2.6	none
Parameter lists (function, method, closure, arrow function)	Section 4.5 example shows a trailing comma	none
Closure use(...)	named in section 2.6	none
match branches	named + shown in section 2.6	none
Argument list inside an attribute	Section 12.3 cross-ref + section 12.4 example	none
Grouped use imports	Section 3 example	none
list() / [] destructuring	not in the spec, extrapolated from "etc."	none
Array literals	Section 11 cross-ref and named + shown in section 2.6	NormalizedArrays.Arrays.CommaAfterLast
Comma between attributes (#[A, B])	not in the spec, extrapolated from "etc."	Universal.Attributes.TrailingComma

Nice listing!

Some notes:

• I believe "Closure use" can be grouped with "parameter lists" as they need the same entry point and are both about function declarations.
• Similarly, I believe that "argument list inside an attribute" should be grouped with "Function/method/new call argument lists" as that is a class instantiation with constructor parameters being passed.

@rodrigoprimo Does that answer your question ?