Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Generic/DuplicateClassName: 4 bug fixes + performance improvement #653

Merged

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Oct 30, 2024

Description

Generic/DuplicateClassName: bug fix - ignore namespace operator

The namespace keyword can also be used as an operator, in which case, the sniff as it were would throw false positives.

The fix now applied for this incidentally also fixes a nasty bug which could throw the sniff into an eternal loop when an unfinished namespace statement was encountered during live coding.

Includes unit tests.

Note: the unit test for the live coding situation has to be the last unit test file for this sniff so as to not influence the other tests. To that end, it has been numbered 99.

Generic/DuplicateClassName: bug fix - ignore comments in namespace declarations

A comment in a namespace declaration statement would throw the sniff off, leading to false positives and false negatives.

Fixed now.

Includes unit tests. Additionally, a unit test for classes declared within a scoped global namespace has also been added.

Generic/DuplicateClassName: bug fix - namespace is reset on PHP open tag

As things were, the sniff listens to the T_OPEN_TAG token, processes the file until the first T_CLOSE_TAG and then waits again for a new T_OPEN_TAG.

However, every time the sniff is called, the namespace is reset, even when still processing the same file, i.e. when the namespace is still in effect.

This led to both false positives as well as false negatives.

Fixed now, by always processing the complete file in one go and not returning on a T_CLOSE_TAG.

Includes unit tests.

Generic/DuplicateClassName: use helper method

Use the File::getDeclarationName() method instead of finding the name of an OO structure in the sniff itself.

This also prevents a potential situation where if a class name could not be found, the findNext() method would return false, which would then be used in the $tokens[$nameToken]['content'] leading to false being juggled to 0 and a class name of <?php being stored in the class name cache.

Includes tests.

Generic/DuplicateClassName: performance fix

As things were, the sniff would unconditionally walk a complete file, making it one of the top 15 slowest sniffs.

However, OO structures in PHP cannot be nested, so once we've found an OO declaration, we can skip to the end of it before continuing the token walking. See: https://3v4l.org/pbSTG

This small tweak makes a significant difference in the sniff performance without any impact on the sniff results.

Suggested changelog entry

  • Fixed Generic.Classes.DuplicateClassName : the sniff did not skip namespace keywords used as operators, which could lead to false positives.
  • Fixed Generic.Classes.DuplicateClassName : sniff going into an infinite loop during live coding.
  • Fixed Generic.Classes.DuplicateClassName : false positives/negatives when a namespace declaration contained whitespace or comments in unconventional places.
  • Fixed Generic.Classes.DuplicateClassName : namespace for a file going in/out of PHP was not be remembered/applied correctly.
  • Performance improvement for Generic.Classes.DuplicateClassName

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

The `namespace` keyword can also be used as an operator, in which case, the sniff as it were would throw false positives.

The fix now applies for this incidentally also fixes a nasty bug which could throw the sniff into an eternal loop when an unfinished namespace statement was encountered during live coding.

Includes unit tests.

**Note**: the unit test for the live coding situation has to be the last unit test file for this sniff so as to not influence the other tests. To that end, it has been numbered `99`.
…clarations

A comment in a namespace declaration statement would throw the sniff off, leading to false positives and false negatives.

Fixed now.

Includes unit tests. Additionally, a unit test for classes declared within a scoped global namespace has also been added.
As things were, the sniff listens to the `T_OPEN_TAG` token, processes the file until the first `T_CLOSE_TAG` and then waits again for a new `T_OPEN_TAG`.

However, every time the sniff is called, the namespace is reset, even when still processing the same file, i.e. when the namespace is still in effect.

This led to both false positives as well as false negatives.

Fixed now, by always processing the complete file in one go and not returning on a `T_CLOSE_TAG`.

Includes unit tests.
Use the `File::getDeclarationName()` method instead of finding the name of an OO structure in the sniff itself.

This also prevents a potential situation where if a class name could not be found, the `findNext()` method would return `false`, which would then be used in the `$tokens[$nameToken]['content']` leading to `false` being juggled to `0` and a class name of `<?php` being stored in the class name cache.

Includes tests.
As things were, the sniff would unconditionally walk a complete file, making it one of the top 15 slowest sniffs.

However, OO structures in PHP cannot be nested, so once we've found an OO declaration, we can skip to the end of it before continuing the token walking. See: https://3v4l.org/pbSTG

This small tweak makes a significant difference in the sniff performance without any impact on the sniff results.
@jrfnl jrfnl added this to the 3.11.0 milestone Oct 30, 2024
@jrfnl jrfnl changed the title Generic/DuplicateClassName: 4 bug fixes + performance i Generic/DuplicateClassName: 4 bug fixes + performance improvement Oct 30, 2024
@jrfnl jrfnl merged commit f145bb4 into master Nov 2, 2024
52 checks passed
@jrfnl jrfnl deleted the feature/generic-duplicateclassname-fix-bugs-performance branch November 2, 2024 08:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant