Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report by issues grouped by level and type #8774

Merged
merged 18 commits into from Dec 2, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Psalm/Internal/Cli/Psalm.php
Expand Up @@ -1262,7 +1262,7 @@ private static function getHelpText(): string
--output-format=console
Changes the output format.
Available formats: compact, console, text, emacs, json, pylint, xml, checkstyle, junit, sonarqube,
github, phpstorm, codeclimate
github, phpstorm, codeclimate, by-issue-level

--no-progress
Disable the progress indicator
Expand Down
5 changes: 5 additions & 0 deletions src/Psalm/IssueBuffer.php
Expand Up @@ -17,6 +17,7 @@
use Psalm\Issue\UnusedPsalmSuppress;
use Psalm\Plugin\EventHandler\Event\AfterAnalysisEvent;
use Psalm\Plugin\EventHandler\Event\BeforeAddIssueEvent;
use Psalm\Report\ByIssueLevelAndTypeReport;
use Psalm\Report\CheckstyleReport;
use Psalm\Report\CodeClimateReport;
use Psalm\Report\CompactReport;
Expand Down Expand Up @@ -854,6 +855,10 @@ public static function getOutput(
$output = new JsonReport($normalized_data, self::$fixable_issue_counts, $report_options);
break;

case Report::TYPE_BY_ISSUE_LEVEL:
$output = new ByIssueLevelAndTypeReport($normalized_data, self::$fixable_issue_counts, $report_options);
break;

case Report::TYPE_JSON_SUMMARY:
$output = new JsonSummaryReport(
$normalized_data,
Expand Down
1 change: 1 addition & 0 deletions src/Psalm/Report.php
Expand Up @@ -29,6 +29,7 @@ abstract class Report
public const TYPE_SARIF = 'sarif';
public const TYPE_CODECLIMATE = 'codeclimate';
public const TYPE_COUNT = 'count';
public const TYPE_BY_ISSUE_LEVEL = 'by-issue-level';

/**
* @var array<int, IssueData>
Expand Down
188 changes: 188 additions & 0 deletions src/Psalm/Report/ByIssueLevelAndTypeReport.php
@@ -0,0 +1,188 @@
<?php

namespace Psalm\Report;

use Psalm\Config;
use Psalm\Internal\Analyzer\DataFlowNodeData;
use Psalm\Internal\Analyzer\IssueData;
use Psalm\Report;

use function basename;
use function get_cfg_var;
use function ini_get;
use function strlen;
use function strtr;
use function substr;
use function usort;

final class ByIssueLevelAndTypeReport extends Report
{
/** @var string|null */
private $link_format;

public function create(): string
{
$this->sortIssuesByLevelAndType();

$output = <<<HEADING
|----------------------------------------------------------------------------------------|
| Issues have been sorted by level and type. Feature-specific issues and the |
| most serious issues that will always be reported are listed first, with |
| remaining issues in level order. Issues near the top are usually the most serious. |
| Reducing the errorLevel in psalm.xml will suppress output of issues further down |
| this report. |
| |
| The level at which issue is reported as an error is given in brackets - e.g. |
| `ERROR (2): MissingReturnType` indicates that MissingReturnType is only reported |
| as an error when Psalm's level is set to 4 or below. |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it level 2 or level 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's level 2, thanks for catching that. Fixed in 5423983.

| |
| Issues are shown or hidden in this report according to current settings. For |
| the most complete report set Psalm's error level to 0 or use --show-info=true |
| See https://psalm.dev/docs/running_psalm/error_levels/ |
|----------------------------------------------------------------------------------------|


HEADING;

;

foreach ($this->issues_data as $issue_data) {
$output .= $this->format($issue_data) . "\n" . "\n";
}

return $output;
}

/**
* Copied from ConsoleReport with only very minor changes. Todo consider reducing code duplication
*/
private function format(IssueData $issue_data): string
{
$issue_string = '';

$is_error = $issue_data->severity === Config::REPORT_ERROR;

if ($is_error) {
$issue_string .= ($this->use_color ? "\e[0;31mERROR\e[0m" : 'ERROR');
} else {
$issue_string .= 'INFO';
}

$issue_reference = $issue_data->link ? ' (see ' . $issue_data->link . ')' : '';

$level = $issue_data->error_level;
$issue_string .= ($level > 0 ? " ($level): " : ": ")
. $issue_data->type
. ' - ' . $this->getFileReference($issue_data)
. ' - ' . $issue_data->message . $issue_reference . "\n";


if ($issue_data->taint_trace) {
$issue_string .= $this->getTaintSnippets($issue_data->taint_trace);
} elseif ($this->show_snippet) {
$snippet = $issue_data->snippet;

if (!$this->use_color) {
$issue_string .= $snippet;
} else {
$selection_start = $issue_data->from - $issue_data->snippet_from;
$selection_length = $issue_data->to - $issue_data->from;

$issue_string .= substr($snippet, 0, $selection_start)
. ($is_error ? "\e[97;41m" : "\e[30;47m") . substr($snippet, $selection_start, $selection_length)
. "\e[0m" . substr($snippet, $selection_length + $selection_start) . "\n";
}
}

if ($issue_data->other_references) {
if ($this->show_snippet) {
$issue_string .= "\n";
}

$issue_string .= $this->getTaintSnippets($issue_data->other_references);
}

return $issue_string;
}

/**
* Copied from ConsoleReport unchanged. Todo consider reducing code duplication
* @param non-empty-list<DataFlowNodeData|array{label: string, entry_path_type: string}> $taint_trace
*/
private function getTaintSnippets(array $taint_trace): string
{
$snippets = '';

foreach ($taint_trace as $node_data) {
if ($node_data instanceof DataFlowNodeData) {
$snippets .= ' ' . $node_data->label . ' - ' . $this->getFileReference($node_data) . "\n";

if ($this->show_snippet) {
$snippet = $node_data->snippet;

if (!$this->use_color) {
$snippets .= $snippet . "\n\n";
} else {
$selection_start = $node_data->from - $node_data->snippet_from;
$selection_length = $node_data->to - $node_data->from;

$snippets .= substr($snippet, 0, $selection_start)
. "\e[30;47m" . substr($snippet, $selection_start, $selection_length)
. "\e[0m" . substr($snippet, $selection_length + $selection_start) . "\n\n";
}
}
} else {
$snippets .= ' ' . $node_data['label'] . "\n";
$snippets .= ' <no known location>' . "\n\n";
}
}

return $snippets;
}

/**
* @param IssueData|DataFlowNodeData $data
*/
private function getFileReference($data): string
{
$reference = $data->file_name . ':' . $data->line_from . ':' . $data->column_from;

if (!$this->use_color) {
return $reference;
}

$file_basename = basename($data->file_name);
$file_path = substr($data->file_name, 0, -strlen($file_basename));

$reference = $file_path
. "\033[1;31m"
. $file_basename . ':' . $data->line_from . ':' . $data->column_from
. "\033[0m"
;

if ($this->in_ci) {
return $reference;
}

if (null === $this->link_format) {
// if xdebug is not enabled, use `get_cfg_var` to get the value directly from php.ini
$this->link_format = ini_get('xdebug.file_link_format') ?: get_cfg_var('xdebug.file_link_format')
?: 'file://%f#L%l';
}

$link = strtr($this->link_format, ['%f' => $data->file_path, '%l' => $data->line_from]);
// $reference = $data->file_name . ':' . $data->line_from . ':' . $data->column_from;


return "\033]8;;" . $link . "\033\\" . $reference . "\033]8;;\033\\";
}

private function sortIssuesByLevelAndType(): void
{
usort($this->issues_data, function (IssueData $left, IssueData $right): int {

return [$left->error_level > 0, -$left->error_level, $left->type] <=>
[$right->error_level > 0, -$right->error_level, $right->type];
});
}
}
97 changes: 97 additions & 0 deletions tests/ByIssueLevelAndTypeReportTest.php
@@ -0,0 +1,97 @@
<?php

namespace Psalm\Tests;

use PHPUnit\Framework\TestCase;
use Psalm\Internal\Analyzer\IssueData;
use Psalm\Report\ByIssueLevelAndTypeReport;
use Psalm\Report\ReportOptions;

class ByIssueLevelAndTypeReportTest extends TestCase
{
public function testItGeneratesReport(): void
{
$issuesData = [
$this->issueData(2, 'SomeLevel2IssueType'),
$this->issueData(-1, 'SomeAlwaysReportedIssueType'),
$this->issueData(4, 'SomeLevel4IssueType'),
$this->issueData(7, 'SomeLevel7IssueType'),
$this->issueData(4, 'AnotherLevel4IssueType'),
$this->issueData(4, 'SomeLevel4IssueType'), // same issue type as above, will be sorted together
$this->issueData(1, 'SomeIssueType'),
$this->issueData(-2, 'SomeFeatureSpecificIssueType'),
];

$reportOptions = new ReportOptions();
$reportOptions->use_color = false;

$report = new ByIssueLevelAndTypeReport($issuesData, [], $reportOptions);

$this->assertSame(<<<EXPECTED
|----------------------------------------------------------------------------------------|
| Issues have been sorted by level and type. Feature-specific issues and the |
| most serious issues that will always be reported are listed first, with |
| remaining issues in level order. Issues near the top are usually the most serious. |
| Reducing the errorLevel in psalm.xml will suppress output of issues further down |
| this report. |
| |
| The level at which issue is reported as an error is given in brackets - e.g. |
| `ERROR (2): MissingReturnType` indicates that MissingReturnType is only reported |
| as an error when Psalm's level is set to 4 or below. |
| |
| Issues are shown or hidden in this report according to current settings. For |
| the most complete report set Psalm's error level to 0 or use --show-info=true |
| See https://psalm.dev/docs/running_psalm/error_levels/ |
|----------------------------------------------------------------------------------------|

ERROR: SomeAlwaysReportedIssueType - file.php:1:1 - message


ERROR: SomeFeatureSpecificIssueType - file.php:1:1 - message


ERROR (7): SomeLevel7IssueType - file.php:1:1 - message


ERROR (4): AnotherLevel4IssueType - file.php:1:1 - message


ERROR (4): SomeLevel4IssueType - file.php:1:1 - message


ERROR (4): SomeLevel4IssueType - file.php:1:1 - message


ERROR (2): SomeLevel2IssueType - file.php:1:1 - message


ERROR (1): SomeIssueType - file.php:1:1 - message
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a little sparse. Wouldn't it be more readable if we had 1 or two empty lines between levels and none between the issues of the same level?

ERROR: SomeAlwaysReportedIssueType - file.php:1:1 - message
ERROR: SomeFeatureSpecificIssueType - file.php:1:1 - message

ERROR (7): SomeLevel7IssueType - file.php:1:1 - message

ERROR (4): AnotherLevel4IssueType - file.php:1:1 - message
ERROR (4): SomeLevel4IssueType - file.php:1:1 - message
ERROR (4): SomeLevel4IssueType - file.php:1:1 - message

ERROR (2): SomeLevel2IssueType - file.php:1:1 - message

ERROR (1): SomeIssueType - file.php:1:1 - message

Copy link
Contributor Author

@bdsl bdsl Dec 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's basically the same format as the console report, it just looks sparse in the test because the code snippets are empty. I considered adding a separator between levels but I thought it's probably not worth it. Users should be a be able to do a search for whatever level they're interested in if they want to highlight it in their shell or editor.




EXPECTED, $report->create());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW now that we require PHP >7.3 you can indent the heredoc terminator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}

public function issueData(int $errorLevel, string $type): IssueData
{
return new IssueData(
'error',
1,
1,
$type,
'message',
'file.php',
'/',
'',
'',
1,
1,
1,
1,
1,
1,
0,
$errorLevel
);
}
}