PHPCSStandards / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.

BSD 3-Clause "New" or "Revised" License

805 stars 47 forks source link

Tokenizer/PHP: fix handling of "DNF look-a-likes" in named parameters #507

Closed jrfnl closed 1 month ago

jrfnl commented 1 month ago

Description

The last parameter in a function call using named arguments could be confused with a return type by the tokenizer layer handling type declarations.

The net effect of this was that the close parenthesis of the function call would be retokenized to T_TYPE_CLOSE_PARENTHESIS, which is incorrect and would lead to sniffs incorrectly acting on that information.

Fixed now.

Includes tests.

Suggested changelog entry

The tokenizer could inadvertently mistake the last parameter in a function call using named arguments for a DNF type.

Related issues/external references

Fixes #504 Fixes #505

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)

fredden commented 1 month ago

It would be good to get some tests added as part of this to safeguard that | is tokenised as T_BITWISE_OR, and & as T_BITWISE_AND and ) as T_CLOSE_PARENTHESIS when not part of a disjunctive normal form type declaration. The tests being added here only seem to be safeguarding one side of this bug.

jrfnl commented 1 month ago

It would be good to get some tests added as part of this to safeguard that | is tokenised as T_BITWISE_OR, and & as T_BITWISE_AND and ) as T_CLOSE_PARENTHESIS when not part of a disjunctive normal form type declaration. The tests being added here only seem to be safeguarding one side of this bug.

@fredden The test method these test cases feed into already does a check for tokens between the parentheses: https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/8f0d8de9763d99d3801d7d9b7cf303116a739eb4/tests/Core/Tokenizer/PHP/DNFTypesTest.php#L31-L85

Additionally, the original PR added tests with DNF types to both the union type as well as the intersection type test files: https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/461/files#diff-28273165881fe5292d6b2e5599fc0d72b4e80be50e07273d2d96e3a052dfe328

What additional tests did you have in mind ?

jrfnl commented 1 month ago

@fredden Was this what you had in mind ?

fredden commented 1 month ago

@fredden Was this what you had in mind ?

No, but that looks like it covers the concern I had. I was expecting to see some changes to test files named "bitwise" and "parenthesis", but I didn't pause to check what test files already existed. From what I could see, it appeared that this was asserting that the new test cases were not DNF but nothing suggested that they are bitwise operators. I think this is adequately covered now. Thanks for updating this.