Prevent argument count errors by wrapping callables (#234)

Author: sanmai

.php-cs-fixer.dist.php

@@ -37,6 +37,7 @@
 $config = new PhpCsFixer\Config();
 $config
     ->setRiskyAllowed(true)
+    ->setUnsupportedPhpVersionAllowed(true)
     ->setRules([
         'header_comment' => ['comment_type' => 'PHPDoc', 'header' => $header, 'separate' => 'bottom', 'location' => 'after_open'],
         '@PER-CS' => true,

src/Standard.php

@@ -20,6 +20,7 @@
 
 namespace Pipeline;
 
+use ArgumentCountError;
 use ArrayIterator;
 use CallbackFilterIterator;
 use Countable;
@@ -1380,23 +1381,45 @@ public function finalVariance(
     /**
      * Eagerly iterates over the sequence using the provided callback. Discards the sequence after iteration.
      *
-     * @param callable $func A callback such as fn($value, $key); return value is ignored.
+     * @param callable(TValue, TKey=): void $func A callback such as fn($value, $key); return value is ignored.
      * @param bool $discard Whether to discard the pipeline's iterator.
      */
     public function each(callable $func, bool $discard = true): void
+    {
+        try {
+            $this->eachInternal($func);
+        } finally {
+            if ($discard) {
+                $this->discard();
+            }
+        }
+    }
+
+    /**
+     * @param callable(TValue, TKey=): void $func
+     */
+    private function eachInternal(callable $func): void
     {
         if ($this->empty()) {
             return;
         }
 
-        try {
-            foreach ($this->pipeline as $key => $value) {
+        foreach ($this->pipeline as $key => $value) {
+            try {
+                $func($value, $key);
+            } catch (ArgumentCountError) {
+                // Optimization to reduce the number of argument count errors when calling internal callables.
+                // This error is thrown when too many arguments are passed to a built-in function (that are sensitive
+                // to extra arguments), so we can wrap it to prevent the errors later. On the other hand, if there
+                // are too little arguments passed, it will blow up just a line later.
+                $func = self::wrapInternalCallable($func);
                 $func($value, $key);
-            }
-        } finally {
-            if ($discard) {
-                $this->discard();
             }
         }
     }
+
+    private static function wrapInternalCallable(callable $func): callable
+    {
+        return static fn($value) => $func($value);
+    }
 }

tests/EachTest.php

@@ -20,10 +20,13 @@
 
 namespace Tests\Pipeline;
 
+use ArgumentCountError;
 use LogicException;
 use PHPUnit\Framework\TestCase;
 use Pipeline\Standard;
 use ArrayIterator;
+use SplQueue;
+use Tests\Pipeline\Fixtures\CallableThrower;
 
 use function Pipeline\map;
 use function Pipeline\take;
@@ -158,4 +161,58 @@ public function testNotVoidReturn(): void
 
         $this->addToAssertionCount(1);
     }
+
+    public function testStrictArity(): void
+    {
+        $queue = new SplQueue();
+        $pipeline = fromArray([1, 2, 3]);
+        $pipeline->each($queue->enqueue(...));
+
+        $this->assertSame([1, 2, 3], take($queue)->toList());
+    }
+
+    public function testVariadicInternal(): void
+    {
+        $this->expectOutputString("123");
+
+        $pipeline = fromArray(['1', '2', '3']);
+        $pipeline->each(printf(...));
+    }
+
+    public function testVariadicInternalOnIterator(): void
+    {
+        $this->expectOutputString("123");
+
+        $pipeline = take(new ArrayIterator(['1', '2', '3']));
+        $pipeline->each(printf(...));
+    }
+
+    public function testArgumentCountError(): void
+    {
+        $pipeline = fromArray(['1', '2', '3']);
+
+        $this->expectException(ArgumentCountError::class);
+        $this->expectExceptionMessage('Too few arguments');
+        $pipeline->each(static function ($a, $b, $c): void {});
+    }
+
+    /**
+     * Test that the reassignment of the callable inside the loop will affect all iterations.
+     */
+    public function testCallableReassigned(): void
+    {
+        $callback = new CallableThrower();
+
+        $pipeline = fromArray(['1', '2', '3']);
+        $pipeline->each($callback);
+
+        $this->assertSame(4, $callback->callCount, 'Expected 1 initial call that throws + 3 successful calls after wrapping');
+
+        $this->assertSame([
+            ['1', 0],
+            ['1'],
+            ['2'],
+            ['3'],
+        ], $callback->args);
+    }
 }

tests/Fixtures/CallableThrower.php

@@ -0,0 +1,38 @@
+<?php
+
+/**
+ * Copyright 2017, 2018 Alexey Kopytko <[email protected]>
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+namespace Tests\Pipeline\Fixtures;
+
+use ArgumentCountError;
+
+class CallableThrower
+{
+    public array $args = [];
+    public int $callCount = 0;
+
+    public function __invoke(...$args): void
+    {
+        $this->args[] = $args;
+        $this->callCount++;
+
+        // @phpstan-ignore-next-line
+        if (1 === $this->callCount) {
+            throw new ArgumentCountError();
+        }
+    }
+}

tests/Inference/TypeInferenceTest.php

@@ -127,6 +127,7 @@ public function testExtractFixtureNamesFromTests(): void
         ;
 
         /** @var array<string, string> $result */
-        $this->assertSame(['Foo' => 'Foo'], $result);
+        $this->assertArrayHasKey('Foo', $result);
+        $this->assertSame('Foo', $result['Foo']);
     }
 }

tests/TypeAnnotationConsistencyTest.php tests/TypeConsistencyTest.phptests/TypeAnnotationConsistencyTest.php renamed to tests/TypeConsistencyTest.php

@@ -36,7 +36,7 @@
  *
  * @internal
  */
-class TypeAnnotationConsistencyTest extends TestCase
+class TypeConsistencyTest extends TestCase
 {
     /**
      * @dataProvider providePublicMethods