Conflict between Generic.WhiteSpace.ScopeIndent and PSR2.Methods.FunctionCallSignature in a match block

[morozov]

Describe the bug
The two sniffs in subject conflict if applied to a match block.

Code sample

<?php

function toString(): string
{
    return sprintf(
        '%s',
        match ($type) {
            'foo' => 'bar',
        },
    );
}

Custom ruleset
None

To reproduce

$ phpcs --sniffs=Generic.WhiteSpace.ScopeIndent,PSR2.Methods.FunctionCallSignature test.php
E 1 / 1 (100%)

FILE: test.php
------------------------------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------
 7 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
   |       |     (Generic.WhiteSpace.ScopeIndent.IncorrectExact)
 9 | ERROR | [x] Line indented incorrectly; expected 4 spaces, found 8
   |       |     (Generic.WhiteSpace.ScopeIndent.IncorrectExact)
------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
------------------------------------------------------------------------------------------------

$ phpcbf --sniffs=Generic.WhiteSpace.ScopeIndent,PSR2.Methods.FunctionCallSignature test.php
E 1 / 1 (100%)

PHPCBF RESULT SUMMARY
----------------------------------------------------------------------
FILE                                                  FIXED  REMAINING
----------------------------------------------------------------------
test.php                                              FAILED TO FIX
----------------------------------------------------------------------
A TOTAL OF 0 ERRORS WERE FIXED IN 1 FILE
----------------------------------------------------------------------
PHPCBF FAILED TO FIX 1 FILE
----------------------------------------------------------------------

After fixing, the file looks like this:

<?php

function toString(): string
{
    return sprintf(
        '%s',
    match ($type) {
        'foo' => 'bar',
    },
    );
}

Expected behavior
The original formatting is considered valid and isn't attempted to be fixed.

Versions (please complete the following information):

• OS: Linux
• PHP: 8.0.2
• PHPCS: dev-master 1106d65

[jrfnl]

@morozov Thanks for reporting this. I'm looking into it. Initial tests with match did not show any issue, so I need to dig a little deeper. Also see PR #3248. If I find a solution, I'll add it to that PR (as it's still open anyway).

[jrfnl]

FYI: I haven't found a solution yet, but from my initial investigation, I get the impression that when match() is nested in something, the new scope doesn't increase the indent, which is why you are seeing this error.

@gsherwood You are way more familiar with this sniff than me. Can you think of a quick solution of shall I continue to look into it ?

[gsherwood]

@jrfnl I'll take a look at this report now and get back to you

[gsherwood]

This is the debug output:

Start with token 0 on line 1 with indent 0
Opening parenthesis found on line 3
	=> disabling exact indent checking until 6 (T_CLOSE_PARENTHESIS)
Closing parenthesis found on line 3
	 * ignoring single-line definition *
Open scope (T_FUNCTION) on line 4
	=> added open scope 48 (T_CLOSE_CURLY_BRACKET) on line 11, pointing to condition 2 (T_FUNCTION) on line 3
	=> indent set to 4 by token 11 (T_OPEN_CURLY_BRACKET)
Opening parenthesis found on line 5
	=> disabling exact indent checking until 45 (T_CLOSE_PARENTHESIS)
[Line 7] Line indented incorrectly; expected 4 spaces, found 8
	=> add adjustment of -4 for token 24 (T_MATCH) on line 7
Opening parenthesis found on line 7
	=> disabling exact indent checking until 45 (T_CLOSE_PARENTHESIS)
Closing parenthesis found on line 7
	 * ignoring single-line definition *
Open scope (T_MATCH) on line 7
	=> added open scope 41 (T_CLOSE_CURLY_BRACKET) on line 9, pointing to condition 24 (T_MATCH) on line 7
	=> indent set to 8 by token 30 (T_OPEN_CURLY_BRACKET)
Indent adjusted to 8 for T_CONSTANT_ENCAPSED_STRING on line 8
	=> add adjustment of -4 for token 33 (T_CONSTANT_ENCAPSED_STRING) on line 8
Close scope (T_MATCH) on line 9
	=> removed open scope 24 (T_MATCH) on line 7
	* first token is 24 (T_MATCH) on line 7 *
	=> indent set to 4 by token 41 (T_CLOSE_CURLY_BRACKET)
[Line 9] Line indented incorrectly; expected 4 spaces, found 8
	=> add adjustment of -4 for token 41 (T_CLOSE_CURLY_BRACKET) on line 9
Closing parenthesis found on line 10
	* token is inside condition 2 (T_FUNCTION) on line 3 *
	* using condition *
	=> checking indent of 4; main indent set to 4 by token 2 (T_FUNCTION)
Close scope (T_FUNCTION) on line 11
	=> removed open scope 2 (T_FUNCTION) on line 3
	* first token is 2 (T_FUNCTION) on line 3 *
	=> indent set to 0 by token 48 (T_CLOSE_CURLY_BRACKET)

Exact indent checking is turned off during the processing of the return statement, as it should be. But the match block is a new scope, so exact indent checking looks like it's reenabled there. I'm not sure what tests cases are going to fail if I ignore the exact matching for the match block, but I'll guess we'll find out :)

[gsherwood]

It broke what I thought it might, and shows that this sniff needs a stack for temporarily disabling exact indent checking. Here is the failing test:

$list = [
    'fn' => function ($a) {
                if ($a === true) {
                    echo 'hi';
                }
    }
];

Exact checking is disabled inside the array, which means you can now do anything you want in there. It need to revert to exact matching while inside a new scope, then back to ignoring that when it leaves the scope. Will play around and see how best to fix this.

[gsherwood]

A stack isn't what's needed here. This sniff would need to detect function calls and disable exact checking of scope blocks when they are arguments of a function call, which might work by just checking if the scope is inside parenthesis and turning things off. Will try that.

[gsherwood]

That seems to have worked. I'll clean things up and commit a fix a bit later.

[gsherwood]

I've pushed up a fix for this. Thanks for reporting it, and for testing this before it got released.

[morozov]

Works for me. Thank you!

[jrfnl]

Thanks @gsherwood for taking that one on.

I've been trying to think of more test cases with named parameters, match expressions and constructor property promotion for the ScopeIndent sniff, but with simple code snippets, it seems to work just fine. 🤞 It will also do so with more complex ones...