UnusedVariableLinter semi-incorrectly detects some loop variables as unused

[ryangreenberg]

Given the following code, UnusedVariableLinter flags the foreach loop variable $apple as unused and suggests an autofix to $_apple. This then causes a typechecker error because $apple['number'] is using $apple for assignment.

<?hh // strict

<<__EntryPoint>>
function used_via_subscript_example(): void {
	$fruit = vec[dict['varietal' => 'Macintosh'], dict['varietal' => 'Winesap']];

	foreach ($fruit as $idx => $apple) {
		$apple['number'] = $idx;
	}

	return;
}

The reason that $apple is considered "unused" in the loop body is because $apple['number'] = ... is classified as an assignment, not a use. This is less than ideal, though it does flag the problem that this code is probably not doing what is intended (since the mutation of $apple has no effect).

It's possible we should close this wontfix, but it surprised me when I was looking at autofixes for similar code in Slack's codebase.

[jjergus]

I think I'm missing something; in your example $apple is only mentioned once so renaming it to $_apple should be fine...?

Even if you add something like

$apple = dict[];
// ...
$apple['number'] = $idx;

that should still be fine because it will rename both occurences to $_apple, or won't it?

[ryangreenberg]

Past Ján and Ryan did not really help us out here. From #206:

Based on a discussion with @jjergus in #190, this PR eliminates autofixes for unused variables in most cases. Currently the UnusedVariableLinter suggests renaming unused var $a to $_a, mirroring the UnusedParameterLinter. This is rarely the correct fix. Here are some examples: ...

So we'll propose an autofix for the variable $apple in the foreach loop setup but not in the body because we only autofix the former. I think this starts to push more towards your earlier suggestion that we actually not suggest autofixes for anything.

Having recently applied this linter to one of Slack's large codebases I am now less convinced of the merits of even classifying vars in list(...) as unused; prefixing with $_ doesn't really add a lot of value, whereas in most other cases it does signal something of interesting.

[jjergus]

Oh of course, somehow I missed that $apple was used in the foreach loop, sorry.

I agree, we should err on the side of excluding any questionable warnings or autofixes, so that all lint rules remain high signal.