diff --git a/src/lint/linter/ArcanistXHPASTLinter.php b/src/lint/linter/ArcanistXHPASTLinter.php index 5501cd1f..0b4730a4 100644 --- a/src/lint/linter/ArcanistXHPASTLinter.php +++ b/src/lint/linter/ArcanistXHPASTLinter.php @@ -46,6 +46,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter { const LINT_SLOWNESS = 36; const LINT_CLOSING_CALL_PAREN = 37; const LINT_CLOSING_DECL_PAREN = 38; + const LINT_REUSED_ITERATOR_REFERENCE = 39; public function getLintNameMap() { return array( @@ -85,6 +86,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter { self::LINT_SLOWNESS => 'Slow Construct', self::LINT_CLOSING_CALL_PAREN => 'Call Formatting', self::LINT_CLOSING_DECL_PAREN => 'Declaration Formatting', + self::LINT_REUSED_ITERATOR_REFERENCE => 'Reuse of Iterator References', ); } @@ -112,6 +114,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter { self::LINT_COMMENT_SPACING => $advice, self::LINT_CLOSING_CALL_PAREN => $warning, self::LINT_CLOSING_DECL_PAREN => $warning, + self::LINT_REUSED_ITERATOR_REFERENCE => $warning, // This is disabled by default because it implies a very strict policy // which isn't necessary in the general case. @@ -184,6 +187,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter { 'lintCommentSpaces' => self::LINT_COMMENT_SPACING, 'lintHashComments' => self::LINT_COMMENT_STYLE, 'lintReusedIterators' => self::LINT_REUSED_ITERATORS, + 'lintReusedIteratorReferences' => self::LINT_REUSED_ITERATOR_REFERENCE, 'lintVariableVariables' => self::LINT_VARIABLE_VARIABLE, 'lintUndeclaredVariables' => array( self::LINT_EXTRACT_USE, @@ -874,6 +878,177 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter { } } + /** + * Find cases where a foreach loop is being iterated using a variable + * reference and the same variable is used outside of the loop without + * calling unset() or reassigning the variable to another variable + * reference. + * + * COUNTEREXAMPLE + * foreach ($ar as &$a) { + * // ... + * } + * $a = 1; // <-- Raises an error for using $a + * + */ + protected function lintReusedIteratorReferences($root) { + + $fdefs = $root->selectDescendantsOfType('n_FUNCTION_DECLARATION'); + $mdefs = $root->selectDescendantsOfType('n_METHOD_DECLARATION'); + $defs = $fdefs->add($mdefs); + + foreach ($defs as $def) { + + $body = $def->getChildByIndex(5); + if ($body->getTypeName() == 'n_EMPTY') { + // Abstract method declaration. + continue; + } + + $exclude = array(); + + // Exclude uses of variables, unsets, and foreach loops + // within closures - they are checked on their own + $func_defs = $body->selectDescendantsOfType('n_FUNCTION_DECLARATION'); + foreach ($func_defs as $func_def) { + $vars = $func_def->selectDescendantsOfType('n_VARIABLE'); + foreach ($vars as $var) { + $exclude[$var->getID()] = true; + } + + $unset_lists = $func_def->selectDescendantsOfType('n_UNSET_LIST'); + foreach ($unset_lists as $unset_list) { + $exclude[$unset_list->getID()] = true; + } + + $foreaches = $func_def->selectDescendantsOfType('n_FOREACH'); + foreach ($foreaches as $foreach) { + $exclude[$foreach->getID()] = true; + } + } + + // Find all variables that are unset within the scope + $unset_vars = array(); + $unset_lists = $body->selectDescendantsOfType('n_UNSET_LIST'); + foreach ($unset_lists as $unset_list) { + if (isset($exclude[$unset_list->getID()])) { + continue; + } + + $unset_list_vars = $unset_list->selectDescendantsOfType('n_VARIABLE'); + foreach ($unset_list_vars as $var) { + $concrete = $this->getConcreteVariableString($var); + $unset_vars[$concrete][] = $var->getOffset(); + $exclude[$var->getID()] = true; + } + } + + // Find all reference variables in foreach expressions + $reference_vars = array(); + $foreaches = $body->selectDescendantsOfType('n_FOREACH'); + foreach ($foreaches as $foreach) { + if (isset($exclude[$foreach->getID()])) { + continue; + } + + $foreach_expr = $foreach->getChildOfType(0, 'n_FOREACH_EXPRESSION'); + $var = $foreach_expr->getChildByIndex(2); + if ($var->getTypeName() != 'n_VARIABLE_REFERENCE') { + continue; + } + + $reference = $var->getChildByIndex(0); + if ($reference->getTypeName() != 'n_VARIABLE') { + continue; + } + + $reference_name = $this->getConcreteVariableString($reference); + $reference_vars[$reference_name][] = $reference->getOffset(); + $exclude[$reference->getID()] = true; + + // Exclude uses of the reference variable within the foreach loop + $foreach_vars = $foreach->selectDescendantsOfType('n_VARIABLE'); + foreach ($foreach_vars as $var) { + $name = $this->getConcreteVariableString($var); + if ($name == $reference_name) { + $exclude[$var->getID()] = true; + } + } + } + + // Allow usage if the reference variable is assigned to another + // reference variable + $binary = $body->selectDescendantsOfType('n_BINARY_EXPRESSION'); + foreach ($binary as $expr) { + if ($expr->getChildByIndex(1)->getConcreteString() != '=') { + continue; + } + $lval = $expr->getChildByIndex(0); + if ($lval->getTypeName() != 'n_VARIABLE') { + continue; + } + $rval = $expr->getChildByIndex(2); + if ($rval->getTypeName() != 'n_VARIABLE_REFERENCE') { + continue; + } + + // Counts as unsetting a variable + $concrete = $this->getConcreteVariableString($lval); + $unset_vars[$concrete][] = $lval->getOffset(); + $exclude[$lval->getID()] = true; + } + + $all_vars = array(); + $all = $body->selectDescendantsOfType('n_VARIABLE'); + foreach ($all as $var) { + if (isset($exclude[$var->getID()])) { + continue; + } + + $name = $this->getConcreteVariableString($var); + + if (!isset($reference_vars[$name])) { + continue; + } + + // Find the closest reference offset to this variable + $reference_offset = null; + foreach ($reference_vars[$name] as $offset) { + if ($offset < $var->getOffset()) { + $reference_offset = $offset; + } else { + break; + } + } + if (!$reference_offset) { + continue; + } + + // Check if an unset exists between reference and usage of this + // variable + $warn = true; + if (isset($unset_vars[$name])) { + foreach ($unset_vars[$name] as $unset_offset) { + if ($unset_offset > $reference_offset && + $unset_offset < $var->getOffset()) { + $warn = false; + break; + } + } + } + if ($warn) { + $this->raiseLintAtNode( + $var, + self::LINT_REUSED_ITERATOR_REFERENCE, + 'This variable was used already as a by-reference iterator ' . + 'variable. Such variables survive outside the foreach loop, ' . + 'do not reuse.'); + } + } + + } + } + protected function lintVariableVariables($root) { $vvars = $root->selectDescendantsOfType('n_VARIABLE_VARIABLE'); foreach ($vvars as $vvar) { @@ -1131,7 +1306,11 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter { // foreach ($a as $b => &$c) { ... } // Reach into the n_VARIABLE_REFERENCE node to grab the n_VARIABLE // node. - $foreach_vars[] = $value_var->getChildOfType(0, 'n_VARIABLE'); + $var = $value_var->getChildByIndex(0); + if ($var->getTypeName() == 'n_VARIABLE_VARIABLE') { + $var = $var->getChildByIndex(0); + } + $foreach_vars[] = $var; } // Remove all uses of the iterators inside of the foreach() loop from diff --git a/src/lint/linter/__tests__/xhpast/reused-iterator-reference.lint-test b/src/lint/linter/__tests__/xhpast/reused-iterator-reference.lint-test new file mode 100644 index 00000000..78d6be83 --- /dev/null +++ b/src/lint/linter/__tests__/xhpast/reused-iterator-reference.lint-test @@ -0,0 +1,137 @@ + &$v) { } + $v++; // Reuse of $v +} + +function key_value2() { + $ar = array(); + foreach ($ar as $k => $v) { } + $v++; +} + +function unset() { + $ar = array(); + foreach ($ar as &$a) { } + unset($a); + $a++; +} + +function unset2() { + $ar = array(); + foreach ($ar as &$a) { } + $a++; // Reuse of $a + unset($a); +} + +function twice_ref() { + $ar1 = array(); + $ar2 = array(); + foreach ($ar1 as &$b) { } + foreach ($ar2 as &$b) { } +} + +function assign_ref(&$a) { + $ar = array(); + foreach ($ar as &$b) { } + $b = &$a; +} + +function assign_ref2(&$a) { + $ar = array(); + foreach ($ar as &$b) { } + $b = &$a; + $c = $b; +} + +function use_inside() { + $ar = array(); + foreach ($ar as &$a) { + $a++; + } +} + +function variable_variable() { + $ar = array(); + foreach ($ar as &$$a) { } + $a++; + $$a++; +} + +function closure1() { + $ar = array(); + foreach ($ar as &$a) { } + function($a) { + $a++; + }; +} + +function closure2() { + function() { + $ar = array(); + foreach ($ar as &$a) { } + }; + $a++; +} + +function closure3() { + function() { + $ar = array(); + foreach ($ar as &$a) { } + $a++; // Reuse of $a + }; +} + +function closure4() { + $ar = array(); + foreach ($ar as &$a) { } + function($a) { + unset($a); + }; + $a++; // Reuse of $a +} + +~~~~~~~~~~ +warning:6:3 +warning:12:8 +warning:18:10 +warning:27:20 +warning:33:3 +warning:52:3 +error:85:20 +error:87:3 +disabled:93:3 +disabled:99:3 +disabled:107:3 +warning:110:5 +disabled:117:3 +warning:120:3 diff --git a/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test b/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test index 7aa359c5..0801ed8f 100644 --- a/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test +++ b/src/lint/linter/__tests__/xhpast/undeclared-variables.lint-test @@ -171,9 +171,10 @@ error:46:5 error:47:5 error:48:10 error:53:10 worst ever +warning:65:3 error:91:3 This stuff is basically testing the lexer/parser for function decls. error:108:15 Variables in instance derefs should be checked, static should not. error:121:3 isset() and empty() should not trigger errors. error:125:3 Should only warn once in this function. error:146:8 -error:152:9 \ No newline at end of file +error:152:9