Add UnusedUseStatementSniff copied from Drupal Coder

[ExplodingCabbage]

Resolves https://github.com/squizlabs/PHP_CodeSniffer/issues/801

• [x] Code conforms to standard
• [x] Has tests
• [x] Unit tests pass.
• [x] Manually tested against a codebase of mine that was rife with unused use statements
• [x] PHPCS team approves?
• [x] Get approval from @klausi and @alexpott on copyright and licensing before merging

Nicked from https://github.com/klausi/coder with the gracious permission of its authors.

Changes from the original at https://github.com/klausi/coder were:

• Modified UnusedUseStatementUnitTest.inc.fixed to remove the Thing/NotUsed use, instead of modifying the type hint involving it, since this is the automatic fix the Sniff would generate and PHPCS's tests require that 'fixed' files match the output of running the fixer.
• Added file headers in the style of PHPCS's other files
• Modified code formatting to match PHPCS's linting rules

Note that I am not the author of this sniff; @klausi and @alexpott are.

When it comes to crafting the file's PHPDoc header and other copyright issues, although I've mostly just imitated the style of existing sniffs, I had to make a few decisions. I'll list them here so that all parties can confirm they're okay with my choices before this gets merged:

• I didn't bother doing any Git magic to preserve the sniff's original commit history from Drupal coder, because doing so didn't seem important
• I listed Klaus Purer and Alex Pott as authors, with the email addresses that they used for committing on Drupal Coder.
• Since this is Klaus's work but every single other sniff has Squiz listed as a copyright holder, I listed both Klaus Purer and Squiz Pty Ltd (ABN 77 084 670 600) both as copyright holders, in the same style as e.g. Notifysend.php which lists Christian Weiske and Squiz as copyright holders. I'm no expert either in intellectual property law or PHPDoc, but I think this declares that both parties hold full copyright - i.e. that both parties are free to reproduce the work or to license it to others under any licensing terms that they please. @klausi and @alexpott, are you okay with that line being there?

Also, @klausi and @alexpott - you both agreed to relicensing under MIT in https://github.com/squizlabs/PHP_CodeSniffer/issues/801, which Klausi thought PHPCS was licensed under. However, while writing this I saw that all the files actually declare that they are BSD-licensed, not MIT-licensed. @klausi and @alexpott, could you both confirm that you're happy to relicense under BSD?

Assuming everybody's comfortable with the legal stuff, I think this should be good to go.

[alexpott]

I'm happy to relicense under BSD

[ExplodingCabbage]

Looks like this blew up in Travis due to using [] array syntax which wasn't supported in PHP 5.3. Will change it to array() tomorrow.

[klausi]

I'm happy to relicense under BSD. I'm OK with the Squiz Pty Ltd copyright line if it has to be there.

[ExplodingCabbage]

Looking at other PRs that add sniffs, I see that I might want to add docs for this. Will look into doing so at some point.

[ExplodingCabbage]

Added some docs. I think this just needs review from @gsherwood now.

[donatj]

I actually borrowed the sniff from Drupal Coder myself but ran into a number of issues with it

I think it was to do with it not respecting things used by an @var, @param or @return which PHPStorm and Scrutinizer do - which can be seen in my attached diff below.

It was a while ago, so I'm not sure which parts of this diff were hugely important, but I think my noted behaviour above is.

Also the $warning = 'Unused use statement: ' . $className; which prepends the class name is very useful to know which it thinks is not used.

> 
43a45
> 
51,52c53,57
<       $classUsed      = $phpcsFile->findNext(T_STRING, ($classPtr + 1));
<       $lowerClassName = strtolower($tokens[$classPtr]['content']);
---
>       $classUsed      = $phpcsFile->findNext([ T_STRING, T_DOC_COMMENT_STRING ], ($classPtr + 1));
>       $className      = $tokens[$classPtr]['content'];
>       $lowerClassName = strtolower($className);
> 
> 
55,61c60,63
<       $namespacePtr = $phpcsFile->findPrevious(array( T_NAMESPACE ), $stackPtr);
<       // Check if the use statement does aliasing with the "as" keyword. Aliasing
<       // is allowed even in the same namespace.
<       $aliasUsed = $phpcsFile->findPrevious(T_AS, ($classPtr - 1), $stackPtr);
<       if( $namespacePtr !== false && $aliasUsed === false ) {
<           $nsEnd           = $phpcsFile->findNext(
<               array(
---
>       $namespacePtr = $phpcsFile->findPrevious([ T_NAMESPACE ], $stackPtr);
>       if( $namespacePtr !== false ) {
>           $nsEnd     = $phpcsFile->findNext(
>               [
63a66
>                   T_DOC_COMMENT_STRING,
65c68
<               ),
---
>               ],
70,71c73,75
<           $namespace       = trim($phpcsFile->getTokensAsString(($namespacePtr + 1), ($nsEnd - $namespacePtr - 1)));
<           $useNamespacePtr = $phpcsFile->findNext(array( T_STRING ), ($stackPtr + 1));
---
>           $namespace = trim($phpcsFile->getTokensAsString(($namespacePtr + 1), ($nsEnd - $namespacePtr - 1)));
> 
>           $useNamespacePtr = $phpcsFile->findNext([ T_STRING ], ($stackPtr + 1));
73c77
<               array(
---
>               [
76c80
<               ),
---
>               ],
81a86
> 
83c88,89
<               $classUsed = false;
---
>               // This breaks things - I don't know why it exists in the Drupal version
>               // $classUsed = false;
86a93,96
>       $emptyTokens = PHP_CodeSniffer_Tokens::$emptyTokens;
>       unset($emptyTokens[T_DOC_COMMENT_TAG]);
> 
> 
88c98,101
<           if( strtolower($tokens[$classUsed]['content']) === $lowerClassName ) {
---
>           if( ($tokens[$classUsed]['code'] == T_STRING && strtolower($tokens[$classUsed]['content']) === $lowerClassName) ||
>               ($tokens[$classUsed]['code'] == T_DOC_COMMENT_STRING && preg_match('/(\s|\||^)' . preg_quote($lowerClassName) . '(\s|\||$|\[\])/i', $tokens[$classUsed]['content']))
>           ) {
> 
90c103
<                   PHP_CodeSniffer_Tokens::$emptyTokens,
---
>                   $emptyTokens,
95,99d107
<               // If a backslash is used before the class name then this is some other
<               // use statement.
<               if( $tokens[$beforeUsage]['code'] !== T_USE && $tokens[$beforeUsage]['code'] !== T_NS_SEPARATOR ) {
<                   return;
<               }
101,103c109,129
<               // Trait use statement within a class.
<               if( $tokens[$beforeUsage]['code'] === T_USE && empty($tokens[$beforeUsage]['conditions']) === false ) {
<                   return;
---
> 
>               if( $tokens[$classUsed]['code'] == T_STRING ) {
>                   // If a backslash is used before the class name then this is some other
>                   // use statement.
>                   if( $tokens[$beforeUsage]['code'] !== T_USE && $tokens[$beforeUsage]['code'] !== T_NS_SEPARATOR ) {
>                       return;
>                   }
> 
>                   // Trait use statement within a class.
>                   if( $tokens[$beforeUsage]['code'] === T_USE && empty($tokens[$beforeUsage]['conditions']) === false ) {
>                       return;
>                   }
>               } else {
>                   if( $tokens[$beforeUsage]['code'] === T_DOC_COMMENT_TAG && (
>                           $tokens[$beforeUsage]['content'] == '@var' ||
>                           $tokens[$beforeUsage]['content'] == '@param' ||
>                           $tokens[$beforeUsage]['content'] == '@return'
>                       )
>                   ) {
>                       return;
>                   }
107c133
<           $classUsed = $phpcsFile->findNext(T_STRING, ($classUsed + 1));
---
>           $classUsed = $phpcsFile->findNext([ T_STRING, T_DOC_COMMENT_STRING ], ($classUsed + 1));
110c136
<       $warning = 'Unused use statement';
---
>       $warning = 'Unused use statement: ' . $className;

[donatj]

For better clarity, the full text of my variant is here: http://jdon.at/1h0wb

[ExplodingCabbage]

Thanks @donatj, I'll take a look when I have a chance and try to integrate and add tests for your improvements.

I'll need to have a think/read to figure out whether references to classes from within PHPDoc syntax should be treated as a usage of the class always or whether that should be optional via a property - I don't personally know how standardised that syntax is or whether all tools that read it respect use statements. Perhaps you (or other folks here) know better than I do.

[andreasnij]

Great work @ExplodingCabbage and @donatj! I would love to see this merged. I replaced these lines in donatjs code:

if( $tokens[$beforeUsage]['code'] === T_DOC_COMMENT_TAG && (
    $tokens[$beforeUsage]['content'] == '@var' ||
    $tokens[$beforeUsage]['content'] == '@param' ||
    $tokens[$beforeUsage]['content'] == '@return'
)

with:

if ($tokens[$beforeUsage]['code'] === T_DOC_COMMENT_TAG && 
    in_array(
        $tokens[$beforeUsage]['content'], 
        array('@var', '@param', '@return', '@throws', '@method')
    )
)

to make it work with our codebase.

[ExplodingCabbage]

I've returned to this to get it merge-ready, and have made some progress now and will finish tomorrow.

Actions I've taken today:

• Bringing this PR up to date with master and confirming that the tests still pass
• Making the code of the new Sniff mostly conform to the PHPCS coding standard (I can't achieve total compliance because the @license line in the standard docstrings present on every sniff violates the standard's line length limit, causing all sniffs in PHPCS to violate PHPCS's own standard)
• Making the warnings emitted by the sniff contain the class name, as in @donatj's variant.

I still need to pick though the rest of the diff, understand the changes, and implement them if appropriate (possibly adding test cases in the process). A small part of the diff is just changing from array() syntax in the Drupal version to [] syntax in @donatj's version, which is a change I won't preserve since PHPCS supports PHP 5.3 which doesn't support the [] syntax. I think the rest of the changes must relate to class names inside PHPDoc comments; I'll do my best to understand those changes and implement them.

[ExplodingCabbage]

Hmm, some of the stuff in the diff is also clearly the result of @donatj's version being forked off Drupal Coder before some of the most recent commits. For instance, https://github.com/klausi/coder/commit/4283dae86e2ccd8e856e43fa2b8a2ab9488783cb isn't included in @donatj's version. I'll try to figure out exactly when @donatj forked in order to understand which parts of the diff are genuine changes he made and which are the absence of changes that @klausi made.

Edit: alright, easy enough. @donatj forked off https://github.com/klausi/coder/commit/1c8b5a4f6a1c8e2a3a78035272ecd64b8a74eade

[ExplodingCabbage]

A slimmed-down diff with the above point in mind, showing only @donatj's actual changes:

56,57c56,58
<         $classUsed      = $phpcsFile->findNext(T_STRING, ($classPtr + 1));
<         $lowerClassName = strtolower($tokens[$classPtr]['content']);
---
>         $classUsed      = $phpcsFile->findNext([ T_STRING, T_DOC_COMMENT_STRING ], ($classPtr + 1));
>         $className      = $tokens[$classPtr]['content'];
>         $lowerClassName = strtolower($className);
66a68
>                  T_DOC_COMMENT_STRING,
88c90,91
<                 $classUsed = false;
---
>                 // This breaks things - I don't know why it exists in the Drupal version
>                 // $classUsed = false;
92,93c95,101
<         while ($classUsed !== false) {
<             if (strtolower($tokens[$classUsed]['content']) === $lowerClassName) {
---
>         $emptyTokens = PHP_CodeSniffer_Tokens::$emptyTokens;
>         unset($emptyTokens[T_DOC_COMMENT_TAG]);
> 
>         while($classUsed !== false) {
>             if (($tokens[$classUsed]['code'] == T_STRING && strtolower($tokens[$classUsed]['content']) === $lowerClassName)
>                 || ($tokens[$classUsed]['code'] == T_DOC_COMMENT_STRING && preg_match('/(\s|\||^)'.preg_quote($lowerClassName).'(\s|\||$|\[\])/i', $tokens[$classUsed]['content']))
>             ) {
95c103
<                     PHP_CodeSniffer_Tokens::$emptyTokens,
---
>                     $emptyTokens,
100,104d107
<                 // If a backslash is used before the class name then this is some other
<                 // use statement.
<                 if ($tokens[$beforeUsage]['code'] !== T_USE && $tokens[$beforeUsage]['code'] !== T_NS_SEPARATOR) {
<                     return;
<                 }
106,108c109,126
<                 // Trait use statement within a class.
<                 if ($tokens[$beforeUsage]['code'] === T_USE && empty($tokens[$beforeUsage]['conditions']) === false) {
<                     return;
---
>                 if ($tokens[$classUsed]['code'] == T_STRING) {
>                     // If a backslash is used before the class name then this is some other
>                     // use statement.
>                     if ($tokens[$beforeUsage]['code'] !== T_USE && $tokens[$beforeUsage]['code'] !== T_NS_SEPARATOR) {
>                         return;
>                     }
> 
>                     // Trait use statement within a class.
>                     if ($tokens[$beforeUsage]['code'] === T_USE && empty($tokens[$beforeUsage]['conditions']) === false) {
>                         return;
>                     }
>                 } else {
>                     if ($tokens[$beforeUsage]['code'] === T_DOC_COMMENT_TAG && (                        $tokens[$beforeUsage]['content'] == '@var'
>                         || $tokens[$beforeUsage]['content'] == '@param'
>                         || $tokens[$beforeUsage]['content'] == '@return')
>                     ) {
>                         return;
>                     }
110c128
<             }
---
>             }//end if
112c130
<             $classUsed = $phpcsFile->findNext(T_STRING, ($classUsed + 1));
---
>             $classUsed = $phpcsFile->findNext([T_STRING, T_DOC_COMMENT_STRING], ($classUsed + 1));
115c133
<         $warning = 'Unused use statement';
---
>         $warning = 'Unused use statement: '.$className;

[prevostc]

+1 I really need this one and I'll be happy to fix this branch if it's been abandoned

[ExplodingCabbage]

Hi @prevostc. Sorry for 'abandoning' this - I keep intending to come back to it, and never quite doing so.

All that remains to be done here is understanding @donatj's changes and implementing them. That basically comes down to two things:

• [ ] deciding whether to remove or keep the line

  $classUsed = false;
  

  which @donatj comment out, alleging that it 'breaks things'. I forget whether I looked into this enough to understand if he was right, or if the line had a purpose he was missing (or both).

• [ ] adding support for recognising class names used as types in PHPDoc comments. @donatj does this using a regex; I'm not familiar enough with PHPDoc syntax to know whether this is reasonable or whether it's a crazy misguided idea in the same genre as using regexes to validate email addresses and we ought to be using a PHPDoc parser library instead.

If you'd like to look into either of these questions, the help would be welcome!

It would also be good to hear from @gsherwood whether he'll even accept this PR when it's done. Honestly, not having had him opine on this at all saps my motivation to spend time putting the final polish on it - I'm not currently sure whether I'd be wasting my time by doing more work on this.

[donatj]

@ExplodingCabbage

which @donatj comment out, alleging that it 'breaks things'.

I wish I could tell you, it's been a very long time since I wrote that. I can tell you we've been using this unchanged from what I posted on a very large production code base for a few years without issue though.

@donatj does this using a regex; I'm not familiar enough with PHPDoc syntax to know whether this is reasonable or whether it's a crazy misguided idea

That code exists to parse multiple pipe | seperated types in phpdocs.

I don't know if there's a better way built into PHPCS. I certainly couldn't find one it at the time. If the phpcs gods know better I'd be very happy to have that replaced.

[gmponos]

I tried to see if there was any related comment but I could not find anything. Forgive me if I am mentioning something that it is already said.

I copy pasted the code of this sniff and used it into a project of mine.

I had a use statement because of some comments. This sniff still detects them as unused. For instance:

<?php

use MyNamspace\MyException; //Detects this as unused.

class MyClass
{
    /**
     * @throws MyException
     */
     public function myFunction(){}
}

It's not easy to go through all the comments and use the full path of the namespace and it's not practical. It increases maintenance. Can this be fixed?

[ExplodingCabbage]

Yep @gmponos handling your use case is one of the things that @donatj's changes will handle - it's what I was talking about above when I said that

adding support for recognising class names used as types in PHPDoc comments

was still to be done.

[gsherwood]

I don't personally think that adding a use statement just to shorten PHP doc comments is something that should be done, but I think it can be optionally supported by doing this:

Create two error codes for this sniff - one as an error and one as a warning. If the used class is never used anywhere in the code, generate a NotUsed error. If it is only used in doc comments, generate a UsedInDoc warning. If people don't want the warning because they like using the alias feature, they can exclude that code in their ruleset.

For fixing, just remove the use statement as is done now. If the warning is excluded, the addFixableWarning() method will return false and the fix will not be applied. So excluding that warning message will not generate any errors, and will leave the use statement in place.

This sniff will need extensive unit tests to prove that it is detecting and fixing things correctly because removing use statements is a great way to stop code from working.

[levacic]

@ExplodingCabbage @donatj @gmponos @gsherwood Just to mention that it's not just doc comments as in "method documentation" - in docblocks, you can also use imported annotations, e.g. check this out: http://docs.doctrine-project.org/projects/doctrine-common/en/latest/reference/annotations.html#usage

Excerpt:

<?php
namespace MyCompany\Entity;

use MyCompany\Annotations\Foo;
use MyCompany\Annotations\Bar;

/**
 * @Foo(bar="foo")
 * @Bar(foo="bar")
 */
class User
{
}

So yeah, this might be an additional complication, as the code needs to check both for stuff like @param Foo and @Foo, as these both represent valid usage of an imported class.

Perhaps something like https://github.com/phpDocumentor/ReflectionDocBlock could be used to help with parsing docblocks without having to reinvent the wheel? However I do see that this project has no dependencies on other projects, so I get if you want it to remain like that as well.

@gsherwood Our company does import all classes, even those used only in docblocks, and we would really benefit from the sniff to automate these checks. The why is simple - consistency. Otherwise, we would have to use FQCNs in docblocks if a class is only used there, but short names otherwise. Well, obviously we could just use FQCNs in all docblocks, but that's painful to read and we definitely don't want to do that.

[ExplodingCabbage]

I've come back to this in the last few days and made a bunch of progress, mostly in the form of adding failing tests that reveal existing bugs or omissions. (@gsherwood was right to insist on heavy test coverage.) Things that need(ed) doing to get the tests to pass:

• [x] Detect use statements in braced namespaces
• [x] Handle class references in PHPDoc with home-rolled regexes based on @donatj's. (None of the PHPDoc-parsing libraries are usable here because they all have much higher minimum PHP versions than PHPCS does, unfortunately.)
• [ ] Fix the fixer's handling of multiple use statements on one line. (If a line contains one good use statement and one bad one, the fixer shouldn't remove the trailing newline when removing the bad use statement. More generally, the rule should probably be that the trailing newline should only be removed along with a use statement if it was the only non-whitespace on the line.)
• [x] Handle PHP 7 Group Use Declarations.
• [x] Handle files containing multiple namespaces correctly. (Currently an unused use is missed if the name being used is referenced in a different namespace later in the file; this shouldn't be the case, since the later namespace can't see the names that were used earlier.)

I intend to plough through this over the coming few days until it's done.

[ExplodingCabbage]

Made some progress today. Still TODO:

• [ ] Handle const and function use statements, including inside group use declarations
  • [ ] I think this will require fixing an existing bug in the tokenizer; the const in use const BlaBlaNamespace\SOME_UNUSED_CONSTANT; in UnusedUseStatementTest.1.inc gets parsed as a T_STRING instead of a T_CONST, which I think is an existing bug.
• [ ] Fix fixer's handling of multiple statements on one line, as per previous post
• Make handling of PHPDoc references better; currently, I have a super-liberal implementation that just looks for the name appearing as a full word anywhere. Instead, I should explicitly detect use in:
  • [ ] An annotation
  • [ ] https://www.phpdoc.org/docs/latest/references/phpdoc/tags/global.html
  • [ ] https://www.phpdoc.org/docs/latest/references/phpdoc/tags/throws.html
  • [ ] https://www.phpdoc.org/docs/latest/references/phpdoc/tags/var.html
  • [ ] https://www.phpdoc.org/docs/latest/references/phpdoc/tags/method.html
  • [ ] https://www.phpdoc.org/docs/latest/references/phpdoc/tags/param.html
  • [ ] https://www.phpdoc.org/docs/latest/references/phpdoc/tags/property.html
  • [ ] https://www.phpdoc.org/docs/latest/references/phpdoc/tags/property-read.html
  • [ ] https://www.phpdoc.org/docs/latest/references/phpdoc/tags/property-write.html
  • [ ] https://www.phpdoc.org/docs/latest/references/phpdoc/tags/return.html
  • [ ] ... including as part of the type-expression, array-of-type-expression or array-of-type type syntaxes documented at https://www.phpdoc.org/docs/latest/references/phpdoc/types.html
• [ ] Conform to PHPCS's own coding standard, which my current implementation almost certainly doesn't do.

Once all those things are fixed, this will truly be done.

[TomasVotruba]

Any updates on this? I'm afraid that scope is growing a lot, but no working sniff is ready. Maybe splitting responsibility to class, constant, function etc. would make this happen. Imo

[maxgorovenko]

This sniff doesn't work if there is no namespace at all. The problem with $endOfNamespacePtr. Method getContainingNamespace() makes it as false, but $file->find() method accepts end value only as int or null.

And the other thing: sniff missing method return types, which are new to php 7.0 "Return type declarations".

[donatj]

I don't know if you guys are still working on this, but we finally got php7 at our shop and I had to update my version to note T_RETURN_TYPE

The most up to date version of my sniff being here: http://jdon.at/iJbEi1

[mathroc]

I wanted to try this and got started but it does not seems to be up to date for the latest phpcs 3. I’ve tried to updated but after updating the class names I got stuck (I’ll add a comment in the code to explain where)

@donatj your version is working well, it just does not take into account annotations in docblocks. I’ll see if I have time to add that

[mathroc]

and here it is with annotation support (eg: @Security(…) in symfony, etc.)

https://pastebin.com/7vNpufey

the changes were minor

[gmponos]

Hello @mathroc , unfortunately it does not work good with doctrine.

use Doctrine\ORM\Mapping\UniqueConstraint;

/**
 * @ORM\Entity(repositoryClass="\MyEntityFQN")
 * @ORM\Table(name="mytable", uniqueConstraints={@UniqueConstraint(...), @UniqueConstraint(...)}))
 */

It finds the UniqueConstraint as unused :(

[WishCow]

Not sure if it has been mentioned, but php-cs-fixer already includes logic for finding and removing unused use statements, has anyone looked at that?

https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.8/src/Fixer/Import/NoUnusedImportsFixer.php

https://github.com/FriendsOfPHP/PHP-CS-Fixer

[mathroc]

@gmponos ah yes, I didn’t think of that case, can you try to replace this line:

($tokens[$classUsed]['code'] == T_DOC_COMMENT_TAG && preg_match('/(\s|^)@' . preg_quote($lowerClassName) . '\b/i', $tokens[$classUsed]['content']))

with

($tokens[$classUsed]['code'] == T_DOC_COMMENT_TAG && preg_match('@' . preg_quote($lowerClassName) . '\b/i', $tokens[$classUsed]['content']))

[gmponos]

I don't think this is the issue.

https://regex101.com/r/2IRwNW/1

Anyway. I would not suggest doing this back and forth test thing. (giving me changes to test). It's not very efficient. 😄