From fb9f20f4b81524abf876e62b45313e5b37edd5fb Mon Sep 17 00:00:00 2001 From: Matthew Brown Date: Wed, 10 Jan 2018 23:29:18 -0500 Subject: [PATCH] Find unused properties with dead code checks Fixes #424 --- config.xsd | 2 + psalm.xml | 12 +++ src/Psalm/Checker/ClassLikeChecker.php | 36 +++---- src/Psalm/Checker/FileChecker.php | 20 ---- src/Psalm/Checker/FunctionLikeChecker.php | 95 +++++++++---------- src/Psalm/Checker/MethodChecker.php | 1 - src/Psalm/Checker/NamespaceChecker.php | 15 --- src/Psalm/Checker/ProjectChecker.php | 38 +++++++- .../Checker/Statements/Block/IfChecker.php | 1 - .../Statements/Expression/FetchChecker.php | 50 ++++++++-- src/Psalm/Config.php | 12 --- src/Psalm/Config/FileFilter.php | 5 - src/Psalm/Issue/PossiblyUnusedProperty.php | 6 ++ src/Psalm/Issue/UnusedProperty.php | 6 ++ src/Psalm/Scope/IfScope.php | 5 - src/Psalm/Storage/ClassLikeStorage.php | 10 -- src/Psalm/Storage/FunctionLikeStorage.php | 5 - src/Psalm/Storage/MethodStorage.php | 10 -- src/Psalm/Storage/PropertyStorage.php | 5 + src/Psalm/Type/Atomic/Fn.php | 6 +- src/Psalm/Visitor/DependencyFinderVisitor.php | 5 - tests/ConfigTest.php | 3 - tests/TestCase.php | 3 - tests/TypeTest.php | 9 ++ tests/UnusedCodeTest.php | 7 +- 25 files changed, 190 insertions(+), 177 deletions(-) create mode 100644 src/Psalm/Issue/PossiblyUnusedProperty.php create mode 100644 src/Psalm/Issue/UnusedProperty.php diff --git a/config.xsd b/config.xsd index ee0f700dd..65a0d1c3c 100644 --- a/config.xsd +++ b/config.xsd @@ -203,6 +203,7 @@ + @@ -233,6 +234,7 @@ + diff --git a/psalm.xml b/psalm.xml index 99da9f75c..9ebfd69d9 100644 --- a/psalm.xml +++ b/psalm.xml @@ -45,6 +45,18 @@ + + + + + + + + + + + + diff --git a/src/Psalm/Checker/ClassLikeChecker.php b/src/Psalm/Checker/ClassLikeChecker.php index 564615ead..0b72ff197 100644 --- a/src/Psalm/Checker/ClassLikeChecker.php +++ b/src/Psalm/Checker/ClassLikeChecker.php @@ -88,11 +88,6 @@ abstract class ClassLikeChecker extends SourceChecker implements StatementsSourc */ protected $fq_class_name; - /** - * @var bool - */ - protected $has_custom_get = false; - /** * The parent class * @@ -100,16 +95,6 @@ abstract class ClassLikeChecker extends SourceChecker implements StatementsSourc */ protected $parent_fq_class_name; - /** - * @var array - */ - protected $method_checkers = []; - - /** - * @var array - */ - protected $property_types = []; - /** * @var array>|null */ @@ -1552,8 +1537,11 @@ abstract class ClassLikeChecker extends SourceChecker implements StatementsSourc * * @return bool */ - public static function propertyExists(ProjectChecker $project_checker, $property_id) - { + public static function propertyExists( + ProjectChecker $project_checker, + $property_id, + CodeLocation $code_location = null + ) { // remove trailing backslash if it exists $property_id = preg_replace('/^\\\\/', '', $property_id); @@ -1562,6 +1550,20 @@ abstract class ClassLikeChecker extends SourceChecker implements StatementsSourc $class_storage = $project_checker->classlike_storage_provider->get($fq_class_name); if (isset($class_storage->declaring_property_ids[$property_name])) { + if ($project_checker->collect_references && $code_location) { + $declaring_property_id = $class_storage->declaring_property_ids[$property_name]; + list($declaring_property_class, $declaring_property_name) = explode('::$', $declaring_property_id); + + $declaring_class_storage = $project_checker->classlike_storage_provider->get($declaring_property_class); + $declaring_property_storage = $declaring_class_storage->properties[$declaring_property_name]; + + if ($declaring_property_storage->referencing_locations === null) { + $declaring_property_storage->referencing_locations = []; + } + + $declaring_property_storage->referencing_locations[$code_location->file_path][] = $code_location; + } + return true; } diff --git a/src/Psalm/Checker/FileChecker.php b/src/Psalm/Checker/FileChecker.php index 8a3622b3d..d92296e45 100644 --- a/src/Psalm/Checker/FileChecker.php +++ b/src/Psalm/Checker/FileChecker.php @@ -48,16 +48,6 @@ class FileChecker extends SourceChecker implements StatementsSource */ protected $namespace_aliased_classes_flipped = []; - /** - * @var array - */ - protected $preloaded_statements = []; - - /** - * @var bool - */ - public static $show_notices = true; - /** * @var array */ @@ -73,16 +63,6 @@ class FileChecker extends SourceChecker implements StatementsSource */ protected $function_checkers = []; - /** - * @var array - */ - protected $namespace_checkers = []; - - /** - * @var array - */ - private $included_file_paths = []; - /** * @var ?Context */ diff --git a/src/Psalm/Checker/FunctionLikeChecker.php b/src/Psalm/Checker/FunctionLikeChecker.php index cf0ea20c2..8dcddad75 100644 --- a/src/Psalm/Checker/FunctionLikeChecker.php +++ b/src/Psalm/Checker/FunctionLikeChecker.php @@ -59,11 +59,6 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo */ protected $is_static = false; - /** - * @var StatementsChecker|null - */ - protected $statements_checker; - /** * @var StatementsSource */ @@ -259,11 +254,6 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo $statements_checker = new StatementsChecker($this); - // this increases memory, so only do it if running under this flag - if ($project_checker->infer_types_from_usage) { - $this->statements_checker = $statements_checker; - } - $template_types = $storage->template_types; if ($class_storage && $class_storage->template_types) { @@ -439,14 +429,7 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo [], false ); - } elseif (!$this->function instanceof Closure) { - $fleshed_out_return_type = ExpressionChecker::fleshOutType( - $project_checker, - $storage->return_type, - $context->self, - $cased_method_id - ); - + } else { $fleshed_out_signature_type = ExpressionChecker::fleshOutType( $project_checker, $storage->signature_return_type, @@ -454,38 +437,54 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo $cased_method_id ); - if (!TypeChecker::isContainedBy( - $project_checker, - $fleshed_out_return_type, - $fleshed_out_signature_type - ) - ) { - if ($project_checker->alter_code - && isset($project_checker->getIssuesToFix()['MismatchingDocblockReturnType']) - ) { - $this->addOrUpdateReturnType($project_checker, $storage->signature_return_type); + $fleshed_out_signature_type->check( + $this, + $storage->signature_return_type_location ?: $storage->return_type_location, + $storage->suppressed_issues, + [], + false + ); - return null; - } - - if (IssueBuffer::accepts( - new MismatchingDocblockReturnType( - 'Docblock has incorrect return type \'' . $storage->return_type . - '\', should be \'' . $storage->signature_return_type . '\'', - $storage->return_type_location - ), - $storage->suppressed_issues - )) { - return false; - } - - $storage->signature_return_type->check( - $this, - $storage->return_type_location, - $storage->suppressed_issues, - [], - false + if (!$this->function instanceof Closure) { + $fleshed_out_return_type = ExpressionChecker::fleshOutType( + $project_checker, + $storage->return_type, + $context->self, + $cased_method_id ); + + $fleshed_out_signature_type = ExpressionChecker::fleshOutType( + $project_checker, + $storage->signature_return_type, + $context->self, + $cased_method_id + ); + + if (!TypeChecker::isContainedBy( + $project_checker, + $fleshed_out_return_type, + $fleshed_out_signature_type + ) + ) { + if ($project_checker->alter_code + && isset($project_checker->getIssuesToFix()['MismatchingDocblockReturnType']) + ) { + $this->addOrUpdateReturnType($project_checker, $storage->signature_return_type); + + return null; + } + + if (IssueBuffer::accepts( + new MismatchingDocblockReturnType( + 'Docblock has incorrect return type \'' . $storage->return_type . + '\', should be \'' . $storage->signature_return_type . '\'', + $storage->return_type_location + ), + $storage->suppressed_issues + )) { + return false; + } + } } } } diff --git a/src/Psalm/Checker/MethodChecker.php b/src/Psalm/Checker/MethodChecker.php index 60d85b189..55644d68d 100644 --- a/src/Psalm/Checker/MethodChecker.php +++ b/src/Psalm/Checker/MethodChecker.php @@ -177,7 +177,6 @@ class MethodChecker extends FunctionLikeChecker $declaring_class = $method->getDeclaringClass(); $storage->is_static = $method->isStatic(); - $storage->namespace = $declaring_class->getNamespaceName(); $class_storage->declaring_method_ids[$method_name] = $declaring_class->name . '::' . strtolower((string)$method->getName()); diff --git a/src/Psalm/Checker/NamespaceChecker.php b/src/Psalm/Checker/NamespaceChecker.php index 3a015c938..0c1fccd90 100644 --- a/src/Psalm/Checker/NamespaceChecker.php +++ b/src/Psalm/Checker/NamespaceChecker.php @@ -26,21 +26,6 @@ class NamespaceChecker extends SourceChecker implements StatementsSource */ private $namespace_name; - /** - * @var array - */ - public $function_checkers = []; - - /** - * @var array - */ - public $class_checkers = []; - - /** - * @var array - */ - public $interface_checkers = []; - /** * A lookup table for public namespace constants * diff --git a/src/Psalm/Checker/ProjectChecker.php b/src/Psalm/Checker/ProjectChecker.php index abc1ae10f..c2a4aff89 100644 --- a/src/Psalm/Checker/ProjectChecker.php +++ b/src/Psalm/Checker/ProjectChecker.php @@ -9,8 +9,10 @@ use Psalm\FileManipulation\FunctionDocblockManipulator; use Psalm\Issue\CircularReference; use Psalm\Issue\PossiblyUnusedMethod; use Psalm\Issue\PossiblyUnusedParam; +use Psalm\Issue\PossiblyUnusedProperty; use Psalm\Issue\UnusedClass; use Psalm\Issue\UnusedMethod; +use Psalm\Issue\UnusedProperty; use Psalm\IssueBuffer; use Psalm\Provider\ClassLikeStorageProvider; use Psalm\Provider\FileProvider; @@ -181,11 +183,6 @@ class ProjectChecker */ private $scanned_files = []; - /** - * @var array - */ - private $visited_classes = []; - /** * @var array */ @@ -1274,6 +1271,37 @@ class ProjectChecker } } } + + foreach ($classlike_storage->properties as $property_name => $property_storage) { + if (($property_storage->referencing_locations === null + || count($property_storage->referencing_locations) === 0) + && (substr($property_name, 0, 2) !== '__' || $property_name === '__construct') + && $property_storage->location + ) { + $property_id = $classlike_storage->name . '::$' . $property_name; + + if ($property_storage->visibility === ClassLikeChecker::VISIBILITY_PUBLIC) { + if (IssueBuffer::accepts( + new PossiblyUnusedProperty( + 'Cannot find public calls to property ' . $property_id, + $property_storage->location + ), + $classlike_storage->suppressed_issues + )) { + // fall through + } + } elseif (!isset($classlike_storage->declaring_method_ids['__get'])) { + if (IssueBuffer::accepts( + new UnusedProperty( + 'Property ' . $property_id . ' is never used', + $property_storage->location + ) + )) { + // fall through + } + } + } + } } /** diff --git a/src/Psalm/Checker/Statements/Block/IfChecker.php b/src/Psalm/Checker/Statements/Block/IfChecker.php index 48013298a..6813d63eb 100644 --- a/src/Psalm/Checker/Statements/Block/IfChecker.php +++ b/src/Psalm/Checker/Statements/Block/IfChecker.php @@ -89,7 +89,6 @@ class IfChecker $context->inside_conditional = false; $if_scope = new IfScope(); - $if_scope->has_elseifs = count($stmt->elseifs) > 0; $if_context = clone $context; diff --git a/src/Psalm/Checker/Statements/Expression/FetchChecker.php b/src/Psalm/Checker/Statements/Expression/FetchChecker.php index a5ecb5d0d..d79c99183 100644 --- a/src/Psalm/Checker/Statements/Expression/FetchChecker.php +++ b/src/Psalm/Checker/Statements/Expression/FetchChecker.php @@ -80,6 +80,8 @@ class FetchChecker return false; } + $project_checker = $statements_checker->getFileChecker()->project_checker; + $stmt_var_id = ExpressionChecker::getVarId( $stmt->var, $statements_checker->getFQCLN(), @@ -98,6 +100,25 @@ class FetchChecker // we don't need to check anything $stmt->inferredType = $context->vars_in_scope[$var_id]; + if ($context->collect_references + && isset($stmt->var->inferredType) + && $stmt->var->inferredType->hasObjectType() + && is_string($stmt->name) + ) { + // log the appearance + foreach ($stmt->var->inferredType->getTypes() as $lhs_type_part) { + if ($lhs_type_part instanceof TNamedObject) { + $property_id = $lhs_type_part->value . '::$' . $stmt->name; + + ClassLikeChecker::propertyExists( + $project_checker, + $property_id, + new CodeLocation($statements_checker->getSource(), $stmt) + ); + } + } + } + return null; } @@ -208,8 +229,6 @@ class FetchChecker continue; } - $project_checker = $statements_checker->getFileChecker()->project_checker; - if (!ClassChecker::classExists($project_checker, $lhs_type_part->value)) { if (InterfaceChecker::interfaceExists($project_checker, $lhs_type_part->value)) { if (IssueBuffer::accepts( @@ -260,7 +279,12 @@ class FetchChecker } } - if (!ClassLikeChecker::propertyExists($project_checker, $property_id)) { + if (!ClassLikeChecker::propertyExists( + $project_checker, + $property_id, + $context->collect_references ? new CodeLocation($statements_checker->getSource(), $stmt) : null + ) + ) { if ($context->inside_isset) { return; } @@ -699,16 +723,30 @@ class FetchChecker $statements_checker ); + $property_id = $fq_class_name . '::$' . $stmt->name; + if ($var_id && $context->hasVariable($var_id)) { // we don't need to check anything $stmt->inferredType = $context->vars_in_scope[$var_id]; + if ($context->collect_references) { + // log the appearance + ClassLikeChecker::propertyExists( + $project_checker, + $property_id, + new CodeLocation($statements_checker->getSource(), $stmt) + ); + } + return null; } - $property_id = $fq_class_name . '::$' . $stmt->name; - - if (!ClassLikeChecker::propertyExists($project_checker, $property_id)) { + if (!ClassLikeChecker::propertyExists( + $project_checker, + $property_id, + $context->collect_references ? new CodeLocation($statements_checker->getSource(), $stmt) : null + ) + ) { if (IssueBuffer::accepts( new UndefinedPropertyFetch( 'Static property ' . $property_id . ' is not defined', diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php index abb54de51..399e048f2 100644 --- a/src/Psalm/Config.php +++ b/src/Psalm/Config.php @@ -110,13 +110,6 @@ class Config */ protected $base_dir; - /** - * The path to this config file - * - * @var string - */ - public $file_path; - /** * @var array */ @@ -186,9 +179,6 @@ class Config */ private $plugins = []; - /** @var array */ - private $predefined_classlikes = []; - /** @var array */ private $predefined_constants; @@ -238,8 +228,6 @@ class Config { $config = new static(); - $config->file_path = $file_path; - $config->base_dir = $base_dir; $schema_path = dirname(dirname(__DIR__)) . '/config.xsd'; diff --git a/src/Psalm/Config/FileFilter.php b/src/Psalm/Config/FileFilter.php index 160bf7505..10720b4ef 100644 --- a/src/Psalm/Config/FileFilter.php +++ b/src/Psalm/Config/FileFilter.php @@ -21,11 +21,6 @@ class FileFilter */ protected $files_lowercase = []; - /** - * @var array - */ - protected $patterns = []; - /** * @var bool */ diff --git a/src/Psalm/Issue/PossiblyUnusedProperty.php b/src/Psalm/Issue/PossiblyUnusedProperty.php new file mode 100644 index 000000000..1efff6293 --- /dev/null +++ b/src/Psalm/Issue/PossiblyUnusedProperty.php @@ -0,0 +1,6 @@ + */ diff --git a/src/Psalm/Storage/ClassLikeStorage.php b/src/Psalm/Storage/ClassLikeStorage.php index a9418cf13..a17e9fdd2 100644 --- a/src/Psalm/Storage/ClassLikeStorage.php +++ b/src/Psalm/Storage/ClassLikeStorage.php @@ -32,11 +32,6 @@ class ClassLikeStorage */ public $populated = false; - /** - * @var bool - */ - public $reflected = false; - /** * @var bool */ @@ -62,11 +57,6 @@ class ClassLikeStorage */ public $name; - /** - * @var array - */ - public $aliased_classes; - /** * Is this class user-defined * diff --git a/src/Psalm/Storage/FunctionLikeStorage.php b/src/Psalm/Storage/FunctionLikeStorage.php index edec3d9bc..9a3817e16 100644 --- a/src/Psalm/Storage/FunctionLikeStorage.php +++ b/src/Psalm/Storage/FunctionLikeStorage.php @@ -22,11 +22,6 @@ class FunctionLikeStorage */ public $param_types = []; - /** - * @var string - */ - public $namespace; - /** * @var Type\Union|null */ diff --git a/src/Psalm/Storage/MethodStorage.php b/src/Psalm/Storage/MethodStorage.php index 14f8ed57c..1be3c8fe3 100644 --- a/src/Psalm/Storage/MethodStorage.php +++ b/src/Psalm/Storage/MethodStorage.php @@ -10,16 +10,6 @@ class MethodStorage extends FunctionLikeStorage */ public $is_static; - /** - * @var bool - */ - public $reflected; - - /** - * @var bool - */ - public $registered; - /** * @var int */ diff --git a/src/Psalm/Storage/PropertyStorage.php b/src/Psalm/Storage/PropertyStorage.php index 2956e05a0..60787a3d9 100644 --- a/src/Psalm/Storage/PropertyStorage.php +++ b/src/Psalm/Storage/PropertyStorage.php @@ -45,4 +45,9 @@ class PropertyStorage * @var bool */ public $deprecated = false; + + /** + * @var array>|null + */ + public $referencing_locations; } diff --git a/src/Psalm/Type/Atomic/Fn.php b/src/Psalm/Type/Atomic/Fn.php index 73029bec8..0877a7bff 100644 --- a/src/Psalm/Type/Atomic/Fn.php +++ b/src/Psalm/Type/Atomic/Fn.php @@ -6,11 +6,6 @@ use Psalm\Type\Union; class Fn extends TNamedObject { - /** - * @var string - */ - public $value = 'Closure'; - /** * @var array */ @@ -30,6 +25,7 @@ class Fn extends TNamedObject */ public function __construct($value, array $params, Union $return_type) { + $this->value = 'Closure'; $this->params = $params; $this->return_type = $return_type; } diff --git a/src/Psalm/Visitor/DependencyFinderVisitor.php b/src/Psalm/Visitor/DependencyFinderVisitor.php index e4f74364a..39bc76cde 100644 --- a/src/Psalm/Visitor/DependencyFinderVisitor.php +++ b/src/Psalm/Visitor/DependencyFinderVisitor.php @@ -41,11 +41,6 @@ class DependencyFinderVisitor extends PhpParser\NodeVisitorAbstract implements P /** @var Aliases */ protected $file_aliases; - /** - * @var bool - */ - protected $in_classlike = false; - /** * @var string[] */ diff --git a/tests/ConfigTest.php b/tests/ConfigTest.php index d54cac020..38a16a874 100644 --- a/tests/ConfigTest.php +++ b/tests/ConfigTest.php @@ -6,9 +6,6 @@ use Psalm\Config; class ConfigTest extends TestCase { - /** @var \PhpParser\Parser */ - protected static $parser; - /** @var TestConfig */ protected static $config; diff --git a/tests/TestCase.php b/tests/TestCase.php index 5833ad399..30ce6fdea 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -7,9 +7,6 @@ use Psalm\Checker\ProjectChecker; class TestCase extends BaseTestCase { - /** @var \PhpParser\Parser */ - protected static $parser; - /** @var TestConfig */ protected static $config; diff --git a/tests/TypeTest.php b/tests/TypeTest.php index 02ca5dd42..b47321bcc 100644 --- a/tests/TypeTest.php +++ b/tests/TypeTest.php @@ -958,6 +958,15 @@ class TypeTest extends TestCase public function providerFileCheckerInvalidCodeParse() { return [ + 'possiblyUndefinedVariable' => [ + ' 'PossiblyUndefinedGlobalVariable', + ], 'nullableMethodCall' => [ 'value = $value; } + + public function getFoo() : string { + return $this->value; + } } $m = new A(); $m->foo("value"); - $m->modifyFoo("value2");', + $m->modifyFoo("value2"); + echo $m->getFoo();', ], 'usedTraitMethod' => [ '