Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only allow unused values in associative foreach loops with option #167

Merged
merged 6 commits into from
Apr 19, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ The available options are as follows:
- `validUnusedVariableNames` (string, default `null`): a space-separated list of names of placeholder variables that you want to ignore from unused variable warnings. For example, to ignore the variables `$junk` and `$unused`, this could be set to `'junk unused'`.
- `ignoreUnusedRegexp` (string, default `null`): a PHP regexp string (note that this requires explicit delimiters) for variables that you want to ignore from unused variable warnings. For example, to ignore the variables `$_junk` and `$_unused`, this could be set to `'/^_/'`.
- `validUndefinedVariableNames` (string, default `null`): a space-separated list of names of placeholder variables that you want to ignore from undefined variable warnings. For example, to ignore the variables `$post` and `$undefined`, this could be set to `'post undefined'`.
- `allowUnusedForeachVariables` (bool, default `true`): if set to true, unused keys or values created by the `as` statement in a `foreach` loop will never be marked as unused.
- `allowUnusedForeachVariables` (bool, default `true`): if set to true, unused values from the `key => value` syntax in a `foreach` loop will never be marked as unused.
- `sitePassByRefFunctions` (string, default `null`): a list of custom functions which pass in variables to be initialized by reference (eg `preg_match()`) and therefore should not require those variables to be defined ahead of time. The list is space separated and each entry is of the form `functionName:1,2`. The function name comes first followed by a colon and a comma-separated list of argument numbers (starting from 1) which should be considered variable definitions. The special value `...` in the arguments list will cause all arguments after the last number to be considered variable definitions.
- `allowWordPressPassByRefFunctions` (bool, default `false`): if set to true, a list of common WordPress pass-by-reference functions will be added to the list of PHP ones so that passing undefined variables to these functions (to be initialized by reference) will be allowed.

Expand Down
4 changes: 2 additions & 2 deletions VariableAnalysis/Lib/Helpers.php
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public static function findParenthesisOwner(File $phpcsFile, $stackPtr) {
* @return bool
*/
public static function areAnyConditionsAClass(array $conditions) {
foreach (array_reverse($conditions, true) as $scopePtr => $scopeCode) {
foreach (array_reverse($conditions, true) as $scopeCode) {
if ($scopeCode === T_CLASS || $scopeCode === T_ANON_CLASS || $scopeCode === T_TRAIT) {
return true;
}
Expand All @@ -99,7 +99,7 @@ public static function areConditionsWithinFunctionBeforeClass(array $conditions)
// Return true if the token conditions are within a function before
// they are within a class.
$classTypes = [T_CLASS, T_ANON_CLASS, T_TRAIT];
foreach (array_reverse($conditions, true) as $scopePtr => $scopeCode) {
foreach (array_reverse($conditions, true) as $scopeCode) {
if (in_array($scopeCode, $classTypes)) {
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion VariableAnalysis/Lib/VariableInfo.php
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ class VariableInfo {
/**
* @var bool
*/
public $isForeachLoopVar = false;
public $isForeachLoopAssociativeValue = false;

/**
* @var string[]
Expand Down
12 changes: 8 additions & 4 deletions VariableAnalysis/Sniffs/CodeAnalysis/VariableAnalysisSniff.php
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ class VariableAnalysisSniff implements Sniff {
public $allowUnusedParametersBeforeUsed = true;

/**
* If set to true, unused keys or values created by the `as` statement
* If set to true, unused values from the `key => value` syntax
* in a `foreach` loop will never be marked as unused.
*
* @var bool
Expand Down Expand Up @@ -608,7 +608,7 @@ protected function checkForThisWithinClass(File $phpcsFile, $stackPtr, $varName)
}

$inFunction = false;
foreach (array_reverse($token['conditions'], true) as $scopePtr => $scopeCode) {
foreach (array_reverse($token['conditions'], true) as $scopeCode) {
// $this within a closure is valid
if ($scopeCode === T_CLOSURE && $inFunction === false) {
return true;
Expand Down Expand Up @@ -1029,7 +1029,11 @@ protected function checkForForeachLoopVar(File $phpcsFile, $stackPtr, $varName,
}
$this->markVariableAssignment($varName, $stackPtr, $currScope);
$varInfo = $this->getOrCreateVariableInfo($varName, $currScope);
$varInfo->isForeachLoopVar = true;

// Is this the value of a key => value foreach?
if ($phpcsFile->findPrevious(T_DOUBLE_ARROW, $stackPtr - 1, $openParenPtr) !== false) {
$varInfo->isForeachLoopAssociativeValue = true;
}

return true;
}
Expand Down Expand Up @@ -1415,7 +1419,7 @@ protected function processScopeCloseForVariable(File $phpcsFile, VariableInfo $v
if ($this->allowUnusedParametersBeforeUsed && $varInfo->scopeType === 'param' && $this->areFollowingArgumentsUsed($varInfo, $scopeInfo)) {
return;
}
if ($this->allowUnusedForeachVariables && $varInfo->isForeachLoopVar) {
if ($this->allowUnusedForeachVariables && $varInfo->isForeachLoopAssociativeValue) {
return;
}
if ($varInfo->passByReference && isset($varInfo->firstInitialized)) {
Expand Down
10 changes: 10 additions & 0 deletions VariableAnalysis/Tests/CodeAnalysis/VariableAnalysisTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,10 @@ public function testFunctionWithForeachWarnings() {
22,
24,
26,
48,
50,
52,
54,
// FIXME: this is an unused variable that needs to be fixed but for now
// we will ignore it. See
// https://github.com/sirbrillig/phpcs-variable-analysis/pull/36
Expand Down Expand Up @@ -836,12 +840,15 @@ public function testUnusedForeachVariablesAreIgnoredByDefault() {
$phpcsFile->process();
$lines = $this->getWarningLineNumbersFromFile($phpcsFile);
$expectedWarnings = [
5,
7,
8,
16,
17,
23,
25,
26,
32,
33,
34,
];
Expand Down Expand Up @@ -886,12 +893,15 @@ public function testUnusedForeachVariablesAreIgnoredIfSet() {
$phpcsFile->process();
$lines = $this->getWarningLineNumbersFromFile($phpcsFile);
$expectedWarnings = [
5,
7,
8,
16,
17,
23,
25,
26,
32,
33,
34,
];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ function nestedFunctionDeclaration() {
}
}
}
}
};

$anonClass = class() {
$anonClass = new class() {
public function bar() {
// Using $this here is fine.
if ($this->something) {
Expand All @@ -50,7 +50,7 @@ function nestedFunctionDeclaration() {
}
}
}
}
};

trait FooTrait {
public function bar() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

function loopWithUnusedKey() {
$array = [];
foreach ( $array as $key => $value ) { // maybe marked as unused
foreach ( $array as $key => $value ) { // key always marked as unused
echo $value;
$unused = 'foobar'; // should always be marked as unused
echo $undefined; // should always be marked as undefined
Expand All @@ -11,7 +11,7 @@ function loopWithUnusedKey() {

function loopWithUnusedValue() {
$array = [];
foreach ( $array as $key => $value ) { // maybe marked as unused
foreach ( $array as $key => $value ) { // key maybe marked as unused
echo $key;
$unused = 'foobar'; // should always be marked as unused
echo $undefined; // should always be marked as undefined
Expand All @@ -20,7 +20,7 @@ function loopWithUnusedValue() {

function loopWithUnusedKeyAndValue() {
$array = [];
foreach ( $array as $key => $value ) { // maybe marked as unused
foreach ( $array as $key => $value ) { // key always marked as unused
echo 'hello';
$unused = 'foobar'; // should always be marked as unused
echo $undefined; // should always be marked as undefined
Expand All @@ -29,7 +29,7 @@ function loopWithUnusedKeyAndValue() {

function loopWithUnusedValueOnly() {
$array = [];
foreach ( $array as $value ) { // maybe marked as unused
foreach ( $array as $value ) { // value always marked as unused
$unused = 'foobar'; // should always be marked as unused
echo $undefined; // should always be marked as undefined
}
Expand Down