False positive for unused variable when variable is passed by reference in closure

[MPParsley]

DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable gives a false positive for the $uri variable in the following example:

\Drupal::httpClient()->get($input, [
  'on_stats' => function (TransferStats $stats) use (&$uri) {
    $uri = $stats->getEffectiveUri();
  },
]);

Another false positive occurs for the following code snippet which modifies an array by reference:

 80 | WARNING | Unused variable $entry.
    |         | (DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable)

foreach ($derivatives as &$entry) {
  $entry += $base_plugin_definition;
}

The sniff doesn't detect that the $uri variable is actually in use since it is passed by reference to the anonymous function.

Originally posted in https://www.drupal.org/project/coder/issues/3065679.

[sirbrillig]

Thanks! Good find. These sound like slightly different issues despite their similarities (the first has to do with an anonymous function but the second has to do with a foreach loop and they have different scoping rules), so I'll take them on one at a time.

The foreach bug should be fixed by #221 although I think your example should proably read as follows if $entry and $base_plugin_definition are strings:

foreach ($derivatives as &$entry) {
  $entry .= $base_plugin_definition;
}

[sirbrillig]

I wasn't able to reproduce the issue you described with this example:

\Drupal::httpClient()->get($input, [
  'on_stats' => function (TransferStats $stats) use (&$uri) {
    $uri = $stats->getEffectiveUri();
  },
]);

It made me wonder if you had an outdated version of phpcs-variable-analysis installed... It looks like og/composer is using drupal/coder, which is configured to use phpcs-variable-analysis ^2.10, but even if I run the above code through 2.10.0, I don't get any warnings. Can you help me create a test case that fails with 2.10?

[jonathan1055]

Just responding to your first comment, the original example with $entry += $base_plugin_definition is correct because $entry is an array not a string. I've not checked if #221 does fix it, but I expect it will, given that it tackles the by-reference read problem.

[sirbrillig]

$entry += $base_plugin_definition is correct because $entry is an array not a string

Oh, right! Thank you. I shouldn't have assumed.

[MPParsley]

Here's a complete example:

  public function getDerivativeDefinitions($base_plugin_definition) {
    $derivatives = [];

    foreach (array_keys($this->groupTypeManager->getGroupMap()) as $entity_type_id) {
      $route_name = "entity.$entity_type_id.og_admin_routes";

      if (!$this->routProvider->getRoutesByNames([$route_name])) {
        // Route not found.
        continue;
      }

      $derivatives[$entity_type_id . '.og_admin_routes'] = [
        'route_name' => $route_name,
        'title' => $this->t('Group'),
        'base_route' => 'entity.' . $entity_type_id . '.canonical',
        'weight' => 50,
      ];
    }

    // @todo Coder throws a false positive for this line. Remove this once the
    //   issue is fixed in the Coder project.
    // @see https://www.drupal.org/project/coder/issues/3065679
    // @codingStandardsIgnoreLine
    foreach ($derivatives as &$entry) {
      $entry += $base_plugin_definition;
    }

    return $derivatives;
  }

Currently running

            "name": "sirbrillig/phpcs-variable-analysis",
            "version": "v2.10.1",

[sirbrillig]

@MPParsley thank you! That should be fixed now with v2.10.2. Can you help me to reproduce the other issue you reported? This one:

\Drupal::httpClient()->get($input, [
  'on_stats' => function (TransferStats $stats) use (&$uri) {
    $uri = $stats->getEffectiveUri();
  },
]);

[MPParsley]

The original bug in https://www.drupal.org/project/coder/issues/3065679 was posted by @pfrenssen but here's an attempt to reproduce it (requires https://github.com/guzzle/guzzle):

use GuzzleHttp\TransferStats;

$client = new GuzzleHttp\Client();

\Drupal::httpClient()->get($input, [
  'on_stats' => function (TransferStats $stats) use (&$uri) {
    $uri = $stats->getEffectiveUri();
  },
]);

[sirbrillig]

Thanks! I think that the issue in the above code is that $uri is not defined anywhere before it's used by the use() expression. Try this:

 use GuzzleHttp\TransferStats;

$client = new GuzzleHttp\Client();
$uri = 'hello';

\Drupal::httpClient()->get($input, [
  'on_stats' => function (TransferStats $stats) use (&$uri) {
    $uri = $stats->getEffectiveUri();
  },
]);

[MPParsley]

Hi @sirbrillig, as mentioned the code doesn't come from me but I assume $uri was used somewhere, most likely before the snippet as you mentioned:

use GuzzleHttp\TransferStats;

$client = new GuzzleHttp\Client();
$uri = 'somuri';

\Drupal::httpClient()->get($input, [
  'on_stats' => function (TransferStats $stats) use (&$uri) {
    $uri = $stats->getEffectiveUri();
  },
]);

[sirbrillig]

👍 I tried the following:

file-from-pr.php:

<?php
use GuzzleHttp\TransferStats;

$uri = 'somuri';
$input = '';

\Drupal::httpClient()->get($input, [
  'on_stats' => function (TransferStats $stats) use (&$uri) {
    $uri = $stats->getEffectiveUri();
  },
]);

Then I ran this (using v2.10.2):

phpcs -s --standard=VariableAnalysis ./file-from-pr.php

I get no warnings on that file. I know this isn't your code (maybe @pfrenssen will be able to jump in to offer assistance 🙇‍♂️ ), but I can't find any further issues here. If you can think of something that will generate a false positive, or if I'm doing something wrong with the code above, please let me know! In the meantime, I'm going to close this issue. I'm happy to continue the discussion, though, and re-open it if necessary.

[pfrenssen]

Thanks! This last code snippet is from an issue that I reported back in 2019, it's probably been fixed a long time ago.