Skip to content

Commit

Permalink
Split up constant variable warnings into multiple types
Browse files Browse the repository at this point in the history 
For phan#2704
  • Loading branch information
@TysonAndre
TysonAndre committed May 2, 2019
1 parent 963cc8f commit baaf40e
Show file tree
Hide file tree
Showing 4 changed files with 135 additions and 33 deletions.
55 changes: 55 additions & 0 deletions src/Phan/Issue.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,13 @@ class Issue
const UnusedVariableCaughtException = 'PhanUnusedVariableCaughtException'; // has higher false positive rates than UnusedVariable
const UnusedGotoLabel = 'PhanUnusedGotoLabel';
const VariableDefinitionCouldBeConstant = 'PhanVariableDefinitionCouldBeConstant';
const VariableDefinitionCouldBeConstantEmptyArray = 'PhanVariableDefinitionCouldBeConstantEmptyArray';
const VariableDefinitionCouldBeConstantString = 'PhanVariableDefinitionCouldBeConstantString';
const VariableDefinitionCouldBeConstantFloat = 'PhanVariableDefinitionCouldBeConstantFloat';
const VariableDefinitionCouldBeConstantInt = 'PhanVariableDefinitionCouldBeConstantInt';
const VariableDefinitionCouldBeConstantTrue = 'PhanVariableDefinitionCouldBeConstantTrue';
const VariableDefinitionCouldBeConstantFalse = 'PhanVariableDefinitionCouldBeConstantFalse';
const VariableDefinitionCouldBeConstantNull = 'PhanVariableDefinitionCouldBeConstantNull';

// Issue::CATEGORY_REDEFINE
const RedefineClass = 'PhanRedefineClass';
Expand Down Expand Up @@ -2981,6 +2988,54 @@ public static function issueMap()
self::REMEDIATION_B,
6061
),
new Issue(
self::VariableDefinitionCouldBeConstantEmptyArray,
self::CATEGORY_NOOP,
self::SEVERITY_LOW,
'Uses of ${VARIABLE} could probably be replaced with an empty array',
self::REMEDIATION_B,
6062
),
new Issue(
self::VariableDefinitionCouldBeConstantString,
self::CATEGORY_NOOP,
self::SEVERITY_LOW,
'Uses of ${VARIABLE} could probably be replaced with a literal or constant string',
self::REMEDIATION_B,
6063
),
new Issue(
self::VariableDefinitionCouldBeConstantFloat,
self::CATEGORY_NOOP,
self::SEVERITY_LOW,
'Uses of ${VARIABLE} could probably be replaced with a literal or constant float',
self::REMEDIATION_B,
6064
),
new Issue(
self::VariableDefinitionCouldBeConstantInt,
self::CATEGORY_NOOP,
self::SEVERITY_LOW,
'Uses of ${VARIABLE} could probably be replaced with true/false or a named constant',
self::REMEDIATION_B,
6065
),
new Issue(
self::VariableDefinitionCouldBeConstantTrue,
self::CATEGORY_NOOP,
self::SEVERITY_LOW,
'Uses of ${VARIABLE} could probably be replaced with true or a named constant',
self::REMEDIATION_B,
6066
),
new Issue(
self::VariableDefinitionCouldBeConstantFalse,
self::CATEGORY_NOOP,
self::SEVERITY_LOW,
'Uses of ${VARIABLE} could probably be replaced with false or a named constant',
self::REMEDIATION_B,
6067
),

// Issue::CATEGORY_REDEFINE
new Issue(
Expand Down
16 changes: 8 additions & 8 deletions src/Phan/Plugin/Internal/VariableTracker/VariableGraph.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ final class VariableGraph
public $def_lines = [];

/**
* @var array<string,array<int,true>>
* @var array<string,array<int,Node|int|float|string>>
*
* Maps variable id to a set of definition ids that have constant values.
* Maps variable id to a set of definition ids and their corresponding constant AST nodes
*/
public $is_constant_map = [];
public $const_expr_declarations = [];

/**
* @var array<int,true>
Expand Down Expand Up @@ -65,19 +65,19 @@ public function __construct()

/**
* Record the fact that $node is a definition of the variable with name $name in the scope $scope
* @param bool $is_const_expr is the definition's value a value that could be a constant?
* @param ?(Node|string|int|float) $const_expr is the definition's value a value that could be a constant?
* @return void
*/
public function recordVariableDefinition(string $name, Node $node, VariableTrackingScope $scope, bool $is_const_expr)
public function recordVariableDefinition(string $name, Node $node, VariableTrackingScope $scope, $const_expr)
{
// TODO: Measure performance against SplObjectHash
$id = \spl_object_id($node);
if (!isset($this->def_uses[$name][$id])) {
$this->def_uses[$name][$id] = [];
}
$this->def_lines[$name][$id] = $node->lineno;
if ($is_const_expr) {
$this->is_constant_map[$name][$id] = true;
if ($const_expr !== null) {
$this->const_expr_declarations[$name][$id] = $const_expr;
}
$scope->recordDefinitionById($name, $id);
}
Expand Down Expand Up @@ -113,7 +113,7 @@ public function recordVariableUsage(string $name, Node $node, VariableTrackingSc
* Record that $name was modified in place
*/
public function recordVariableModification(string $name) {
$this->is_constant_map[$name][-1] = true;
$this->const_expr_declarations[$name][-1] = 0;
}

/**
Expand Down
38 changes: 25 additions & 13 deletions src/Phan/Plugin/Internal/VariableTracker/VariableTrackerVisitor.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ public function visitAssignRef(Node $node)
self::$variable_graph->recordVariableUsage($name, $var_node, $this->scope);
}
}
return $this->analyzeAssignmentTarget($var_node, false, false);
return $this->analyzeAssignmentTarget($var_node, false, null);
}

/**
Expand Down Expand Up @@ -184,7 +184,7 @@ public function visitAssignOp(Node $node)
// The left-hand node ($var_node) is the usage of this variable
self::$variable_graph->recordVariableUsage($name, $var_node, $this->scope);
// And the whole assignment operation is the redefinition of this variable
self::$variable_graph->recordVariableDefinition($name, $node, $this->scope, false);
self::$variable_graph->recordVariableDefinition($name, $node, $this->scope, null);
$this->scope->recordDefinition($name, $node);
return $this->scope;
case ast\AST_PROP:
Expand All @@ -210,13 +210,22 @@ public function visitAssign(Node $node)
if ($expr instanceof Node) {
$this->scope = $this->analyze($this->scope, $expr);
}
return $this->analyzeAssignmentTarget($node->children['var'], false, ParseVisitor::isConstExpr($expr));
return $this->analyzeAssignmentTarget($node->children['var'], false, self::getConstExprOrNull($expr));
}

/**
* @param Node|string|int|float $expr
* @return Node|string|int|float|null
*/
private static function getConstExprOrNull($expr) {
return ParseVisitor::isConstExpr($expr) ? $expr : null;
}

/**
* @param Node|int|string|float|null $node
* @param Node|int|string|float|null $const_expr
*/
private function analyzeAssignmentTarget($node, bool $is_ref, bool $is_constant) : VariableTrackingScope
private function analyzeAssignmentTarget($node, bool $is_ref, $const_expr) : VariableTrackingScope
{
// TODO: Push onto the node list?
if (!($node instanceof Node)) {
Expand All @@ -231,14 +240,14 @@ private function analyzeAssignmentTarget($node, bool $is_ref, bool $is_constant)
if ($is_ref) {
self::$variable_graph->markAsReference($name);
}
self::$variable_graph->recordVariableDefinition($name, $node, $this->scope, $is_constant);
self::$variable_graph->recordVariableDefinition($name, $node, $this->scope, $const_expr);
$this->scope->recordDefinition($name, $node);
return $this->scope;
case ast\AST_ARRAY:
return $this->analyzeArrayAssignmentTarget($node, $is_constant);
return $this->analyzeArrayAssignmentTarget($node, $const_expr);

case ast\AST_REF:
return $this->analyzeAssignmentTarget($node->children['var'], true, false);
return $this->analyzeAssignmentTarget($node->children['var'], true, null);
case ast\AST_PROP:
return $this->analyzePropAssignmentTarget($node);
case ast\AST_DIM:
Expand All @@ -252,15 +261,18 @@ private function analyzeAssignmentTarget($node, bool $is_ref, bool $is_constant)
return $this->scope;
}

private function analyzeArrayAssignmentTarget(Node $node, bool $is_constant) : VariableTrackingScope
/**
* @param Node|int|string|float|null $const_expr
*/
private function analyzeArrayAssignmentTarget(Node $node, $const_expr) : VariableTrackingScope
{
foreach ($node->children as $elem_node) {
if (!($elem_node instanceof Node)) {
continue;
}
// Treat $key in `[$key => $y] = $array` as a usage of $key
$this->scope = $this->analyzeWhenValidNode($this->scope, $elem_node->children['key']);
$this->scope = $this->analyzeAssignmentTarget($elem_node->children['value'], false, $is_constant);
$this->scope = $this->analyzeAssignmentTarget($elem_node->children['value'], false, $const_expr);
}
return $this->scope;
}
Expand Down Expand Up @@ -390,7 +402,7 @@ public function visitClosure(Node $node)
}

if ($closure_use->flags & ast\flags\PARAM_REF) {
self::$variable_graph->recordVariableDefinition($name, $closure_use, $this->scope, false);
self::$variable_graph->recordVariableDefinition($name, $closure_use, $this->scope, null);
self::$variable_graph->markAsReference($name);
} else {
self::$variable_graph->recordVariableUsage($name, $closure_use, $this->scope);
Expand Down Expand Up @@ -469,13 +481,13 @@ public function visitForeach(Node $node)
$this->scope = new VariableTrackingLoopScope($outer_scope);

$key_node = $node->children['key'];
$this->scope = $this->analyzeAssignmentTarget($key_node, false, false);
$this->scope = $this->analyzeAssignmentTarget($key_node, false, null);

$value_node = $node->children['value'];
if (isset($key_node)) {
self::$variable_graph->markAsLoopValueNode($value_node);
}
$this->scope = $this->analyzeAssignmentTarget($value_node, false, false); // analyzeAssignmentTarget checks for AST_REF
$this->scope = $this->analyzeAssignmentTarget($value_node, false, null); // analyzeAssignmentTarget checks for AST_REF

// TODO: Update graph: inner loop definitions can be used inside the loop.
// TODO: Create a branchScope? - Loop iterations can run 0 times.
Expand Down Expand Up @@ -749,7 +761,7 @@ public function visitCatch(Node $node)
if ($var_node->kind === \ast\AST_VAR) {
$name = $var_node->children['name'];
if (is_string($name)) {
self::$variable_graph->recordVariableDefinition($name, $var_node, $scope, false);
self::$variable_graph->recordVariableDefinition($name, $var_node, $scope, null);
self::$variable_graph->markAsCaughtException($var_node);
$scope->recordDefinition($name, $var_node);
}
Expand Down
59 changes: 47 additions & 12 deletions src/Phan/Plugin/Internal/VariableTrackerPlugin.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ private function addParametersAndUseVariablesToGraph(
// We narrow this down to the specific category if we need to warn.
$result[\spl_object_id($parameter)] = Issue::UnusedPublicMethodParameter;

$graph->recordVariableDefinition($parameter_name, $parameter, $scope, false);
$graph->recordVariableDefinition($parameter_name, $parameter, $scope, null);
if ($parameter->flags & ast\flags\PARAM_REF) {
$graph->markAsReference($parameter_name);
}
Expand All @@ -131,7 +131,7 @@ private function addParametersAndUseVariablesToGraph(
}
$result[\spl_object_id($closure_use)] = Issue::UnusedClosureUseVariable;

$graph->recordVariableDefinition($name, $closure_use, $scope, false);
$graph->recordVariableDefinition($name, $closure_use, $scope, null);
if ($closure_use->flags & ast\flags\PARAM_REF) {
$graph->markAsReference($name);
}
Expand Down Expand Up @@ -279,26 +279,61 @@ private function warnAboutVariableGraph(
// We already warned that this was unused
continue;
}
if (!isset($graph->is_constant_map[$variable_name][$definition_id])) {
$value_node = $graph->const_expr_declarations[$variable_name][$definition_id] ?? null;
if ($value_node === null) {
continue;
}
if (isset($graph->is_constant_map[$variable_name][-1])) {
if (isset($graph->const_expr_declarations[$variable_name][-1])) {
// Set by recordVariableModification
continue;
}
$line = $graph->def_lines[$variable_name][$definition_id] ?? 1;
Issue::maybeEmitWithParameters(
$this->code_base,
$this->context,
Issue::VariableDefinitionCouldBeConstant,
$line,
[$variable_name]
);
$this->warnAboutCouldBeConstant($graph, $variable_name, $definition_id, $value_node);
}
}
}
}

/**
* @param Node|string|int|float $value_node
*/
private function warnAboutCouldBeConstant(VariableGraph $graph, string $variable_name, int $definition_id, $value_node) {
$issue_type = Issue::VariableDefinitionCouldBeConstant;
if ($value_node instanceof Node) {
if ($value_node->kind === ast\AST_ARRAY) {
if (count($value_node->children) === 0) {
$issue_type = Issue::VariableDefinitionCouldBeConstantEmptyArray;
}
} elseif ($value_node->kind === ast\AST_CONST) {
$name = strtolower((string)$value_node->children['name']->children['name'] ?? '');
switch ($name) {
case 'false':
$issue_type = Issue::VariableDefinitionCouldBeConstantFalse;
break;
case 'true':
$issue_type = Issue::VariableDefinitionCouldBeConstantTrue;
break;
case 'null':
$issue_type = Issue::VariableDefinitionCouldBeConstantNull;
break;
}
}
} elseif (is_string($value_node)) {
$issue_type = Issue::VariableDefinitionCouldBeConstantString;
} elseif (is_int($value_node)) {
$issue_type = Issue::VariableDefinitionCouldBeConstantInt;
} elseif (is_float($value_node)) {
$issue_type = Issue::VariableDefinitionCouldBeConstantFloat;
}
$line = $graph->def_lines[$variable_name][$definition_id] ?? 1;
Issue::maybeEmitWithParameters(
$this->code_base,
$this->context,
$issue_type,
$line,
[$variable_name]
);
}

/**
* @return ?Suggestion
*/
Expand Down

0 comments on commit baaf40e

Please sign in to comment.