On Tue, Apr 7, 2020 at 2:44 PM Nikita Popov <nikita....@gmail.com> wrote:
> Hi internals, > > It's been a few years since I originally brought up the named parameters > RFC: https://wiki.php.net/rfc/named_params > > This topic has recently come up as part of > https://externals.io/message/109220 again, in particular with the > observation that the combination of constructor parameter promotion and > named parameters will give us, as a side-effect, an ergonomic object > initialization syntax that is well-integrated with the remainder of the > language. > > I've reimplemented basic named param support for master in > https://github.com/php/php-src/pull/5357, but before going any further > here, I want to restart discussion on the various open issues that the > named parameter concept in PHP has. I think there's two primary issues: > > ## LSP checks for parameter names > > Parameter names currently have no particular significance in PHP, only > their position in the signature is important. If named parameters are > introduced (in a way that does not require opt-in at the declaration-site), > then parameter names become significant. This means that changing a > parameter name during inheritance constitutes an LSP violation (for > non-constructor methods). There are a number of ways in which this issue > may be approached: > > 1. Silently ignore the problem. No diagnostic is issued when a parameter > name is changed during inheritance. An error exception will be thrown when > trying to use the (now) unknown parameter name. > > 2. Throw a notice if a parameter name is changed. While LSP violations are > normally fatal errors (in PHP 8), we could use a lower-severity diagnostic > for this case, that allows code to still run, but makes developers aware of > the problem. (It should be noted that automatic fixup of parameter names > using tooling should be fairly easy to implement.) > > 3. Allow using parameter names from the parent method, even if they have > been renamed. This makes things "just work", but seems fairly magic, and > has edge cases like a signature foo($a, $b) being changed to foo($b, $a), > where it's not possible to implicitly support both at the same time. > > 4. Make named-parameter opt-in in some fashion, so that parameter names > only need to be preserved for methods that have the opt-in marker. I'm not > a fan of this, as it greatly diminishes the practical usefulness of named > parameters. > I've done some due diligence on this issue, and checked how other languages with named parameters handle this, and also did some quick checks on what the fallout looks like if we add a notice. ## C# C# does something fairly peculiar. Changing the parameter name introduces a new Schrödinger's overload: We use "new" here to indicate that the parent method is hidden, and it would be if we invoked positionally, but we can actually still access the parent method by performing the call with the old parameter name. This seems pretty weird to me. ``` class Test1 { public void test(int x) { Console.WriteLine("X: {0}", x); } } class Test2 : Test1 { public new void test(int y) { Console.WriteLine("Y: {0}", y); } } public class Program { public static void Main() { Test2 test2 = new Test2(); test2.test(y: 42); // Y: 42 test2.test(x: 42); // X: 42 } } ``` ## Python Unsurprisingly, Python just ignores the problem. If you use the name from the parent, you get an error. ``` class Test1: def test(self, x): print('X:', x); class Test2(Test1): def test(self, y): print('Y:', y); test2 = Test2(); test2.test(y=42); // Y: 42 test2.test(x=42); // TypeError: test() got an unexpected keyword argument 'x' ``` ## Ruby Similarly, Ruby also ignores the issue. It should be noted though that Ruby distinguishes keyword arguments from positional arguments, so you need to opt-in to their use (and if you do, they must be used at the call-site). ``` class Test1 def test(x:) print('X: ', x) end end class Test2 < Test1 def test(y:) print('Y: ', y) end end test2 = Test2.new; test2.test(y: 42) // Y: 42 test2.test(x: 42) // missing keyword: y ``` ## Kotlin Kotlin does what is reasonable: There is a warning about the parameter name change at the declaration-site, and a compile-error at the call-site. ``` open class Test1 { open fun test(x: Int) { println("X: $x") } } class Test2 : Test1() { // Warning: the corresponding parameter in the supertype 'Test1' is named 'x'. // This may cause problems when calling this function with named arguments. override fun test(y: Int) { println("Y: $y") } } fun main() { val test2 = Test2() test2.test(y=42); // Y: 42 // Error: Cannot find a parameter with this name: x test2.test(x=42); } ``` ## Swift Swift behaves somewhat similarly to C#, in that by default changing the parameter name introduces a new overload. In this case it's a proper overload though, as named parameters must be used at the call-site, so there is no method hiding going on. ``` class Test1 { func test(x: Int) { print("X: ", x) } } class Test2 : Test1 { func test(y: Int) { print("Y: ", y) } } let test2 = Test2() test2.test(x: 42) test2.test(y: 42) ``` When specifying that an override is desired instead, an error is generated: ``` class Test2 : Test1 { // error: argument labels for method 'test(y:)' // do not match those of overridden method 'test(x:)' override func test(y: Int) { print("Y: ", y) } } ``` Swift also distinguishes between the internal and the exernal parameter names, so it's possible to rename the internal name, while keeping the external one: ``` class Test2 : Test1 { override func test(x y: Int) { print("Y: ", y) } } ``` ## Internal methods When adding a notice for changed parameter names, methods defined by bundled extensions are mostly clean, with one notable exception: > PHP Notice: Parameter $offset of ArrayAccess::offsetExists() has been renamed to $object in WeakMap::offsetExists(), which may break named parameters in Unknown on line 0 > PHP Notice: Parameter $offset of ArrayAccess::offsetGet() has been renamed to $object in WeakMap::offsetGet(), which may break named parameters in Unknown on line 0 > PHP Notice: Parameter $offset of ArrayAccess::offsetSet() has been renamed to $object in WeakMap::offsetSet(), which may break named parameters in Unknown on line 0 > PHP Notice: Parameter $offset of ArrayAccess::offsetUnset() has been renamed to $object in WeakMap::offsetUnset(), which may break named parameters in Unknown on line 0 We get similar warnings for CachingIterator, ArrayObject, ArrayIterator, SplDoublyLinkedList, SplFixedArray, SplObjectStorage, Phar and PharData. The commonality is always that ArrayAccess parameters are renamed. > PHP Notice: Parameter $position of SeekableIterator::seek() has been renamed to $line_pos in SplFileObject::seek(), which may break named parameters in Unknown on line 0 This is the only occurrence that is not related to ArrayAccess. ## Userland methods When testing PHP-Parser, there are only two warnings. One comes from generated PhpUnit mocks, where codegen would just need to be fixed: > Parameter $invocationRule of PHPUnit\Framework\MockObject\MockObject::expects() has been renamed to $matcher in Mock_Parser_eaee5d81::expects(), which may break named parameters The other one is a renamed parameter in my own code: > Parameter $node of PhpParser\NodeVisitorAbstract::enterNode() has been renamed to $origNode in PhpParser\NodeVisitor\CloningVisitor::enterNode(), which may break named parameters To test a larger project, here are warnings during Laravel startup: https://gist.github.com/nikic/c624399659ef1052e8751d8bb3024bc4 There quite a few more of these. The majority of the warnings occurs because an internal method parameter was renamed, not a userland parameter. I haven't checked whether the parameters match docs or not here. >From the non-internal cases, one that stood out is: > Notice: Parameter $files of Orchestra\Testbench\Contracts\TestCase::call() has been renamed to $cookies in Orchestra\Testbench\TestCase::call(), which may break named parameters in /home/nikic/repos/framework/vendor/orchestra/testbench-core/src/TestCase.php on line 15 > Notice: Parameter $server of Orchestra\Testbench\Contracts\TestCase::call() has been renamed to $files in Orchestra\Testbench\TestCase::call(), which may break named parameters in /home/nikic/repos/framework/vendor/orchestra/testbench-core/src/TestCase.php on line 15 > Notice: Parameter $content of Orchestra\Testbench\Contracts\TestCase::call() has been renamed to $server in Orchestra\Testbench\TestCase::call(), which may break named parameters in /home/nikic/repos/framework/vendor/orchestra/testbench-core/src/TestCase.php on line 15 > Notice: Parameter $changeHistory of Orchestra\Testbench\Contracts\TestCase::call() has been renamed to $content in Orchestra\Testbench\TestCase::call(), which may break named parameters in /home/nikic/repos/framework/vendor/orchestra/testbench-core/src/TestCase.php on line 15 That looks like an actual bug where the signature went out of sync. Illuminate defines: > public function call($method, $uri, $parameters = [], $cookies = [], $files = [], $server = [], $content = null) while Orchestra defines: > public function call($method, $uri, $parameters = [], $files = [], $server = [], $content = null, $changeHistory = true); Note that the $cookies parameter went missing. ## Conclusion >From other languages, we have: * Python, Ruby: Parameter name changes ignored, throw error on call. * C#, Swift: Parameter name changes introduce new overload. (Unless "override" is used, in which case it is an error.) * Kotlin: Warns on parameter name change, error on call. >From the warnings observed in practice, I think the ArrayAccess case is an interesting one, in that it's pretty unlikely that those methods will ever be called with named parameters (usually they are not explicitly called at all!) I think we might ultimately be best off with going for the Python/Ruby approach, at least initially, and encourage IDEs / static analyzers / style analyzers to offer inspections that check whether parameter names match. As we are retrofitting named parameters to a language where parameter names held no significance historically, being overly pedantic about this might not do us any favors, especially if we consider that languages like Python do get away with not enforcing anything in this regard. ## Internal functions > > There are two problems when it comes to named parameters and internal > functions. The first is that the internal parameter names do not always > match the parameter names in the documentation. This is nothing we can't > solve (in a hopefully mostly automated way), but is work that needs to be > done. > > The larger problem is that internal functions don't have a well-defined > concept of parameter default value. Parameter defaults are implicit in C > code, and sometimes based on argument count checks. When named parameters > are involved, one generally cannot assume that if a later (optional) > parameter is passed, all earlier (optional) parameters are passed as well. > > While it is possible (but rather challenging) to make things work mostly > transparently with current ZPP, some functions do need to adjusted to > support named parameters, and may cause misbehavior/crashes if they are > not. As a rule of thumb, functions that make non-trivial use of > ZEND_NUM_ARGS() are affected. This is something we can address for > functions in bundled extensions, but may also affect 3rd-party extensions. > > I think that ideally, we'd deal with internal functions the same way we do > for userland functions, by relying on default values that are part of the > function declaration. As part of the PHP stub work, we now have information > on default values in stub files, but this information is currently not > available to the engine. > > Máté Kocsis has been working on getting this information exposed in > Reflection, see https://github.com/php/php-src/pull/5353. In the current > form, accessing these defaults is fairly inefficient, but maybe with some > caching, we could base named parameters on this information: > > If there are no "gaps" in the passed parameters, then named parameters > will always work. If an optional parameter is not specified, but a later > parameter is, then we will fetch the default value for that parameter, and > pass it instead. If the parameter is optional, but has no well-defined > default (UNKNOWN in stubs), then the call is not permitted (Error: "Cannot > skip parameter $foo with unknown default value", or similar.) > For the record, internal function default values are now available via Reflection, so the technical groundwork to go down this path is present now. Regards, Nikita
