squizlabs/PHP_CodeSniffer

PHP 8.0 | File::getMethodParameters() needs to support attributes

jrfnl opened this issue · 6 comments

jrfnl commented

Describe the bug
Parameters in a function declaration can have attributes attached to them. This is currently not handled correctly in the File::getMethodParameters() method.

Code sample

class ParametersWithAttributes(
    public function __construct(
        #[\MyExample\MyAttribute] private string $constructorPropPromTypedParamSingleAttribute,
        #[MyAttr([1, 2])]
        Type|false
        $typedParamSingleAttribute,
        #[MyAttribute(1234), MyAttribute(5678)] ?int $nullableTypedParamMultiAttribute,
        #[WithoutArgument] #[SingleArgument(0)] $nonTypedParamTwoAttributes,
        #[MyAttribute(array("key" => "value"))]
        &...$otherParam,
    ) {}
}

For the above code, the T_STRING tokens from within the attribute would be added to the type_hint array key in the return value, so for example, for the first parameter, the type_hint would (incorrectly) come back as 'type_hint' => '\MyExample\MyAttributestring'

Proposed behavior
I'm currently looking into fixing this.

My current thinking hinges on four possible "solutions".

  1. Ignore attributes in function declarations altogether. Just skip over them.
  2. Add an attributes index key to the array for each parameter with a boolean flag to indicate whether there are attribute(s) attached to the parameter. This index key would always be set and defaults to false.
    The content key would remain the same as it is now and not contain the attribute(s).
    This option will give sniff writers an indication whether attributes are attached to the parameter. If the sniff writer would need the attribute details, they can do a findPrevious() for an T_ATTRIBUTE_END token before 'token' and walk the attribute tokens from there.
  3. Add three new index keys to the array for each parameter which would only be set when there are attribute(s) attached to the parameter.
    The content key would now also contain the complete attribute(s).
    • attributes containing a string representation of the attribute(s).
    • attributes_start containing the stack pointer to the first attribute opener for this parameter
    • attributes_end containing the stack pointer to the last attribute closer for this parameter.
  4. Add a new multi-level array index key attributes, which would only be set when there are attribute(s) attached to the parameter. The array format would be along the lines of the below.
    The content key would now also contain the complete attribute(s). 
    'attributes' => [
    [0] => [
        - `attribute` containing a string representation of the first attribute.
        - `attribute_start` containing the stack pointer to the attribute opener for the first attribute.
        - `attribute_end` containing the stack pointer to the attribute closer for the first attribute.
    ],
    [1] => ...
]

I'm personally leaning towards either option 1 or option 2.

I don't think option 3 is a good solution as there can be multiple attributes attached to the parameter and that solution does not do that justice.

As for option 4, I'm not so sure that this is really needed as sniffs examining attributes can just listen to T_ATTRIBUTE instead and for sniffs specifically only examining attributes in function declarations, having the attributes indicator (option 2) or walking the tokens between the open/close parentheses looking for T_ATTRIBUTE tokens, is probably enough and would prevent the performance hit of processing potential attributes on each call to File::getMethodParameters().

@gsherwood Your input/opinion on which solution is preferred would be much appreciated.

Versions (please complete the following information):

  • PHPCS: master
jrfnl commented

Hmm.. thinking it over some more, I'm not sure attributes should be excluded from the content as comments in a parameter declaration aren't excluded either.

gsherwood commented

Hmm.. thinking it over some more, I'm not sure attributes should be excluded from the content as comments in a parameter declaration aren't excluded either.

Before reading this comment, I was learning towards option 1. Do you have an example of where comments are included and do you think they should be?

jrfnl commented

Do you have an example of where comments are included ?

@gsherwood Comments are always included in the content (which IMO is probably the least used and least useful of the return indexes) as getTokensAsString() is used:

PHP_CodeSniffer/src/Files/File.php

Line 1496 in 342dbf2

$vars[$paramCount]['content'] = trim($this->getTokensAsString($paramStart, ($i - $paramStart)));

Test which shows this:

PHP_CodeSniffer/tests/Core/File/GetMethodParametersTest.inc

Line 90 in 342dbf2

function duplicateTypeInUnion( int | string /*comment*/ | INT $var) {}

PHP_CodeSniffer/tests/Core/File/GetMethodParametersTest.php

Line 637 in 342dbf2

'content' => 'int | string /*comment*/ | INT $var',

Test which I could add to demonstrate and document this behaviour even better:

function commentsInParams(
	// Leading comment.
	?MyClass /*-*/ & /*-*/.../*-*/ $param /*-*/ = /*-*/ 'default value' . /*-*/ 'second part' // Trailing comment.
) {}
    /**
     * Verify behaviour when there are comments within a parameter declaration.
     *
     * @return void
     */
    public function testCommentsInParameter()
    {
        $expected    = [];
        $expected[0] = [
            'name'                => '$param',
            'content'             => '// Leading comment.
    ?MyClass /*-*/ & /*-*/.../*-*/ $param /*-*/ = /*-*/ \'default value\' . /*-*/ \'second part\' // Trailing comment.',
            'attributes'          => false,
            'pass_by_reference'   => true,
            'variable_length'     => true,
            'type_hint'           => '?MyClass',
            'nullable_type'       => true,
        ];

        $this->getMethodParametersTestHelper('/* '.__FUNCTION__.' */', $expected);

    }//end testCommentsInParameter()

... do you think they should be?

Well, changing that behaviour now would be a BC-break and what with them being included, it makes sense to also include potential attributes in the value of the content key.

So I'm tempted to go for option 2, but with the attributes being included in content.

jrfnl commented

@gsherwood Had a chance to think this over some more ?

gsherwood commented

Had a chance to think this over some more ?

I think you're 100% right in your previous comment. The attributes should be included in content to remain consistent.

The extra flag to indicate they are present is a nice to have, but hard to know how useful it is during sniff development right now (unless you need it already). Happy to have it there if you feel it is useful.

jrfnl commented

@gsherwood Thanks for letting me know. I've pulled the fix now in #3320.