False positive Universal.Arrays.DuplicateArrayKey.Found when array keys look numeric

[aymanrady]

Bug Description

Universal.Arrays.DuplicateArrayKey.Found is incorrectly reported in an array with number-like keys.

Given the following reproduction Scenario

The issue happens when running this command:

phpcs -ps file.php --standard=Universal

... over a file containing this code:

<?php

array(
'_1' => 'value1',
'_2' => 'value2',
);

I'd expect the following behaviour

No errors.

Instead this happened

---------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------------------------------------------------------------
 5 | ERROR | Duplicate array key found. The value will be overwritten. The integer array key "0" was first seen on line 4 (Universal.Arrays.DuplicateArrayKey.Found)
---------------------------------------------------------------------------------------------------------------------------------------------------------------------

Environment

Environment	Answer
PHP version	8.2.22
PHP_CodeSniffer version	3.10.2
PHPCSExtra version	1.2.1
PHPCSUtils version	1.0.12
Install type	composer project local

Additional Context (optional)

Ran unit tests with this patch

diff --git a/Universal/Tests/Arrays/DuplicateArrayKeyUnitTest.inc b/Universal/Tests/Arrays/DuplicateArrayKeyUnitTest.inc
index 2101f89..14d7ebf 100644
--- a/Universal/Tests/Arrays/DuplicateArrayKeyUnitTest.inc
+++ b/Universal/Tests/Arrays/DuplicateArrayKeyUnitTest.inc
@@ -165,3 +165,8 @@ $testPHPLt8VsPHP8 = array(
     1   => 'j', // Duplicate in PHP < 8, not in PHP 8.
     2   => 'k', // Duplicate in PHP < 8, not in PHP 8.
 );
+
+$testNumberLikeKey = array(
+    '_1' => 1,
+    '_2' => 2,
+);

There was 1 failure:

1) PHPCSExtra\Universal\Tests\Arrays\DuplicateArrayKeyUnitTest::testSniff
[LINE 171] Expected 0 error(s) in DuplicateArrayKeyUnitTest.inc but found 1 error(s). The error(s) found were:
 -> Duplicate array key found. The value will be overwritten. The integer array key "0" was first seen on line 170 (Universal.Arrays.DuplicateArrayKey.Found)

/home/aymanrady/Development/open-source/PHPCSExtra/vendor/squizlabs/php_codesniffer/tests/Standards/AbstractSniffUnitTest.php:214
/home/aymanrady/Development/open-source/PHPCSExtra/vendor/squizlabs/php_codesniffer/tests/TestSuite7.php:28

Tested Against develop branch?

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

[jrfnl]

@aymanrady Thank you for reporting this issue! The actual problem is in the underlying PHPCSUtils package, so I'll transfer the issue to that repo.

Technical explanation of the bug: The PHPCSUtils Numbers::is*() methods (like Numbers::isDecimalInt(), which is used in the AbstractArrayDeclarationSniff::getActualArrayKey() method, which is what the DuplicateArrayKey sniff uses to determine the real key), strips underscores from a textual representation of the array key as part of its handling of PHP 7.4 numeric literals with separators. However, the underscore separator is not allowed at the start or end of the number, but the Numbers class did not take this into account properly.

This was partially by design to make the class developer-error tolerant - which allows for PHPCS sniffs using the functions to report on such developer errors -, but this bug report makes a good case for removing this developer error tolerance from these methods.

[jrfnl]

FYI: PR #620 should fix this issue and will be included in PHPCSUtils1.1.0, which I'm hoping to release later this week.