Use different warning and errors for namespaced names

[rjd22]

This implementation returns a different warning or error when the line is too long because of namespaced names. By using a different warning or error they can be excluded by using the following excludes.

<rule ref="Generic.Files.LineLength">
    <exclude name="Generic.Files.LineLength.NamespacedNameTooLong"/>
    <exclude name="Generic.Files.LineLength.NamespacedNameMaxExceeded"/>
</rule>

I flagged this a breaking change for the following reason:
When user excludes only the warning for line length. Their build would now break because a new warning is introduced.

Suggested changelog entry

Use NamespacedNameTooLong warning & NamespacedNameMaxExceeded error when a namespaced name LineLength is too long or exceeded max length.

Related issues/external references

Fixes #1365

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.
• I have opened a sister-PR in the documentation repository to update the Wiki.

[rodrigoprimo]

@rjd22, I took the liberty of reviewing this PR even though I'm not a maintainer of this project. So take that into account while checking what I said, as @jrfnl may disagree with some of my remarks.

I didn't do a full code review, but I have some broad remarks about the changes proposed in this PR.

Condition doesn't match the original suggestion

Correct me if I'm wrong, but I believe the PR checks whether any namespaced name token exists on a line that exceeds one of the limits, regardless of the token's length. Consider:

 $result = \Short\Ns\doSomething($arg1, $arg2, $arg3, $arg4, $arg5, $arg6, $arg7);

That line triggers the Generic.Files.LineLength.NamespacedNameTooLong warning, even though the long line is not caused by a long namespace name, and it can be split into multiple lines.

What I understood from @jrfnl's suggestion is that the error code should only change when the namespaced name token itself exceeds one of the limits.

Performance concern with the new method

isLineNamespacedName() uses array_filter() on the entire token stack for every long line found, and I don't think this is necessary. I guess it would be more efficient to iterate backwards from $stackPtr through only the tokens on the current line and check whether any is a namespaced name token?

Tests placement

I suggest adding the tests to LineLengthUnitTest.1.inc rather than creating a new file. My understanding is that the main file contains most of the tests for a given sniff, unless there is a reason to put them in a separate file (typically, tests with intentional syntax errors or tests that need to be run in a separate file).

[rodrigoprimo]

Just noting here that I chatted briefly with @jrfnl about the performance concern I raised in my review, and she suggested an approach different than the one that I suggested. She pointed out that since the sniff already walks the complete file, adding some code to keep track of the last "namespace name" token seen should be sufficient (or track "namespaced name" tokens on the current line).

[rjd22]

Just noting here that I chatted briefly with @jrfnl about the performance concern I raised in my review, and she suggested an approach different than the one that I suggested. She pointed out that since the sniff already walks the complete file, adding some code to keep track of the last "namespace name" token seen should be sufficient (or track "namespaced name" tokens on the current line).

I would say that this approach would be more expensive then only looking back the stack and when line limit is exceeded. The reason being that you will need to check every token for it's type (this is more expensive than checking when line length is exceeded, and breaking out of the loop when line doesn't match anymore).

ATM, I'm doing working on a lookback since it's also the easiest to reason about and has the advantage to be a bit more adaptable.

[rjd22]

@rodrigoprimo thanks for your review. I made some changes based on your advice. I do feel the test separation is valid. My reasoning being that the change on validating namespaced names diverts from the default validation and most likely we want to make even more checks for this. Putting al those in the main file would muddy the waters with the default tests of LineLengthSniff.

[rodrigoprimo]

Thanks for your continued work on this PR and for considering my suggestions, @rjd22!

I left some inline comments for you to take a look at and see what you think.

ATM, I'm doing working on a lookback since it's also the easiest to reason about and has the advantage to be a bit more adaptable.

I don't have a strong opinion on which approach is better, and the lookback approach seems good to me. That said, let's see what @jrfnl thinks when she reviews the updated code, as she may still prefer the tracking approach for some reason.

I do feel the test separation is valid. My reasoning being that the change on validating namespaced names diverts from the default validation and most likely we want to make even more checks for this. Putting al those in the main file would muddy the waters with the default tests of LineLengthSniff.

I understand your point about keeping namespace-related tests grouped separately. However, my understanding is that every additional test case file adds overhead due to setup/tokenizing, so extra files should only be added when needed to test a particular aspect of the sniff. See what @jrfnl said in a previous PR review and the guidelines in the CONTRIBUTING.md file about when to use multiple test case files.

[rodrigoprimo]

Now that the method changed the description needs to be updated.

[rodrigoprimo]

I think this is still not correct. The code checks whether the token's end column position exceeds $lineLimit, rather than checking if the token's own length exceeds $lineLimit. This means that something like the code below will trigger the NamespacedNameTooLong warning, while my understanding from the discussion in the issue is that it shouldn't:

$result = doSomething($arg1, $arg2, $arg3, $arg4, $arg5, $arg6, \Short\Ns\myFunction($arg7));

I believe the code should check simply if the length of the current token exceeds $lineLimit when the token is one of the namespaced name tokens.

[rodrigoprimo]

This comment doesn't seem accurate.

[rodrigoprimo]

This is a bit of an edge case, but I think the code should protect against it. Consider the following code (note that the code is on the first line together with the PHP opening tag):

<?php $result = doSomething($arg1, $arg2, $arg3, $arg4, $arg5, $arg6, \Short\Ns\myFunction($arg7));

It generates two warnings as this loop tries to access $tokens[-1]:

Undefined array key -1
Trying to access array offset on null

[rodrigoprimo]

I would suggest naming $stackPtr to something else that better describes what this variable holds. $stackPtr is typically used to refer to the position of the token that triggered the execution of the sniff code in the $tokens array. In the case of this sniff, checkLineLength() already uses this variable name to refer to something else, which I find confusing, but maybe is the reason why you opted to use this name in this method as well.

[rodrigoprimo]

A few thoughts on the tests:

Some of the test cases seem redundant to me. For example, lines 13-15 test use, use function, and use const, but from the sniff's perspective, all three produce the same T_NAME_QUALIFIED token and exercise the same code path. From my perspective, just one of those tests would be sufficient. This also applies to other test "groups" in this file.

On the other hand, some scenarios that exercise different code paths are missing. For example:

• Tests using the T_NAME_RELATIVE token. The sniff checks for this type, but no test covers it.
• A namespace token whose length is exactly at the $lineLimit boundary (if there is indeed no test that matches the $lineLimit boundary, I might have missed it).

Maybe there could be more variation as well using namespace tokens in other contexts (like type hints, instanceof, or catch blocks).