Repository: phpactor/phpactor (php)

Issue:
Problems with unicode
Phpactor does not seem to correctly work with various unicode characters. Some characters raise exceptions and "position aware" features like diagnostics have wrong positions.

Example PHP code:

```php
<?php

echo '👩🏽‍👨🏽‍👦🏽‍👧🏽 ' . invalid() . strlen('Lorem ipsum dolor sit amet');
```

Phpactor shows the following error when requesting completion:

```
Request textDocument/completion failed.
  Message: Exception [RuntimeException] preg_replace could not parse string (size 49): Malformed UTF-8 characters, possibly incorrectly encoded at dantleech.vscode-phpactor/phpactor/lib/Completion/Core/Util/OffsetHelper.php#15
  Code: -32603 
#0 dantleech.vscode-phpactor/phpactor/lib/Completion/Bridge/TolerantParser/WorseReflection/DoctrineAnnotationCompletor.php(95): Phpactor\Completion\Core\Util\OffsetHelper::lastNonWhitespaceCharacterOffset()
#1 dantleech.vscode-phpactor/phpactor/lib/Completion/Bridge/TolerantParser/WorseReflection/DoctrineAnnotationCompletor.php(38): Phpactor\Completion\Bridge\TolerantParser\WorseReflection\DoctrineAnnotationCompletor->truncateSource()
#2 dantleech.vscode-phpactor/phpactor/lib/Completion/Core/ChainCompletor.php(25): Phpactor\Completion\Bridge\TolerantParser\WorseReflection\DoctrineAnnotationCompletor->complete()
...
```

Furthermore the red underlines because of the unknown function `invalid` are at the wrong position.

VS Code:
![image](https://github.com/phpactor/phpactor/assets/14216933/b0656319-7bbc-4b2e-8517-be4bf5ff9c35)

Neovim:
![image](https://github.com/phpactor/phpactor/assets/14216933/5e6691eb-a8d1-4bc0-bd87-13f6c9eadbb3)

Pull request description:
Gh 2529 other UTF-16 fixes
Fixes #2529 

Updates position converter and document highlighting to convert to Positions with UTF-16 character offsets as per expected LSP defaults. It's somewhat +1/-1.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c2d973a6..c2317593 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -24,7 +24,7 @@ Bug fixes:
 
   - Fix detection of import used relatively in an annotation #2539
   - Fix PHAR crashing issue on PHP8.3 #2533
-  - Fix UTF-16 conversion for LSP #2530
+  - Fix UTF-16 conversion for LSP #2530 #2557
   - Fix support for Attributes on readonly classes #2493
   - Fix `$this` undefined var false positive in anon. class #2469 @mamazu
   - Fix `$argv` undefined var false positives #2468 @mamazu
diff --git a/lib/Extension/LanguageServerBridge/Converter/PositionConverter.php b/lib/Extension/LanguageServerBridge/Converter/PositionConverter.php
index 416ce04e..ba0f26a5 100644
--- a/lib/Extension/LanguageServerBridge/Converter/PositionConverter.php
+++ b/lib/Extension/LanguageServerBridge/Converter/PositionConverter.php
@@ -5,7 +5,6 @@ namespace Phpactor\Extension\LanguageServerBridge\Converter;
 use Phpactor\LanguageServerProtocol\Position;
 use Phpactor\TextDocument\ByteOffset;
 use Phpactor\TextDocument\LineCol;
-use Phpactor\TextDocument\Util\LineAtOffset;
 use RuntimeException;
 
 class PositionConverter
@@ -21,24 +20,31 @@ class PositionConverter
             $offset = ByteOffset::fromInt(strlen($text));
         }
 
-        $lineCol = LineCol::fromByteOffset($text, $offset);
-        $lineAtOffset = LineAtOffset::lineAtByteOffset($text, $offset);
+        $lineCol = LineCol::fromByteOffset($text, $offset, true);
 
-        $lineAtOffset = mb_substr(
-            $lineAtOffset,
-            0,
-            $lineCol->col() - 1
-        );
-
-        return new Position($lineCol->line() - 1, self::countUtf16CodeUnits($lineAtOffset));
+        return new Position($lineCol->line() - 1, $lineCol->col() - 1);
     }
 
+    /**
+     * Convert UTF-16 position to byteoffset.
+     */
     public static function positionToByteOffset(Position $position, string $text): ByteOffset
     {
+        // get byte offset position of line start
         $lineCol = new LineCol($position->line + 1, 1);
         $byteOffset = $lineCol->toByteOffset($text);
 
-        return ByteOffset::fromInt($byteOffset->toInt() + $position->character);
+        // convert line to UTF-16 as Position character is UTF-16 code unit position
+        $rest = substr($text, $byteOffset->toInt());
+        $lineEnd = strpos($rest, "\n");
+        if ($lineEnd !== false) {
+            $rest = substr($rest, 0, $lineEnd);
+        }
+        $rest = self::normalizeUtf16($rest);
+        $seg = substr($rest, 0, $position->character * 2);
+        $utf8 = \mb_convert_encoding($seg, 'UTF-8', 'UTF-16');
+
+        return ByteOffset::fromInt($byteOffset->toInt() + strlen($utf8));
     }
 
     /**
@@ -53,9 +59,4 @@ class PositionConverter
 
         return $utf16;
     }
-
-    private static function countUtf16CodeUnits(string $string): int
-    {
-        return (int)(strlen(self::normalizeUtf16($string)) / 2);
-    }
 }
diff --git a/lib/TextDocument/EfficientLineCols.php b/lib/TextDocument/EfficientLineCols.php
index c27398fa..ebdc6f57 100644
--- a/lib/TextDocument/EfficientLineCols.php
+++ b/lib/TextDocument/EfficientLineCols.php
@@ -25,13 +25,15 @@ final class EfficientLineCols
      * Initialize the converter with the list of byte offsets to be converted.
      *
      * For converting to LSP positions there is a flag to return the character
-     * offset.Note that, like the line/col number, this is 1 based.
+     * offset. Note that, unlike the line/col number, this is 1 based and is based
+     * on UTF-16 code units.
+     *
      * @param list<int> $byteOffsetInts
      */
     public static function fromByteOffsetInts(
         string $text,
         array $byteOffsetInts,
-        bool $charOffset = false
+        bool $lspPosition = false
     ): self {
         sort($byteOffsetInts);
 
@@ -70,10 +72,18 @@ final class EfficientLineCols
                     $byteOffset - $start
                 );
 
-                $positions[$byteOffset] = new LineCol(
-                    $lineNb,
-                    $charOffset ? strlen($section) + 1 : mb_strlen($section) + 1
-                );
+                if ($lspPosition) {
+                    $utf16 = \mb_convert_encoding($section, 'UTF-16', 'UTF-8');
+                    $positions[$byteOffset] = new LineCol(
+                        $lineNb,
+                        intval(strlen($utf16) / 2) + 1,
+                    );
+                } else {
+                    $positions[$byteOffset] = new LineCol(
+                        $lineNb,
+                        strlen($section) + 1,
+                    );
+                }
                 $byteOffset = array_shift($byteOffsetInts);
                 if (null === $byteOffset) {
                     break;
diff --git a/lib/TextDocument/LineCol.php b/lib/TextDocument/LineCol.php
index 616b0dcd..e10924b7 100644
--- a/lib/TextDocument/LineCol.php
+++ b/lib/TextDocument/LineCol.php
@@ -76,7 +76,7 @@ final class LineCol
         return ByteOffset::fromInt(strlen($text));
     }
 
-    public static function fromByteOffset(string $text, ByteOffset $byteOffset): self
+    public static function fromByteOffset(string $text, ByteOffset $byteOffset, bool $lspPosition = false): self
     {
         if ($byteOffset->toInt() > strlen($text)) {
             $byteOffset = ByteOffset::fromInt(strlen($text));
@@ -111,6 +111,13 @@ final class LineCol
                     0,
                     $byteOffset->toInt() - $start
                 );
+                if ($lspPosition) {
+                    $utf16 = \mb_convert_encoding($section, 'UTF-16', 'UTF-8');
+                    return new self(
+                        $lineNb,
+                        intval(strlen($utf16) / 2) + 1,
+                    );
+                }
 
                 return new self($lineNb, mb_strlen($section) + 1);
             }


Test patch:
diff --git a/lib/Extension/LanguageServerBridge/Tests/Converter/LocationConverterTest.php b/lib/Extension/LanguageServerBridge/Tests/Converter/LocationConverterTest.php
index 09e1d0f5..daa522c4 100644
--- a/lib/Extension/LanguageServerBridge/Tests/Converter/LocationConverterTest.php
+++ b/lib/Extension/LanguageServerBridge/Tests/Converter/LocationConverterTest.php
@@ -101,21 +101,21 @@ class LocationConverterTest extends IntegrationTestCase
             '😼😼😼😼😼',
             2,
             2,
-            $this->createRange(0, 2, 0, 2)
+            $this->createRange(0, 1, 0, 1)
         ];
 
         yield '4 byte char 2nd char' => [
             '😼😼😼😼😼',
             2,
             5,
-            $this->createRange(0, 2, 0, 4)
+            $this->createRange(0, 1, 0, 3)
         ];
 
         yield '4 byte char 4th char' => [
             '😼😼😼😼😼',
             2,
             16,
-            $this->createRange(0, 2, 0, 8)
+            $this->createRange(0, 1, 0, 8)
         ];
     }
 
diff --git a/lib/Extension/LanguageServerBridge/Tests/Unit/Converter/PositionConverterTest.php b/lib/Extension/LanguageServerBridge/Tests/Unit/Converter/PositionConverterTest.php
index 1d9c098f..1557ecbb 100644
--- a/lib/Extension/LanguageServerBridge/Tests/Unit/Converter/PositionConverterTest.php
+++ b/lib/Extension/LanguageServerBridge/Tests/Unit/Converter/PositionConverterTest.php
@@ -9,6 +9,45 @@ use Phpactor\TextDocument\ByteOffset;
 
 class PositionConverterTest extends TestCase
 {
+    public function testPositionToByteOffset(): void
+    {
+        self::assertEquals(
+            ByteOffset::fromInt(15),
+            PositionConverter::positionToByteOffset(
+                new Position(2, 3),
+                <<<'EOT'
+                    Hello
+                    Carld
+                    World
+                    Farld
+                    EOT
+            )
+        );
+        self::assertEquals(
+            ByteOffset::fromInt(39),
+            PositionConverter::positionToByteOffset(
+                new Position(2, 3),
+                <<<'EOT'
+                    👩👨👦👧
+                    👩👨👦👧
+                    👩👨👦👧
+                    👩👨👦👧
+                    EOT
+            )
+        );
+
+        self::assertEquals(
+            ByteOffset::fromInt(45),
+            PositionConverter::positionToByteOffset(
+                new Position(2, 30),
+                <<<'PHP'
+                    <?php
+
+                    echo '👩👨👦👧' . invalid() . strlen('Lorem ipsum dolor sit amet');
+                    PHP
+            )
+        );
+    }
     public function testWhenOutOfBoundsAssumeEndOfDocument(): void
     {
         self::assertEquals(
@@ -38,7 +77,7 @@ class PositionConverterTest extends TestCase
             )
         );
         self::assertEquals(
-            new Position(0, 3),
+            new Position(0, 2),
             PositionConverter::byteOffsetToPosition(
                 ByteOffset::fromInt(2),
                 'a𐐀b'
diff --git a/lib/TextDocument/Tests/Benchmark/EfficientLineColsBench.php b/lib/TextDocument/Tests/Benchmark/EfficientLineColsBench.php
new file mode 100644
index 00000000..e33274f7
--- /dev/null
+++ b/lib/TextDocument/Tests/Benchmark/EfficientLineColsBench.php
@@ -0,0 +1,44 @@
+<?php
+
+namespace Phpactor\TextDocument\Tests\Benchmark;
+
+use PhpBench\Attributes\BeforeMethods;
+use Phpactor\TextDocument\ByteOffset;
+use Phpactor\TextDocument\EfficientLineCols;
+use Phpactor\TextDocument\LineCol;
+
+#[BeforeMethods('setUp')]
+class EfficientLineColsBench
+{
+    private string $contents;
+
+    public function setUp(): void
+    {
+        $this->contents = (string)file_get_contents(__DIR__ . '/example/code.test');
+    }
+    public function benchLineCols(): void
+    {
+        EfficientLineCols::fromByteOffsetInts(
+            $this->contents,
+            [89,191,274,326,373,480,552,2000],
+        );
+    }
+
+    public function benchLineColsUtf16Positions(): void
+    {
+        EfficientLineCols::fromByteOffsetInts(
+            $this->contents,
+            [89,191,274,326,373,480,552,2000],
+            true
+        );
+    }
+
+    public function benchIneffificentLineCols(): void
+    {
+        foreach (
+            [89,191,274,326,373,480,552,2000] as $byteOffset
+        ) {
+            LineCol::fromByteOffset($this->contents, ByteOffset::fromInt($byteOffset));
+        }
+    }
+}
diff --git a/lib/TextDocument/Tests/Benchmark/example/code.test b/lib/TextDocument/Tests/Benchmark/example/code.test
new file mode 100644
index 00000000..f321ce80
--- /dev/null
+++ b/lib/TextDocument/Tests/Benchmark/example/code.test
@@ -0,0 +1,108 @@
+<?php
+
+class PositionConverter
+{
+    public static function intByteOffsetToPosition(int $offset, string $text): Position
+    {
+        return self::byteOffsetToPosition(ByteOffset::fromInt($offset), $text);
+    }
+
+    public static function byteOffsetToPosition(ByteOffset $offset, string $text): Position
+    {
+        if ($offset->toInt() > strlen($text)) {
+            $offset = ByteOffset::fromInt(strlen($text));
+        }
+
+        $lineCol = LineCol::fromByteOffset($text, $offset);
+        $lineAtOffset = LineAtOffset::lineAtByteOffset($text, $offset);
+
+        $lineAtOffset = mb_substr(
+            $lineAtOffset,
+            0,
+            $lineCol->col() - 1
+        );
+
+        return new Position($lineCol->line() - 1, self::countUtf16CodeUnits($lineAtOffset));
+    }
+}
+<?php
+
+class PositionConverter
+{
+    public static function intByteOffsetToPosition(int $offset, string $text): Position
+    {
+        return self::byteOffsetToPosition(ByteOffset::fromInt($offset), $text);
+    }
+
+    public static function byteOffsetToPosition(ByteOffset $offset, string $text): Position
+    {
+        if ($offset->toInt() > strlen($text)) {
+            $offset = ByteOffset::fromInt(strlen($text));
+        }
+
+        $lineCol = LineCol::fromByteOffset($text, $offset);
+        $lineAtOffset = LineAtOffset::lineAtByteOffset($text, $offset);
+
+        $lineAtOffset = mb_substr(
+            $lineAtOffset,
+            0,
+            $lineCol->col() - 1
+        );
+
+        return new Position($lineCol->line() - 1, self::countUtf16CodeUnits($lineAtOffset));
+    }
+}
+<?php
+
+class PositionConverter
+{
+    public static function intByteOffsetToPosition(int $offset, string $text): Position
+    {
+        return self::byteOffsetToPosition(ByteOffset::fromInt($offset), $text);
+    }
+
+    public static function byteOffsetToPosition(ByteOffset $offset, string $text): Position
+    {
+        if ($offset->toInt() > strlen($text)) {
+            $offset = ByteOffset::fromInt(strlen($text));
+        }
+
+        $lineCol = LineCol::fromByteOffset($text, $offset);
+        $lineAtOffset = LineAtOffset::lineAtByteOffset($text, $offset);
+
+        $lineAtOffset = mb_substr(
+            $lineAtOffset,
+            0,
+            $lineCol->col() - 1
+        );
+
+        return new Position($lineCol->line() - 1, self::countUtf16CodeUnits($lineAtOffset));
+    }
+}
+<?php
+
+class PositionConverter
+{
+    public static function intByteOffsetToPosition(int $offset, string $text): Position
+    {
+        return self::byteOffsetToPosition(ByteOffset::fromInt($offset), $text);
+    }
+
+    public static function byteOffsetToPosition(ByteOffset $offset, string $text): Position
+    {
+        if ($offset->toInt() > strlen($text)) {
+            $offset = ByteOffset::fromInt(strlen($text));
+        }
+
+        $lineCol = LineCol::fromByteOffset($text, $offset);
+        $lineAtOffset = LineAtOffset::lineAtByteOffset($text, $offset);
+
+        $lineAtOffset = mb_substr(
+            $lineAtOffset,
+            0,
+            $lineCol->col() - 1
+        );
+
+        return new Position($lineCol->line() - 1, self::countUtf16CodeUnits($lineAtOffset));
+    }
+}
diff --git a/lib/TextDocument/Tests/Unit/EfficientLineColsTest.php b/lib/TextDocument/Tests/Unit/EfficientLineColsTest.php
index 252459f7..473125f4 100644
--- a/lib/TextDocument/Tests/Unit/EfficientLineColsTest.php
+++ b/lib/TextDocument/Tests/Unit/EfficientLineColsTest.php
@@ -66,11 +66,23 @@ class EfficientLineColsTest extends TestCase
      */
     public function provideConvertOffsetsToLineColAsOffset(): Generator
     {
-        yield 'cat is 4 bytes' => [
+        yield 'cat' => [
             [5],
             'a😸bc',
             function (EfficientLineCols $lineCols): void {
-                self::assertEquals(6, $lineCols->get(5)->col());
+                self::assertEquals(4, $lineCols->get(5)->col());
+            }
+        ];
+        yield 'utf16' => [
+            [46],
+            <<<'PHP'
+                <?php
+
+                echo '👩👨👦👧' . invalid() . strlen('Lorem ipsum dolor sit amet');
+                PHP,
+            function (EfficientLineCols $lineCols): void {
+                self::assertEquals(3, $lineCols->get(46)->line());
+                self::assertEquals(32, $lineCols->get(46)->col());
             }
         ];
     }