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.

Sign up for GitHub

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

Array-like-object comparison message improvement #7445

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Array-like-object comparison message improvement #7445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bbd703e
Add expansion of array keys to make managing large array-like-objects…
M1ke Jan 20, 2022
1731c93
Update to avoid crash on some atomic types without array
M1ke Jan 20, 2022
6bb1189
Refactor into method on Union
M1ke Jan 21, 2022
73442a1
Handle argument type coercion and improve message for possibly undefi…
M1ke Jan 21, 2022
8c4d6e2
Resolve phpcs and psalm errors
M1ke Jan 21, 2022
a5498f7
Add the same explanation for more/less specific return type
M1ke Jan 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/Psalm/Internal/Analyzer/FunctionLike/ReturnTypeAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,11 +529,14 @@ public static function verifyReturnType(
}
}
} else {
$extension = $declared_return_type->getExtendedComparisonDescription($inferred_return_type);
$extension = $extension ? '. '.$extension : '';

if (IssueBuffer::accepts(
new MoreSpecificReturnType(
'The declared return type \'' . $declared_return_type->getId() . '\' for '
. $cased_method_id . ' is more specific than the inferred return type '
. '\'' . $inferred_return_type->getId() . '\'',
. '\'' . $inferred_return_type->getId() . '\''.$extension,
$return_type_location
),
$suppressed_issues
Expand Down
48 changes: 28 additions & 20 deletions src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,10 +986,13 @@ public static function verifyType(
$statements_analyzer->getSuppressedIssues()
);
} elseif ($cased_method_id !== 'echo' && $cased_method_id !== 'print') {
$extension = $param_type->getExtendedComparisonDescription($input_type);
$extension = $extension ? '. '.$extension : '';

IssueBuffer::maybeAdd(
new ArgumentTypeCoercion(
'Argument ' . ($argument_offset + 1) . $method_identifier . ' expects ' . $param_type->getId() .
', parent type ' . $input_type->getId() . ' provided',
', parent type ' . $input_type->getId() . ' provided'.$extension,
$arg_location,
$cased_method_id
),
Expand Down Expand Up @@ -1031,26 +1034,31 @@ public static function verifyType(
$statements_analyzer->getSuppressedIssues()
);
}
} elseif ($types_can_be_identical) {
IssueBuffer::maybeAdd(
new PossiblyInvalidArgument(
'Argument ' . ($argument_offset + 1) . $method_identifier . ' expects ' . $param_type->getId() .
', possibly different type ' . $type . ' provided',
$arg_location,
$cased_method_id
),
$statements_analyzer->getSuppressedIssues()
);
} else {
IssueBuffer::maybeAdd(
new InvalidArgument(
'Argument ' . ($argument_offset + 1) . $method_identifier . ' expects ' . $param_type->getId() .
', ' . $type . ' provided',
$arg_location,
$cased_method_id
),
$statements_analyzer->getSuppressedIssues()
);
$extension = $param_type->getExtendedComparisonDescription($input_type);
$extension = $extension ? '. '.$extension : '';

if ($types_can_be_identical) {
IssueBuffer::maybeAdd(
new PossiblyInvalidArgument(
'Argument '.($argument_offset+1).$method_identifier.' expects '.$param_type->getId().
', possibly different type '.$type.' provided'.$extension,
$arg_location,
$cased_method_id
),
$statements_analyzer->getSuppressedIssues()
);
} else {
IssueBuffer::maybeAdd(
new InvalidArgument(
'Argument '.($argument_offset+1).$method_identifier.' expects '.$param_type->getId().
', '.$type.' provided'.$extension,
$arg_location,
$cased_method_id
),
$statements_analyzer->getSuppressedIssues()
);
}
}

return null;
Expand Down
5 changes: 4 additions & 1 deletion src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,11 +417,14 @@ public static function analyze(
}
}
} else {
$extension = $stmt_type->getExtendedComparisonDescription($local_return_type);
$extension = $extension ? '. '.$extension : '';

IssueBuffer::maybeAdd(
new LessSpecificReturnStatement(
'The type \'' . $stmt_type->getId() . '\' is more general than the'
. ' declared return type \'' . $local_return_type->getId() . '\''
. ' for ' . $cased_method_id,
. ' for ' . $cased_method_id.$extension,
new CodeLocation($source, $stmt->expr)
),
$statements_analyzer->getSuppressedIssues()
Expand Down
46 changes: 46 additions & 0 deletions src/Psalm/Type/Union.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
use Psalm\Type\Atomic\TFloat;
use Psalm\Type\Atomic\TInt;
use Psalm\Type\Atomic\TIntRange;
use Psalm\Type\Atomic\TKeyedArray;
use Psalm\Type\Atomic\TList;
use Psalm\Type\Atomic\TLiteralFloat;
use Psalm\Type\Atomic\TLiteralInt;
Expand All @@ -45,7 +46,9 @@
use Psalm\Type\Atomic\TTrue;
use UnexpectedValueException;

use function array_diff;
use function array_filter;
use function array_keys;
use function array_merge;
use function array_unique;
use function count;
Expand Down Expand Up @@ -1585,4 +1588,47 @@ public function isUnionEmpty(): bool
{
return $this->types === [];
}

public function getExtendedComparisonDescription(Union $other_type): ?string
{
$this_single = $other_single = null;
if ($this->isSingle()) {
$this_single = $this->getSingleAtomic();
}
if ($other_type->isSingle()) {
$other_single = $other_type->getSingleAtomic();
}

if (!$this_single instanceof TKeyedArray || !$other_single instanceof TKeyedArray) {
return null;
}

// There's many ways to illustrate this but this is the simplest and provides info
// without being too opinionated
$text_diff = 'The differences are in the following keys: ';
$param_keys = array_keys(self::rekeyPropertiesArray($this_single->properties));
$input_keys = array_keys(self::rekeyPropertiesArray($other_single->properties));
$key_comparison = array_diff($param_keys, $input_keys);

$text_diff .= implode(', ', $key_comparison);

return $text_diff;
}

/**
* @param non-empty-array<int|string, Union> $properties
* @return non-empty-array<int|string, Union> $properties
*/
private static function rekeyPropertiesArray(array $properties): array
{
$rekeyed = [];
foreach ($properties as $key => $property) {
if ($property->possibly_undefined) {
$key = "?$key";
}
$rekeyed[$key] = $property;
}

return $rekeyed;
}
}