switch case and default must not have semicolons

[kenguest]

Update section 5.2 advising that semi-colons must not be used in switch statement.

Resolves #128

[theodorejb]

I don't think this is needed, since the example already uses colons, and the semicolon syntax is now deprecated in PHP itself.

[kenguest]

For some people, implicit/implied requirements as shown in sample code is not as clear-cut compared to when outlined in the prose of the requirements document, so this is still very much needed.

[kenguest]

I've changed the wording to below:

The caseanddefault keywords MUST use colons as shown in the sample code below.

[theodorejb]

The description should probably mention that this will close #128.

[kenguest]

The description should probably mention that this will close #128.

I've updated the description accordingly.

[Crell]

(Adjusted reference in the summary to match GitHub automation expectations.)

[Crell]

@jrfnl Would this language be good enough for you? It's a bit wiggly in how it specifies things, but maybe that's OK in this case?

[jrfnl]

@jrfnl Would this language be good enough for you? It's a bit wiggly in how it specifies things, but maybe that's OK in this case?

I still think it's kind of redundant, but if this is being made explicit, than the text should be leading. The reference to the sample code like this, would be the only one of its kind in the whole document, so to me, it feels like it doesn't belong.

If anything, the rule as described now, still leaves room for code like this - take note of the redundant curly braces -, which IIRC is discouraged:

switch ($foo) {
    case 10 : {
        // Do something.
        break;
    }
}

I wonder if PERCS should be explicit with an opinion about redundant curlies in this context if the use of the colon is being made explicit now anyway ?

[Crell]

(With editor hat on) I'm open to not doing anything here if that ends up being the consensus. Or if the consensus is we should specify something, that's OK too. I don't have a large personal stake here.

I... didn't know extra braces were legal, either. 😅 I'd be OK with forbidding those if the rest of the WG is.

[Crell]

After sleeping on this for a while, here's my recommendation:

I agree with @jrfnl that "do like the example below" is suboptimal wording, regardless of the section. However, the text does that in a few places right now (eg, match() does the same). Fixing all of that is out of scope for this issue. But we can clarify switch in a way that implicitly removes the semi-colon option.

The text above the example currently reads:

A switch structure looks like the following. Note the placement of parentheses, spaces, and braces. The case statement MUST be indented once from switch, and the break keyword (or other terminating keywords) MUST be indented at the same level as the case body. There MUST be a comment such as // no break when fall-through is intentional in a non-empty case body.

I propose we change that to:

A switch structure must follow the rules below:

* `case` statements MUST be indented one level from the `switch`.
* The `case` statements line MUST consist only of the keyword, the case value, and a colon.
* The statements of a `case` statement MUST be indented one level from the `case`.
* If a non-empty case intends to continue into the following case, then a comment `// no break` MUST be included to highlight the deliberate lack of a `break` statement.
* All other non-empty cases MUST have a terminating `break`, even if they are the final one in the `switch` block.
* The `case` body MUST NOT be wrapped in `{}`.

See the example below.

That more precisely describes the current ruleset, implicitly forbids the semicolon option, and explicitly forbids wrapping {}.

Would that be satisfactory for everyone?

[fredden]

• If a non-empty case intends to continue into the following case, then a comment // no break MUST be included to highlight the deliberate lack of a break statement.

I suggest slightly different wording for the 'must have a comment' rule:

-    * If a non-empty case intends to continue into the following case, then a comment `// no break` MUST be included to highlight the deliberate lack of a `break` statement.
+    * If a non-empty case intends to continue into the following case, then a comment MUST be included to highlight the deliberate lack of a `break` statement. For example, `// no break`.

It seems that in the original text, // no break was an example and not an explicit 'use these characters to satisfy this rule' instruction.

• All other non-empty cases MUST have a terminating break, even if they are the final one in the switch block.

The example 'case 4' does not satisfy this rule as it has a return statement and no break.

[Crell]

"break or return" seems fine to me. For the first, yes, the text above would also standardize the comment text. Whether that's good or an overreach, I suppose the WG will reply after the New Year. 😄

[jrfnl]

@Crell Thanks for that proposal. In my opinion that would definitely be better than what's there now and I'd also advocate for opening separate issues to address the other instances of "look at the examples and figure out what the rules should be".

As for the proposed text, please consider the following feedback:

• The case statements line MUST consist only of the keyword, the case value, and a colon.

the case value, => the case condition

Think:

switch (true) {
    case $a === 10:
        break;
}

Note: I'm not making a case for this type of code, just giving an example of why the phrasing needs to be adjusted.

Also note: the phrasing is still ambiguous as it could strictly be interpreted as not allowing any whitespace or comments on that line....
It also (non-ambiguously) now states that the case statement should be on a single line, which kind of clashes with other PER rules.

Think:

switch (true) {
    case (
        $a === 10
        && $b === 20
    ):
        break;
}

• The statements of a case statement MUST be indented one level from the case.

The statements => The body (also makes the phrasing more consistent with the last bullet)

• If a non-empty case intends to continue into the following case, then a comment // no break MUST be included to highlight the deliberate lack of a break statement.

I support @fredden's observation and don't think standardizing the exact comment wording is a good idea. Quite aside from the fact that the proposed comment does not comply with normalized capitalization and punctuation rules, so will easily conflict with comment related sniffs.

If it would be standardized then I'd suggest that a comment like // Deliberate fall-through. will make the intention more explicit (instead of stating the obvious).

• All other non-empty cases MUST have a terminating break, even if they are the final one in the switch block.

How about calling it a "terminating statement" instead ? That way you avoid having to list all the possibilities.

For the record, the complete list of terminating statements as far as I currently can see it is: break, continue, return, throw, goto, exit and die....

[Crell]

Revised suggestion based on the above comments:

A switch structure must follow the rules below:

* `case` statements MUST be indented one level from the `switch`.
* The `case` statements line MUST consist only of the `case` keyword, a single space, the case condition, and a colon.
* If the case condition is sufficiently complex to warrant being multi-line, it MUST be wrapped in parentheses, MUST have the opening parenthesis on the same line as the `case` keyword, and MUST end with a line containing only the closing parenthesis and colon, with no space between them.
* The body of a `case` statement MUST be indented one level from the `case`.
* If a non-empty case intends to continue into the following case, then a clear comment MUST be included to highlight the deliberate lack of a `break`, `return`, or similar termination statement.  Examples include "No break," "Deliberate fall-through," etc.
* All other non-empty cases MUST have a terminating `break`, `return`, or similar termination statement, even if they are the final one in the `switch` block.
* The `case` body MUST NOT be wrapped in `{}`.

See the example below.

Working group, your thoughts?

[mathroc]

looks good, just a note on "case condition" I think value was more appropriate, what's after the case is not the condition that decides if the case block is executed. it's executed if that value matches the switch value. so it's equivalent to the condition only if it's a switch(true)

in the PHP documentation about switch, we can read

compare the same variable (or expression) with many different values, and execute a different piece of code depending on which value it equals to.

and

when a case statement is found whose expression evaluates to a value that matches the value of the switch expression does PHP begin to execute the statements

so "expression" could work too

[Crell]

@kenguest Are you going to update this PR or shall I make a new one?

[kenguest]

@Crell I'll update it soon.

[kenguest]

trivial-but-not-too-trivial body added to skeleton code example.