Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array destructuring in foreach - conflict between SpaceBeforeComma & NoSpaceAfterComma #527

Open
VladaHejda opened this issue Jun 13, 2024 · 3 comments · May be fixed by #748
Open

Array destructuring in foreach - conflict between SpaceBeforeComma & NoSpaceAfterComma #527

VladaHejda opened this issue Jun 13, 2024 · 3 comments · May be fixed by #748

Comments

@VladaHejda
Copy link

VladaHejda commented Jun 13, 2024

Describe the bug

Destructuring an array for foreach - foreach ($data as [, , $value]) { - violates the Squiz.Arrays.ArrayDeclaration.SpaceBeforeComma ("Expected 0 spaces between "," and comma; 1 found")
But when "fixed" - foreach ($data as [,, $value]) { - the Squiz.Arrays.ArrayDeclaration.NoSpaceAfterComma is violated ("Expected 1 space between comma and ","; 0 found")
So this code cannot be written while using both sniffs together.

Code sample

class Foo
{
	/**
	 * @param list<array{int, int, int}> $data
	 */
	public function bar(array $data): int
	{
		$sum = 0;
		foreach ($data as [, , $value]) {
			$sum += $value;
		}

		return $sum;
	}
}

Versions

Operating System Windows 11
PHP version 8.3
PHP_CodeSniffer version 3.10.1
Install type Composer
@VladaHejda VladaHejda changed the title Array destructuin in foreach - conflict between SpaceBeforeComma & NoSpaceAfterComma Array destructuring in foreach - conflict between SpaceBeforeComma & NoSpaceAfterComma Jun 13, 2024
@jrfnl
Copy link
Member

jrfnl commented Jun 14, 2024

@VladaHejda Thank you for reporting this. This is due to the sniff incorrectly acting on short lists.

The Squiz.Arrays.ArrayDeclaration sniff is known to be problematic and has been for years.

I've been building up a (highly configurable) NormalizedArrays standard in PHPCSExtra to replace it, but that's not complete yet. Would be lovely if I could find some time to finish that at some point.

This specific case does sound like a relatively simple fix though, so if someone would submit a PR with a small/simple fix for this specific issue, I will accept it, but other than that, this sniff is out of bounds for large fixes/rewrites as it is just not worth the time.

@rodrigoprimo
Copy link
Contributor

This specific case does sound like a relatively simple fix though, so if someone would submit a PR with a small/simple fix for this specific issue, I will accept it, but other than that, this sniff is out of bounds for large fixes/rewrites as it is just not worth the time.

@jrfnl, I spent some time checking what is causing this issue and how to fix it. Initially, I thought the idea was to bail early when the T_OPEN_SHORT_ARRAY token represents a short list and not an array. But it seems that doing that without #12 or IsShortArrayOrListWithCache::isShortArray() from PHPCSUtils is not trivial.

If the above is correct and I'm not missing something, the other solution that occurred to me is to bail early when the T_OPEN_SHORT_ARRAY token is inside a foreach statement. I believe this means that the token represents a short list, as I cannot think of a scenario where it would be an array. I can think of two approaches to verify this:

  • Check if the non-empty token before the T_OPEN_SHORT_ARRAY is T_AS.
  • Check if the T_OPEN_SHORT_ARRAY has the nested_parenthesis array and then check if the parenthesis owner of the last parenthesis is a T_FOREACH token (I still have to check if it is the last or the first element of the nested_parenthesis array that should be used in this case).

Is this more or less what you had in mind?

Thanks!

@jrfnl
Copy link
Member

jrfnl commented Jun 25, 2024

Initially, I thought the idea was to bail early when the T_OPEN_SHORT_ARRAY token represents a short list and not an array. But it seems that doing that without #12 or IsShortArrayOrListWithCache::isShortArray() from PHPCSUtils is not trivial.

Correct. Filtering out short lists completely for this sniff is not on the table due to the complexity.

... the other solution that occurred to me is to bail early when the T_OPEN_SHORT_ARRAY token is inside a foreach statement. I believe this means that the token represents a short list, as I cannot think of a scenario where it would be an array.

That was the idea yes, but there is a scenario when a T_OPEN_SHORT_ARRAY could still be an array:

foreach ([$a, $b] as $c) {}

I can think of two approaches to verify this:

  • Check if the non-empty token before the T_OPEN_SHORT_ARRAY is T_AS.
  • Check if the T_OPEN_SHORT_ARRAY has the nested_parenthesis array and then check if the parenthesis owner of the last parenthesis is a T_FOREACH token (I still have to check if it is the last or the first element of the nested_parenthesis array that should be used in this case).

Sort of...
The second check should come first.
The first check after that, but is incorrect/not precise enough as outlined above. There may still be a key between the short list and the as keyword, i.e. https://3v4l.org/b5qm7, so you need to find the stack position of the as keyword between the open/close parentheses of the foreach and then you'll need to compare the position of the T_OPEN_SHORT_ARRAY token with the position of the T_AS.

Note: even with this additional check, this can still lead to false positives/negatives (short list as a value in an array before the as / short array as a list key in a short list after the as), but it should cover the most commonly used code pattern sufficiently.

Does this help ?

rodrigoprimo added a commit to rodrigoprimo/PHP_CodeSniffer that referenced this issue Nov 29, 2024
This commit improves how the `Squiz.Arrays.ArrayDeclaration` sniff
handles short lists inside a foreach and fixes a false positive.

See PHPCSStandards#527
rodrigoprimo added a commit to rodrigoprimo/PHP_CodeSniffer that referenced this issue Nov 29, 2024
This commit improves how the `Squiz.Arrays.ArrayDeclaration` sniff
handles short lists inside a foreach and fixes a false positive.

See PHPCSStandards#527
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants