Added magic variables to suppress unused local variables #7

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
9 participants

dominis commented Jan 4, 2012

We come up against problems like this in our codebase:

<?php
try {
    ...
} catch( Exception $e ) {
    return false;
}

This throws an unused local variable warning. Unfortunately we're unable to suppress this, so I came up with the magic variables detection in the PHP_PMD_Rule_AbstractLocalVariable class. This works same as the superglobals, and now I defined the $_suppress variable to ignore the problem described above.

<?php
try {
    ...
} catch( Exception $_suppress ) {
    return false;
}

nud commented Oct 2, 2012

I would use $_ instead of $_suppress, as

  • it conveys no undesired meaning ($_suppress implies suppression of what?)
  • it is consistent with what some other languages do (e.g. python's common practice is to use _ when doing list expansion)

I found this bug when trying to find a way to make PHPMD ignore unused variables in list() expansions.

shaddyz commented Apr 5, 2013

I don't like the idea of a magic rule based on how the variable is named as a solution to this problem. Then using PHPMD begins to dictate your coding standard. I would prefer a simple code comment.

 try {
     ...
 } catch(Exception $exception) {
     // not used: $exception
     return false;
 }

Something akin to the PSR-2 rule for omitting breaks:

The case statement MUST be indented once from switch, and the break keyword (or other terminating keyword) MUST be indented at the same level as the case body. There MUST be a comment such as // no break when fall-through is intentional in a non-empty case body.

Dynom commented Apr 23, 2013

I think PR #66 is a better solution for this problem, since it's probably an application wide setting. Also it's pretty common to name an exception object $e, which is also something PR #66 solves.

fazy commented Jul 18, 2013

PR #66 is good but there's another possible use case: take a method like this one:

public function buildForm(FormBuilderInterface $builder, array $options) { // blah // }

PHPMD is reporting that $options is unused; but that's because we are overriding a method in an abstract class, and simply don't need the second parameter. In an ideal world this situation shouldn't arise, but when using third party code it can happen.

So should we use a @SuppressWarnings annotation? I prefer not to litter my code with these (although I used @SuppressWarnings(PHPMD.ExcessiveMethodLength) in a long unit test method).

Or does the UnusedLocalVariable rule need to be able to ignore variables that are passed in as parameters when extending classes/implementing interfaces?

Hast commented Aug 24, 2013

Any news here? I have a pretty same problem. In my case I need to get all keys of some array through a foreach, like this:

    foreach ($container->findTaggedServiceIds('app.permission_resolver') as $id => $attributes)
    {
        // Inside foreach I'm using only the $id variable.
    }

And PHPMD is reporting that $attributes is unused local variable.
Is there any chance to ignore unused array elements values inside foreach loops?

Since the purpose of the library is detecting mess, I would pretty much vote against this PR for the following reasons:

  • @nud @shaddyz Try/catch blocks should not be used to control application flow, which is basically what the return false does. Alarming those is correct but having its own rule would be probably better.
  • @fazy If there is a parameter declared that is not used, it is a problem. Avoiding this is easy on your side, by simply throwing and Exception o the usage of that parameter; which really conveys the case.
  • @Hast You can also solve your problem by using array_keys() function.

As for all cases, there are really some problems on the application side, I see no point on keeping this Pull Request open. And I don't really see any point on suppressing a "unused variable" alert, since it is a pretty good indicator of mess in code.

nud commented Sep 13, 2013

@augustohp list($foo, $bar, $baz) = $somearray; do not use $bar. Is it a bad practice as well?

jaysh commented Jan 31, 2014

@augustohp I disagree that littering our code with:

if (!empty($options)) {
    throw new \Exception('...');
}

is the correct way to go. Can you confirm that's what you are saying, or did I misunderstand? Just to be clear here: the issue with the given example is within a FormType, which there may be dozens of in a Symfony codebase (to ensure forms are re-usable). Therefore, copy/pasting this into each and every one of them would be wrong. Additionally, as you have pointed out, the library is designed to detect and fix mess: any one of throwing an exception, artificially suppressing the type of warnings on a per method basis (and littering those around), or using inheritance to solve the problem would not make the code neater.

As previously mentioned, the Python convention is to use _ as a variable that is unused, and Python is well-regarded for being well thought-out and very readable.

Therefore, I would vote in favour of using $_ to fulfil this purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment