Support marking some function arguments as unused

[dotboris]

Take the following code as an example:

<?php
namespace Whatever;

use Symfony\Component\HttpKernel\Exception\HttpException;
use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
use Laravel\Lumen\Exceptions\Handler as ExceptionHandler;

final class LumenExceptionHandler extends ExceptionHandler {
    public function render($request, \Exception $e) {
        $status = 500;
        if ($e instanceof HttpExceptionInterface) {
            $status = $e->getStatusCode();
        }

        return response()->json(['message' => $e->getMessage()], $status);
    }
}

The render function takes 2 arguments. A request and an exception. We have to accept these two arguments because we're overwriting a method and we want to match it's signature.

In our implementation, we don't actually care about the $request param. PHPCS gives me a warning because $request is unused.

I want to be able to tell PHPCS that this argument is unused and that this is OK.

With Rubocop (a ruby linter), I can prefix an argument with _ to mark it as unused and to let the linter know that this is OK. Here's the relevant doc: https://rubocop.readthedocs.io/en/latest/cops_lint/#lintunusedmethodargument

We could have something like this:

public function render($_request, \Exception $e) {
  // ...
}

This would not generate any warnings since we've explicitly said that the argument should be unused.

[dotboris]

Obviously, using an argument that's been marked as unused, should raise a warning.

[sirbrillig]

🤔 this is a really interesting idea. I'm going to look around for prior art in other linters (thanks for the rubocop link) to see what patterns exist.

In the mean time, you could just disable the rule for that line (although I realize that also disables it for the other arguments):

public function render($request, \Exception $e) { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable
  // ...
}

[sirbrillig]

This appears to be handled in eslint by an option on the rule.

Alternatively, there's an option which makes sure at least the last param is used.

after-used - only the last argument must be used. This allows you, for instance, to have two named parameters to a function and as long as you use the second argument, ESLint will not warn you about the first. This is the default setting.

I like both these options better than prefixing the variable with a static string, since that requires altering the code for the sake of the linter, rather than the other way around.

[sirbrillig]

Hey! It turns out this option already exists! I'm going to improve the documentation for it and add tests in #22, but you should be able to do something like the following in your config file (the example below will ignore variables named $ignored):

 <rule ref="VariableAnalysis.CodeAnalysis.VariableAnalysis">
  <properties>
   <!--
      Include the following property if you want to have a whitelist of variable names
      that are commonly used for placeholder "junk" values that ignored and that you
      don't want to provoke an unused variable warning for.
      Format is "variableName" without a leading $ with whitespace delimiting names.
   -->
   <property name="validUnusedVariableNames" value="ignored"/>
  </properties>
 </rule>

[dotboris]

Will this work with multiple ignored arguments?

What about the following case?

function foobar($ignored, $ignored, $usedArg) {

}

[dotboris]

When I do this I get a fatal because I'm redefining an argument.

PHP Fatal error:  Redefinition of parameter $ignored in /tmp/test.php on line 3

In my code I have a bunch of cases, where I'm implementing some contract and that I don't care about 2 or 3 arguments. Using the validUnusedVariableNames seems to be awkward in those cases? I don't want to maintain a list of ignored variables.

I guess that I could call the arguments ignored1, ignored2 and so on, but I'd have to keep adding them to option as I start hitting cases where I don't care about 3, 4, 5 arguments and so on.

[sirbrillig]

That's a good point. As you said you could do something like

   <property name="validUnusedVariableNames" value="ignoredFirst ignoredSecond ignoredThird"/>

and then do

function foobar($ignoredFirst, $ignoredSecond, $ignoredThird, $usedArg);

But that's pretty cumbersome. It'd be nicer if the parameter was a regexp which would match parts of variable names. I considered changing validUnusedVariableNames to be a regexp instead (it'd be pretty easy) but that would be a breaking change. But I certainly could add a new parameter for a regexp, like validUnusedVariablePattern.

Then you could do this:

   <property name="validUnusedVariablePatern" value="ignored"/>

function foobar($ignoredFirst, $ignoredSecond, $ignoredThird, $usedArg);

Or, if you prefer the underscores (note the caret which means "start of string"):

   <property name="validUnusedVariablePatern" value="^_"/>

function foobar($_something, $_another, $_yetAnother, $usedArg);

[dotboris]

@sirbrillig I think that having a validUnusedVariablePattern would be the best solution.

Related question: Does the linter complain if you use a variable that's configured as unused? (through validUnusedVariableNames or validUnusedVariablePattern)

[loilo]

For the record: Having this option would also be great for other uses cases where variables need to be defined but not necessarily used:

Exceptions:

try {
  // Some code that only throws a single type of exception
} catch (Exception $_e) {
  // I know what's happening, I really don't need to use $_e here
}

foreach loops:

foreach ($list as $key => $_value) {
  // Use $key, ignore $_value
}

[sirbrillig]

@loilo thanks for the reminder about this. #22 should take care of this.