diff --git a/src/lint/linter/xhpast/ArcanistXHPASTLinter.php b/src/lint/linter/xhpast/ArcanistXHPASTLinter.php index 303c54d3..404e13e7 100644 --- a/src/lint/linter/xhpast/ArcanistXHPASTLinter.php +++ b/src/lint/linter/xhpast/ArcanistXHPASTLinter.php @@ -47,6 +47,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter { const LINT_TAUTOLOGICAL_EXPRESSION = 20; const LINT_PLUS_OPERATOR_ON_STRINGS = 21; const LINT_DUPLICATE_KEYS_IN_ARRAY = 22; + const LINT_REUSED_ITERATORS = 23; public function getLintNameMap() { @@ -73,6 +74,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter { self::LINT_TAUTOLOGICAL_EXPRESSION => 'Tautological Expression', self::LINT_PLUS_OPERATOR_ON_STRINGS => 'Not String Concatenation', self::LINT_DUPLICATE_KEYS_IN_ARRAY => 'Duplicate Keys in Array', + self::LINT_REUSED_ITERATORS => 'Reuse of Iterator Variable', ); } @@ -152,6 +154,7 @@ class ArcanistXHPASTLinter extends ArcanistLinter { $this->lintTautologicalExpressions($root); $this->lintPlusOperatorOnStrings($root); $this->lintDuplicateKeysInArray($root); + $this->lintReusedIterators($root); } private function lintTautologicalExpressions($root) { @@ -253,6 +256,84 @@ class ArcanistXHPASTLinter extends ArcanistLinter { } } + /** + * Find cases where loops get nested inside each other but use the same + * iterator variable. For example: + * + * COUNTEREXAMPLE + * foreach ($list as $thing) { + * foreach ($stuff as $thing) { // <-- Raises an error for reuse of $thing + * // ... + * } + * } + * + */ + private function lintReusedIterators($root) { + $used_vars = array(); + + $for_loops = $root->selectDescendantsOfType('n_FOR'); + foreach ($for_loops as $for_loop) { + $var_map = array(); + + // Find all the variables that are assigned to in the for() expression. + $for_expr = $for_loop->getChildOfType(0, 'n_FOR_EXPRESSION'); + $bin_exprs = $for_expr->selectDescendantsOfType('n_BINARY_EXPRESSION'); + foreach ($bin_exprs as $bin_expr) { + if ($bin_expr->getChildByIndex(1)->getConcreteString() == '=') { + $var_map[$bin_expr->getChildByIndex(0)->getConcreteString()] = true; + } + } + + $used_vars[$for_loop->getID()] = $var_map; + } + + $foreach_loops = $root->selectDescendantsOfType('n_FOREACH'); + foreach ($foreach_loops as $foreach_loop) { + $var_map = array(); + + $foreach_expr = $foreach_loop->getChildOftype(0, 'n_FOREACH_EXPRESSION'); + + // We might use one or two vars, i.e. "foreach ($x as $y => $z)" or + // "foreach ($x as $y)". + $possible_used_vars = array( + $foreach_expr->getChildByIndex(1), + $foreach_expr->getChildByIndex(2), + ); + foreach ($possible_used_vars as $var) { + if ($var->getTypeName() == 'n_EMPTY') { + continue; + } + $name = $var->getConcreteString(); + $name = trim($name, '&'); // Get rid of ref silliness. + $var_map[$name] = true; + } + + $used_vars[$foreach_loop->getID()] = $var_map; + } + + $all_loops = $for_loops->add($foreach_loops); + foreach ($all_loops as $loop) { + $child_for_loops = $loop->selectDescendantsOfType('n_FOR'); + $child_foreach_loops = $loop->selectDescendantsOfType('n_FOREACH'); + $child_loops = $child_for_loops->add($child_foreach_loops); + + $outer_vars = $used_vars[$loop->getID()]; + foreach ($child_loops as $inner_loop) { + $inner_vars = $used_vars[$inner_loop->getID()]; + $shared = array_intersect_key($outer_vars, $inner_vars); + if ($shared) { + $shared_desc = implode(', ', array_keys($shared)); + $this->raiseLintAtNode( + $inner_loop->getChildByIndex(0), + self::LINT_REUSED_ITERATORS, + "This loop reuses iterator variables ({$shared_desc}) from an ". + "outer loop. You might be clobbering the outer iterator. Change ". + "the inner loop to use a different iterator name."); + } + } + } + } + protected function lintVariableVariables($root) { $vvars = $root->selectDescendantsOfType('n_VARIABLE_VARIABLE'); foreach ($vvars as $vvar) { diff --git a/src/lint/linter/xhpast/__tests__/data/reused-iterators.lint-test b/src/lint/linter/xhpast/__tests__/data/reused-iterators.lint-test new file mode 100644 index 00000000..29d31b72 --- /dev/null +++ b/src/lint/linter/xhpast/__tests__/data/reused-iterators.lint-test @@ -0,0 +1,58 @@ + $jj) { + // Reuse of $ii. + } +} + +for ($jj = 0; $jj < $len; $jj++) { + foreach ($list as $jj) { + // Reuse of $jj. + } +} + +for ($ii = 0; $ii < $len; $ii++) { + foreach ($list as $ii => &$jj) { + // Reuse of $ii. + } +} + +for ($jj = 0; $jj < $len; $jj++) { + foreach ($list as &$jj) { + // Reuse of $jj (by reference). + } +} + +for ($ii = 0; $ii < $len; $ii++) { + for ($jj = 0; $jj < $len; $jj++) { + foreach ($list as $kk) { + // No reuse. + } + } +} + +for ($ii = 0; $ii < $len; $ii++) { + for ($ii = 0; $ii < $len; $ii++) { + // Reuse of $ii, pure for loops. + } +} + +for ($ii = 0; $ii < $len; $ii++) { + for ($jj = $ii; $jj < $jjlen; $jj++) { + // No reuse. + } +} + +foreach ($list as $thing) { + for ($thing = 0; $thing < $len; $thing++) { + // Reuse of $thing, for within foreach. + } +} +~~~~~~~~~~ +error:4:11 +error:10:11 +error:16:11 +error:22:11 +error:36:7 +error:48:7