Mixing concatenating assignment operator and assigment operation results in false positive.

[alexpott]

Given the following code

  $suggestion = 'block';
  while ($part = array_shift($parts)) {
    $suggestions[] = $suggestion .= '__' . strtr($part, '-', '_');
  }

$suggestion will be marked as unused.

[alexpott]

Similarly $entry is reported as unused.

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

    return $this->derivatives;

[jrfnl]

@alexpott Re the second example: IMO this is correct.
You are only assigning something to $entry, but never using the resulting value.

[avpaderno]

That type of code is enough common, with latest PHP versions. It would be nice if the code would not be reported.

[alexpott]

@jrfnl thanks for looking at this issue.

Re the second example - it's a tricky one because I think $entry is used. It's a reference to an element in $this->derivatives - so $this->derivatives is being changed by the foreach loop and then $this->derivatives is being returned.

[sirbrillig]

Thanks for the report! This might be worth reporting as two separate issues unless you see a connection that I'm missing (which is very possible).

I haven't dug into it yet but I'm guessing that in the first case the array assignment (push) is masking the assignment to the single scalar.

In the second case, there's probably a bug with how assignment by reference is being detected? Honestly it's hard for me to grok what's happening there. That code loops through an array of strings and appends another string to each one, modifying the original array items? EDIT: ah, yes, I just read your last comment.

[sirbrillig]

Another interesting bug I found when researching this:

function concatAndAssignWithUndefinedShift() {
  $suggestions = [];
  while ($part = array_shift($parts)) { // undefined variable parts
    $suggestions[] = $part;
  }
	return $suggestions;
}

$parts is not marked as undefined. I created a new issue for this as it's rather thorny. #184

[sirbrillig]

Ok, I know why this is happening. In this example, $suggestion is marked as unused.

function assignFunction($parts) {
  $suggestion = 'block';
  $suggestion .= '__';
  return $parts;
}

The reason is that even though $suggestion is modified, it is never read. Changing the example to return the variable removes the warning.

function assignFunction() {
  $suggestion = 'block';
  $suggestion .= '__';
  return $suggestion;
}

In the case of your first example, the expression $suggestions[] = $suggestion .= '__' is not being counted as a read of $suggestion. This is because the right-hand-side expression taken by itself ($suggestion .= '__') is only a write but when combined with the left-hand-side expression, it is both a write and a read. The sniff will need to be modified to detect such situations.

function causesBug() {
  $suggestions = [];
  $suggestion = 'block'; // marked incorrectly as unused
  $suggestions[] = $suggestion .= '__'; // suggestion is marked as a write
  return $suggestions;
}

function noBug() {
  $suggestions = [];
  $suggestion = 'block';
  $suggestions[] = $suggestion; // suggestion is marked as a read
  return $suggestions;
}

[sirbrillig]

I think a way to resolve this will be: when marking a variable as a write, also look to see if it's on the right-hand-side of an assignment or within a function call (essentially, if we're part of another expression). If so, also mark that variable as a read.

The difficult thing will be how to determine if we're part of another expression. For the above example, it is immediately preceded by an equals sign, but it could also be $suggestions[] = ( $suggestion .= '__' ); or $suggestions[] = getPrefix() . $suggestion .= '__'; or doSomething($suggestion .= '__'); or probably some more complex expressions.