PHP8: Named parameters support & opt-out attribute

[asgrim]

Since it looks like named parameters will pass, with no proposal of having a way to opt out of this feature, I'd like to propose introducing an attribute to have parameter names excluded from BCC rules, by way of annotation.

The main comment I've seen in support of named parameters is that "you can just say you're not supporting named parameters". Unfortunately, just saying that (e.g. by writing in your README.md) doesn't make tools understand that fact, so we need to recognise this some other way. The RFC even goes as far as to mention an "opt-out" possibility, but it never expands on it beyond suggesting an annotation such as <<NoNamedArgs>>. Therefore, unless another RFC is proposed to add this, we can be fairly sure that there will be no runtime checks to prevent calling a method with named-parameter syntax.

So: onto the proposal: Add an annotation to method definition that says "you must not call this method using named parameter syntax". Static analysers such as Psalm/PHPStan could check this at call-time: I believe Psalm "may" support this if there is demand, and PHPStan are actively looking to support this kind of annotation.

Because there is chance for BC break by renaming parameters with the named param RFC (even if a rename is rare), we will need to add this as a rule now from any PHP 8 code. However, we can use the above annotation to exclude any parameter renames from being marked as BC breaks within this tool.

Before API:

function supportsNamedParams($a, $b, $c);

@@NoNamedParams
function doesNotSupportNamedParams($a, $b, $c);

After API

function supportsNamedParams($d, $e, $f); // would be marked as a BC break due to named params in PHP 8

@@NoNamedParams
function doesNotSupportNamedParams($d, $e, $f); // param renames are ignored due to annotation, not classed as a BC break

[ondrejmirtes]

Hi,
PHPStan could definitely disallow using named parameters for functions/methods marked with @@NoNamedParams but it might not even be necessary - PHPStan will tell you if you're using a named parameter that doesn't exist after upgrading the library so you could make the inverted argument - "thanks to using PHPStan, I can use named parameters safely because I'll know when the library breaks them". WDYT?

Another use-case to have in mind is with inheritance - there it makes sense to report using named parameters on @@NoNamedParams because of:

1. Class B extends A
2. Class A has method foo($a, $b, $c) with @@NoNamedParams
3. B::foo() has $d, $e, $f
4. Calls to A::foo() are potentially dangerous because B might get injected there with different named parameters.

[asgrim]

Thanks for the feedback @ondrejmirtes . I think if we're agreed on @@NoNamedParams as the attribute name then we can adapt this here to identify BC breaks (or not, as the case may be) too 👍

[Ocramius]

Just to be clear: the default will be to prevent renaming of arguments, as that's what the PHP baseline will do.

That will require implementing in this library, and being put in the default config (for now)

[muglug]

Realising the risk that named parameters might inflict serious irritation in some quarters, I'm now fully on board with the use of this annotation.

But I also think that /** @NoNamedParams */ should be supported by SA tools too, to allow library maintainers to prevent improper use of their code today.

[muglug]

I'm also thinking of adding a config flag to allow the user to tell Psalm to prevent calling any internal methods in the given project with named parameters (so @NoNamedParams is assumed anywhere where @internal is given)

[Ocramius]

I'm also thinking of adding a config flag to allow the user to tell Psalm to prevent calling any internal methods in the given project with named parameters (so @NoNamedParams is assumed anywhere where @internal is given)

This is probably a good default, rather than a setting - suppressing issues around this is better than configuring the tool to pick it up opt-in.

In order to avoid conflicting with doctrine annotations or such, maybe @no-named-params?

[muglug]

I'm going with @no-named-arguments as that's really what this should prevent

[asgrim]

Alright, we'll adapt this as part of our PHP 8 support too then; Param name changes will be ignored as BC breaks if @no-named-arguments is used 👍

[sebastianbergmann]

👍 from me. I will not make make parameter names part of any backward compatbility promise (I think passing arguments to a function based on the parameter names is a bad idea) and having this tool not yell at me when I change parameter names would be much appreciated.

[Ocramius]

Ok, so I guess the only thing that's left is implementation: we'll go with @no-named-arguments

[sebastianbergmann]

Ok, so I guess the only thing that's left is implementation: we'll go with @no-named-arguments

Just to make sure: both Psalm and BackwardCompatibilityCheck (will) use @no-named-arguments when it is used in a class-level docblock for all methods of that class, right?

[asgrim]

Just to make sure: both Psalm and BackwardCompatibilityCheck (will) use @no-named-arguments when it is used in a class-level docblock for all methods of that class, right?

Seems reasonable, I'll check it when I come to implementing it :)👍

[muglug]

Yup, so will I

[TysonAndre]

How should this work with inheritance? Should it only be limited to methods defined (or inherited) in the class using no-named-arguments, and possibly methods of the same name in subclasses?

/** @no-named-arguments */
class TestCase {
    use SomeTrait{ foo as otherFoo };
    public function assertSame($expected, $actual);
}

class UnrelatedProjectTest extends TestCase {
     public function helper(bool $flag = false, bool $other_flag = true) {
     }

     public function testSomething() {
           $this->assertSame(expected: 1, actual: someFunction());  // should warn?
           $this->helper(other_flag: true);  // should not warn?
           $this->otherFoo(other_flag: true);  // should warn?
     }
}

[Ocramius]

IMO it should be inherited - the first declaration can change at any time, which is what this annotation is about.

In addition to that, PHP raises errors on named params mismatch, so it's a bit of a "we told you so" from the parent implementation :-)

[Ocramius]

Not going to be able to drag this into 6.0 - removing milestone for now.