From 928b01a7c7bf5d0ff9a2c8555b3c53fa584126ea Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Fri, 5 Jan 2018 19:49:27 -0500 Subject: [PATCH] Find and fix code issues where functionality is available --- src/Psalm/Checker/FileChecker.php | 2 + src/Psalm/Checker/FunctionLikeChecker.php | 54 +++++-------------- src/Psalm/Checker/ProjectChecker.php | 51 +++++++++++------- src/Psalm/Checker/StatementsChecker.php | 2 +- .../FileManipulationBuffer.php | 8 +++ src/Psalm/Issue/CodeIssue.php | 6 ++- .../Issue/FalsableInferredReturnType.php | 2 +- src/Psalm/Issue/FixableCodeIssue.php | 35 ++++++++++++ src/Psalm/Issue/InvalidReturnType.php | 2 +- .../Issue/MismatchingDocblockParamType.php | 2 +- .../Issue/MismatchingDocblockReturnType.php | 2 +- .../Issue/NullableInferredReturnType.php | 2 +- src/Psalm/IssueBuffer.php | 19 +++++++ src/psalm.php | 10 ++-- tests/ConfigTest.php | 5 +- tests/FileManipulationTest.php | 4 +- tests/ReportOutputTest.php | 2 - 17 files changed, 132 insertions(+), 76 deletions(-) create mode 100644 src/Psalm/Issue/FixableCodeIssue.php diff --git a/src/Psalm/Checker/FileChecker.php b/src/Psalm/Checker/FileChecker.php index e16b8c851..8a3622b3d 100644 --- a/src/Psalm/Checker/FileChecker.php +++ b/src/Psalm/Checker/FileChecker.php @@ -4,6 +4,7 @@ namespace Psalm\Checker; use PhpParser; use Psalm\Config; use Psalm\Context; +use Psalm\FileManipulation\FileManipulationBuffer; use Psalm\IssueBuffer; use Psalm\StatementsSource; use Psalm\Type; @@ -426,6 +427,7 @@ class FileChecker extends SourceChecker implements StatementsSource FunctionChecker::clearCache(); StatementsChecker::clearCache(); IssueBuffer::clearCache(); + FileManipulationBuffer::clearCache(); FunctionLikeChecker::clearCache(); } diff --git a/src/Psalm/Checker/FunctionLikeChecker.php b/src/Psalm/Checker/FunctionLikeChecker.php index e650ebab5..2793c89cc 100644 --- a/src/Psalm/Checker/FunctionLikeChecker.php +++ b/src/Psalm/Checker/FunctionLikeChecker.php @@ -309,7 +309,8 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo new MismatchingDocblockParamType( 'Parameter $' . $function_param->name . ' has wrong type \'' . $param_type . '\', should be \'' . $signature_type . '\'', - $function_param->type_location + $function_param->type_location, + (string)$signature_type ), $storage->suppressed_issues )) { @@ -416,7 +417,8 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo new MismatchingDocblockReturnType( 'Docblock has incorrect return type \'' . $storage->return_type . '\', should be \'' . $storage->signature_return_type . '\'', - $storage->return_type_location + $storage->return_type_location, + (string) $storage->signature_return_type ), $storage->suppressed_issues )) { @@ -1162,7 +1164,7 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo ) ); - if (!$return_type && !$project_checker->update_docblocks && !$is_to_string) { + if (!$return_type && !$project_checker->add_docblocks && !$is_to_string) { if ($this->function instanceof Closure) { if (IssueBuffer::accepts( new MissingClosureReturnType( @@ -1204,13 +1206,12 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo } } - if (!$project_checker->update_docblocks) { - return null; - } + return null; } if (!$return_type) { if (!$inferred_return_type->isMixed()) { + // $project_checker->add_docblocks is always true here $this->addDocblockReturnType($project_checker, $inferred_return_type); } @@ -1284,19 +1285,12 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo && !$declared_return_type->isNullable() && !$declared_return_type->isVoid() ) { - if ($project_checker->update_docblocks) { - if (!in_array('InvalidReturnType', $this->suppressed_issues, true)) { - $this->addDocblockReturnType($project_checker, $inferred_return_type); - } - - return null; - } - if (IssueBuffer::accepts( new NullableInferredReturnType( 'The declared return type \'' . $declared_return_type . '\' for ' . $cased_method_id . ' is not nullable, but \'' . $inferred_return_type . '\' contains null', - $return_type_location + $return_type_location, + (string) $inferred_return_type ), $this->suppressed_issues )) { @@ -1308,19 +1302,12 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo && !$declared_return_type->isFalsable() && !$declared_return_type->hasBool() ) { - if ($project_checker->update_docblocks) { - if (!in_array('InvalidReturnType', $this->suppressed_issues, true)) { - $this->addDocblockReturnType($project_checker, $inferred_return_type); - } - - return null; - } - if (IssueBuffer::accepts( new FalsableInferredReturnType( 'The declared return type \'' . $declared_return_type . '\' for ' . $cased_method_id . ' does not allow false, but \'' . $inferred_return_type . '\' contains false', - $return_type_location + $return_type_location, + (string) $inferred_return_type ), $this->suppressed_issues )) { @@ -1338,14 +1325,6 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo $type_coerced, $type_coerced_from_mixed )) { - if ($project_checker->update_docblocks) { - if (!in_array('InvalidReturnType', $this->suppressed_issues, true)) { - $this->addDocblockReturnType($project_checker, $inferred_return_type); - } - - return null; - } - // is the declared return type more specific than the inferred one? if ($type_coerced) { if (IssueBuffer::accepts( @@ -1363,7 +1342,8 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo new InvalidReturnType( 'The declared return type \'' . $declared_return_type . '\' for ' . $cased_method_id . ' is incorrect, got \'' . $inferred_return_type . '\'', - $return_type_location + $return_type_location, + (string) $inferred_return_type ), $this->suppressed_issues )) { @@ -1371,14 +1351,6 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo } } } elseif (!$inferred_return_type->isNullable() && $declared_return_type->isNullable()) { - if ($project_checker->update_docblocks) { - if (!in_array('InvalidReturnType', $this->suppressed_issues, true)) { - $this->addDocblockReturnType($project_checker, $inferred_return_type); - } - - return null; - } - if (IssueBuffer::accepts( new LessSpecificReturnType( 'The inferred return type \'' . $inferred_return_type . '\' for ' . $cased_method_id . diff --git a/src/Psalm/Checker/ProjectChecker.php b/src/Psalm/Checker/ProjectChecker.php index 2472b7423..720b41dc6 100644 --- a/src/Psalm/Checker/ProjectChecker.php +++ b/src/Psalm/Checker/ProjectChecker.php @@ -87,7 +87,7 @@ class ProjectChecker /** * @var bool */ - public $update_docblocks = false; + public $add_docblocks = false; /** * @var bool @@ -255,9 +255,9 @@ class ProjectChecker private $replace_code = false; /** - * @var bool + * @var array */ - private $fix_code = false; + private $issues_to_fix = []; const TYPE_CONSOLE = 'console'; const TYPE_JSON = 'json'; @@ -272,7 +272,6 @@ class ProjectChecker * @param string $output_format * @param int $threads * @param bool $debug_output - * @param bool $update_docblocks * @param bool $collect_references * @param string $find_references_to * @param string $reports @@ -285,7 +284,6 @@ class ProjectChecker $output_format = self::TYPE_CONSOLE, $threads = 1, $debug_output = false, - $update_docblocks = false, $collect_references = false, $find_references_to = null, $reports = null @@ -296,7 +294,6 @@ class ProjectChecker $this->show_info = $show_info; $this->debug_output = $debug_output; $this->threads = $threads; - $this->update_docblocks = $update_docblocks; $this->collect_references = $collect_references; $this->find_references_to = $find_references_to; @@ -335,7 +332,7 @@ class ProjectChecker } /** - * @return self + * @return ProjectChecker */ public static function getInstance() { @@ -951,7 +948,7 @@ class ProjectChecker echo 'Analyzing ' . $file_checker->getFilePath() . PHP_EOL; } - $file_checker->analyze(null, $this->update_docblocks); + $file_checker->analyze(null); }; $pool_size = $this->threads; @@ -1007,15 +1004,7 @@ class ProjectChecker } } - if ($this->replace_code) { - foreach ($this->files_to_report as $file_path) { - $this->updateFile($file_path, true); - } - } elseif ($this->update_docblocks) { - foreach ($this->files_to_report as $file_path) { - $this->updateFile($file_path, true); - } - } elseif ($this->fix_code) { + if ($this->replace_code || $this->add_docblocks || $this->issues_to_fix) { foreach ($this->files_to_report as $file_path) { $this->updateFile($file_path, true); } @@ -1030,7 +1019,11 @@ class ProjectChecker */ public function updateFile($file_path, $output_changes = false) { - $new_return_type_manipulations = FunctionDocblockManipulator::getManipulationsForFile($file_path); + if ($this->add_docblocks) { + $new_return_type_manipulations = FunctionDocblockManipulator::getManipulationsForFile($file_path); + } else { + $new_return_type_manipulations = []; + } $other_manipulations = FileManipulationBuffer::getForFile($file_path); @@ -2088,6 +2081,14 @@ class ProjectChecker } } + /** + * @return void + */ + public function addDocblocksAfterCompletion() + { + $this->add_docblocks = true; + } + /** * @return void */ @@ -2097,10 +2098,20 @@ class ProjectChecker } /** + * @param array $issues + * * @return void */ - public function fixCodeAfterCompletion() + public function fixIssuesAfterCompletion(array $issues) { - $this->fix_code = true; + $this->issues_to_fix = $issues; + } + + /** + * @return array + */ + public function getIssuesToFix() + { + return $this->issues_to_fix; } } diff --git a/src/Psalm/Checker/StatementsChecker.php b/src/Psalm/Checker/StatementsChecker.php index 632d0b9ea..a1dde0c7e 100644 --- a/src/Psalm/Checker/StatementsChecker.php +++ b/src/Psalm/Checker/StatementsChecker.php @@ -1023,7 +1023,7 @@ class StatementsChecker extends SourceChecker implements StatementsSource if ($storage->return_type && !$storage->return_type->isMixed() - && !$project_checker->update_docblocks + && !$project_checker->add_docblocks ) { $inferred_type = ExpressionChecker::fleshOutType( $project_checker, diff --git a/src/Psalm/FileManipulation/FileManipulationBuffer.php b/src/Psalm/FileManipulation/FileManipulationBuffer.php index b507d3356..4c3ba5c63 100644 --- a/src/Psalm/FileManipulation/FileManipulationBuffer.php +++ b/src/Psalm/FileManipulation/FileManipulationBuffer.php @@ -38,4 +38,12 @@ class FileManipulationBuffer return $file_manipulations; } + + /** + * @return void + */ + public static function clearCache() + { + self::$file_manipulations = []; + } } diff --git a/src/Psalm/Issue/CodeIssue.php b/src/Psalm/Issue/CodeIssue.php index 37b1d1598..60f3e6b30 100644 --- a/src/Psalm/Issue/CodeIssue.php +++ b/src/Psalm/Issue/CodeIssue.php @@ -22,8 +22,10 @@ abstract class CodeIssue * @param string $message * @param CodeLocation $code_location */ - public function __construct($message, CodeLocation $code_location) - { + public function __construct( + $message, + CodeLocation $code_location + ) { $this->code_location = $code_location; $this->message = $message; } diff --git a/src/Psalm/Issue/FalsableInferredReturnType.php b/src/Psalm/Issue/FalsableInferredReturnType.php index fb7db9c7a..f8fa09a21 100644 --- a/src/Psalm/Issue/FalsableInferredReturnType.php +++ b/src/Psalm/Issue/FalsableInferredReturnType.php @@ -1,6 +1,6 @@ replacement_text = $replacement_text; + } + + /** + * @return ?string + */ + public function getReplacementText() + { + return $this->replacement_text; + } +} diff --git a/src/Psalm/Issue/InvalidReturnType.php b/src/Psalm/Issue/InvalidReturnType.php index c8aa4b556..8358c1b43 100644 --- a/src/Psalm/Issue/InvalidReturnType.php +++ b/src/Psalm/Issue/InvalidReturnType.php @@ -1,6 +1,6 @@ getIssuesToFix(); + + if (isset($issues_to_fix[$issue_type]) + && $e instanceof \Psalm\Issue\FixableCodeIssue + && $replacement_text = $e->getReplacementText() + ) { + $code_location = $e->getLocation(); + $bounds = $code_location->getSelectionBounds(); + FileManipulationBuffer::add( + $e->getFilePath(), + [new FileManipulation($bounds[0], $bounds[1], $replacement_text)] + ); + + return false; + } + $error_message = $issue_type . ' - ' . $e->getShortLocation() . ' - ' . $e->getMessage(); $reporting_level = $config->getReportingLevelForFile($issue_type, $e->getFilePath()); diff --git a/src/psalm.php b/src/psalm.php index f742e9c81..3960e1076 100644 --- a/src/psalm.php +++ b/src/psalm.php @@ -367,7 +367,7 @@ $find_references_to = isset($options['find-references-to']) && is_string($option ? $options['find-references-to'] : null; -$update_docblocks = isset($options['update-docblocks']); +$add_docblocks = isset($options['add-docblocks']); $threads = isset($options['threads']) ? (int)$options['threads'] : 1; @@ -383,7 +383,6 @@ $project_checker = new ProjectChecker( $output_format, $threads, $debug, - $update_docblocks, $find_dead_code || $find_references_to !== null, $find_references_to, isset($options['report']) && is_string($options['report']) ? $options['report'] : null @@ -429,7 +428,12 @@ if (isset($options['fix-issues'])) { $issues = explode(',', $options['fix-issues']); - $project_checker->fixIssuesAfterCompletion($issues); + $keyed_issues = []; + foreach ($issues as $issue) { + $keyed_issues[$issue] = true; + } + + $project_checker->fixIssuesAfterCompletion($keyed_issues); } /** @psalm-suppress MixedArgument */ diff --git a/tests/ConfigTest.php b/tests/ConfigTest.php index 9d6aa335f..d54cac020 100644 --- a/tests/ConfigTest.php +++ b/tests/ConfigTest.php @@ -61,7 +61,10 @@ class ConfigTest extends TestCase * @return bool */ function ($issue_name) { - return !empty($issue_name) && $issue_name !== 'CodeError' && $issue_name !== 'CodeIssue'; + return !empty($issue_name) + && $issue_name !== 'CodeError' + && $issue_name !== 'CodeIssue' + && $issue_name !== 'FixableCodeIssue'; } ); } diff --git a/tests/FileManipulationTest.php b/tests/FileManipulationTest.php index eb509faf9..eeee716f4 100644 --- a/tests/FileManipulationTest.php +++ b/tests/FileManipulationTest.php @@ -31,7 +31,7 @@ class FileManipulationTest extends TestCase $this->project_checker->setConfig(self::$config); - $this->project_checker->update_docblocks = true; + $this->project_checker->add_docblocks = true; } /** @@ -65,6 +65,8 @@ class FileManipulationTest extends TestCase ); $file_checker = new FileChecker($file_path, $this->project_checker); + $this->project_checker->addDocblocksAfterCompletion(); + $this->project_checker->fixIssuesAfterCompletion(['InvalidReturnType' => true]); $file_checker->visitAndAnalyzeMethods($context); $this->project_checker->updateFile($file_path); $this->assertSame($output_code, $this->project_checker->getFileContents($file_path)); diff --git a/tests/ReportOutputTest.php b/tests/ReportOutputTest.php index 701d09723..f049c5cbd 100644 --- a/tests/ReportOutputTest.php +++ b/tests/ReportOutputTest.php @@ -47,7 +47,6 @@ class ReportOutputTest extends TestCase 1, false, false, - false, null, '/tmp/report' . $extension ); @@ -70,7 +69,6 @@ class ReportOutputTest extends TestCase 1, false, false, - false, null, '/tmp/report.log' );