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

Fix misc callable bugs #10370

Merged
merged 15 commits into from Nov 22, 2023
Merged
Show file tree
Hide file tree
Changes from all 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 dictionaries/CallMap.php
Expand Up @@ -371,7 +371,7 @@
'array_diff_ukey\'1' => ['array', 'array'=>'array', 'rest'=>'array', 'arr3'=>'array', 'arg4'=>'array|callable(mixed,mixed):int', '...rest='=>'array|callable(mixed,mixed):int'],
'array_fill' => ['array<int,mixed>', 'start_index'=>'int', 'count'=>'int', 'value'=>'mixed'],
'array_fill_keys' => ['array', 'keys'=>'array', 'value'=>'mixed'],
'array_filter' => ['array', 'array'=>'array', 'callback='=>'callable(mixed,mixed=):scalar|null', 'mode='=>'int'],
'array_filter' => ['array', 'array'=>'array', 'callback='=>'callable(mixed,array-key=):mixed|null', 'mode='=>'int'],
'array_flip' => ['array<string|int>', 'array'=>'array<string|int>'],
'array_intersect' => ['array', 'array'=>'array', '...arrays='=>'array'],
'array_intersect_assoc' => ['array', 'array'=>'array', '...arrays='=>'array'],
Expand Down
4 changes: 2 additions & 2 deletions dictionaries/CallMap_80_delta.php
Expand Up @@ -553,8 +553,8 @@
'new' => ['array', 'array'=>'array', '...arrays='=>'array'],
],
'array_filter' => [
'old' => ['array', 'array'=>'array', 'callback='=>'callable(mixed,mixed=):scalar', 'mode='=>'int'],
'new' => ['array', 'array'=>'array', 'callback='=>'callable(mixed,mixed=):scalar|null', 'mode='=>'int'],
'old' => ['array', 'array'=>'array', 'callback='=>'callable(mixed,array-key=):mixed', 'mode='=>'int'],
'new' => ['array', 'array'=>'array', 'callback='=>'callable(mixed,array-key=):mixed|null', 'mode='=>'int'],
],
'array_key_exists' => [
'old' => ['bool', 'key'=>'string|int', 'array'=>'array|object'],
Expand Down
2 changes: 1 addition & 1 deletion dictionaries/CallMap_historical.php
Expand Up @@ -9288,7 +9288,7 @@
'array_diff_ukey\'1' => ['array', 'array'=>'array', 'rest'=>'array', 'arr3'=>'array', 'arg4'=>'array|callable(mixed,mixed):int', '...rest='=>'array|callable(mixed,mixed):int'],
'array_fill' => ['array<int,mixed>', 'start_index'=>'int', 'count'=>'int', 'value'=>'mixed'],
'array_fill_keys' => ['array', 'keys'=>'array', 'value'=>'mixed'],
'array_filter' => ['array', 'array'=>'array', 'callback='=>'callable(mixed,mixed=):scalar', 'mode='=>'int'],
'array_filter' => ['array', 'array'=>'array', 'callback='=>'callable(mixed,array-key=):mixed', 'mode='=>'int'],
'array_flip' => ['array<string|int>', 'array'=>'array<string|int>'],
'array_intersect' => ['array', 'array'=>'array', '...arrays'=>'array'],
'array_intersect_assoc' => ['array', 'array'=>'array', '...arrays'=>'array'],
Expand Down
61 changes: 61 additions & 0 deletions src/Psalm/Config.php
Expand Up @@ -21,6 +21,7 @@
use Psalm\Internal\Analyzer\ClassLikeAnalyzer;
use Psalm\Internal\Analyzer\FileAnalyzer;
use Psalm\Internal\Analyzer\ProjectAnalyzer;
use Psalm\Internal\CliUtils;
use Psalm\Internal\Composer;
use Psalm\Internal\EventDispatcher;
use Psalm\Internal\IncludeCollector;
Expand Down Expand Up @@ -1298,6 +1299,66 @@ private static function fromXmlAndPaths(
$config->project_files = ProjectFileFilter::loadFromXMLElement($config_xml->projectFiles, $base_dir, true);
}

// any paths passed via CLI should be added to the projectFiles
// as they're getting analyzed like if they are part of the project
// ProjectAnalyzer::getInstance()->check_paths_files is not populated at this point in time
$paths_to_check = CliUtils::getPathsToCheck(null);
if ($paths_to_check !== null) {
$paths_to_add_to_project_files = array();
foreach ($paths_to_check as $path) {
// if we have an .xml arg here, the files passed are invalid
// valid cases (in which we don't want to add CLI passed files to projectFiles though)
// are e.g. if running phpunit tests for psalm itself
if (substr($path, -4) === '.xml') {
$paths_to_add_to_project_files = array();
break;
}

// we need an absolute path for checks
if ($path[0] !== '/' && DIRECTORY_SEPARATOR === '/') {
$prospective_path = $base_dir . DIRECTORY_SEPARATOR . $path;
} else {
$prospective_path = $path;
}

// will report an error when config is loaded anyway
if (!file_exists($prospective_path)) {
continue;
}

if ($config->isInProjectDirs($prospective_path)) {
continue;
}

$paths_to_add_to_project_files[] = $prospective_path;
}

if ($paths_to_add_to_project_files !== array() && !isset($config_xml->projectFiles)) {
if ($config_xml === null) {
$config_xml = new SimpleXMLElement('<psalm/>');
}
$config_xml->addChild('projectFiles');
}

if ($paths_to_add_to_project_files !== array() && isset($config_xml->projectFiles)) {
foreach ($paths_to_add_to_project_files as $path) {
if (is_dir($path)) {
$child = $config_xml->projectFiles->addChild('directory');
} else {
$child = $config_xml->projectFiles->addChild('file');
}

$child->addAttribute('name', $path);
}

$config->project_files = ProjectFileFilter::loadFromXMLElement(
$config_xml->projectFiles,
$base_dir,
true,
);
}
}

if (isset($config_xml->extraFiles)) {
$config->extra_files = ProjectFileFilter::loadFromXMLElement($config_xml->extraFiles, $base_dir, true);
}
Expand Down
111 changes: 83 additions & 28 deletions src/Psalm/Internal/Analyzer/MethodComparator.php
Expand Up @@ -7,6 +7,7 @@
use Psalm\CodeLocation;
use Psalm\Codebase;
use Psalm\Config;
use Psalm\Internal\Codebase\InternalCallMapHandler;
use Psalm\Internal\FileManipulation\FileManipulationBuffer;
use Psalm\Internal\MethodIdentifier;
use Psalm\Internal\PhpVisitor\ParamReplacementVisitor;
Expand Down Expand Up @@ -37,6 +38,7 @@
use Psalm\Type\Union;

use function array_filter;
use function count;
use function in_array;
use function strpos;
use function strtolower;
Expand Down Expand Up @@ -116,21 +118,21 @@ public static function compare(
);
}

// CallMapHandler needed due to https://github.com/vimeo/psalm/issues/10378
if (!$guide_classlike_storage->user_defined
&& $implementer_classlike_storage->user_defined
&& $codebase->analysis_php_version_id >= 8_01_00
&& ($guide_method_storage->return_type
&& (($guide_method_storage->return_type && InternalCallMapHandler::inCallMap($cased_guide_method_id))
|| $guide_method_storage->signature_return_type
)
&& !$implementer_method_storage->signature_return_type
) && !$implementer_method_storage->signature_return_type
&& !array_filter(
$implementer_method_storage->attributes,
static fn(AttributeStorage $s): bool => $s->fq_class_name === 'ReturnTypeWillChange',
)
) {
IssueBuffer::maybeAdd(
new MethodSignatureMustProvideReturnType(
'Method ' . $cased_implementer_method_id . ' must have a return type signature!',
'Method ' . $cased_implementer_method_id . ' must have a return type signature',
$implementer_method_storage->location ?: $code_location,
),
$suppressed_issues + $implementer_classlike_storage->suppressed_issues,
Expand Down Expand Up @@ -199,10 +201,9 @@ public static function compare(
);
}

if ($guide_classlike_storage->user_defined
&& ($guide_classlike_storage->is_interface
|| $guide_classlike_storage->preserve_constructor_signature
|| $implementer_method_storage->cased_name !== '__construct')
if (($guide_classlike_storage->is_interface
|| $guide_classlike_storage->preserve_constructor_signature
|| $implementer_method_storage->cased_name !== '__construct')
&& $implementer_method_storage->required_param_count > $guide_method_storage->required_param_count
) {
if ($implementer_method_storage->cased_name !== '__construct') {
Expand Down Expand Up @@ -361,10 +362,20 @@ private static function compareMethodParams(
CodeLocation $code_location,
array $suppressed_issues
): void {
// ignore errors from stubbed/out of project files
$config = Config::getInstance();
if (!$implementer_classlike_storage->user_defined
&& (!$implementer_param->location
|| !$config->isInProjectDirs(
$implementer_param->location->file_path,
)
)) {
return;
}

if ($prevent_method_signature_mismatch) {
if (!$guide_classlike_storage->user_defined
&& $guide_param->type
) {
&& $guide_param->type) {
$implementer_param_type = $implementer_param->signature_type;

$guide_param_signature_type = $guide_param->type;
Expand All @@ -386,8 +397,6 @@ private static function compareMethodParams(
&& !$guide_param->type->from_docblock
&& ($implementer_param_type || $guide_param_signature_type)
) {
$config = Config::getInstance();

if ($implementer_param_type
&& (!$guide_param_signature_type
|| strtolower($implementer_param_type->getId())
Expand Down Expand Up @@ -438,11 +447,8 @@ private static function compareMethodParams(
}
}

$config = Config::getInstance();

if ($guide_param->name !== $implementer_param->name
&& $guide_method_storage->allow_named_arg_calls
&& $guide_classlike_storage->user_defined
&& $implementer_classlike_storage->user_defined
&& $implementer_param->location
&& $guide_method_storage->cased_name
Expand All @@ -451,7 +457,10 @@ private static function compareMethodParams(
$implementer_param->location->file_path,
)
) {
if ($config->allow_named_arg_calls
if (!$guide_classlike_storage->user_defined && $i === 0 && count($guide_method_storage->params) < 2) {
// if it's third party defined and a single arg, renaming is unnecessary
// if we still want to psalter it, move this if and change the else below to elseif
} elseif ($config->allow_named_arg_calls
|| ($guide_classlike_storage->location
&& !$config->isInProjectDirs($guide_classlike_storage->location->file_path)
)
Expand Down Expand Up @@ -491,9 +500,7 @@ private static function compareMethodParams(
}
}

if ($guide_classlike_storage->user_defined
&& $implementer_param->signature_type
) {
if ($implementer_param->signature_type) {
self::compareMethodSignatureParams(
$codebase,
$i,
Expand Down Expand Up @@ -532,9 +539,7 @@ private static function compareMethodParams(
);
}

if ($guide_classlike_storage->user_defined && $implementer_param->by_ref !== $guide_param->by_ref) {
$config = Config::getInstance();

if ($implementer_param->by_ref !== $guide_param->by_ref) {
IssueBuffer::maybeAdd(
new MethodSignatureMismatch(
'Argument ' . ($i + 1) . ' of ' . $cased_implementer_method_id . ' is' .
Expand Down Expand Up @@ -585,6 +590,50 @@ private static function compareMethodSignatureParams(
)
: null;

// CallMapHandler needed due to https://github.com/vimeo/psalm/issues/10378
if (!$guide_param->signature_type
&& $guide_param->type
&& InternalCallMapHandler::inCallMap($cased_guide_method_id)) {
$guide_method_storage_param_type = TypeExpander::expandUnion(
$codebase,
$guide_param->type,
$guide_classlike_storage->is_trait && $guide_method_storage->abstract
? $implementer_classlike_storage->name
: $guide_classlike_storage->name,
$guide_classlike_storage->is_trait && $guide_method_storage->abstract
? $implementer_classlike_storage->name
: $guide_classlike_storage->name,
$guide_classlike_storage->is_trait && $guide_method_storage->abstract
? $implementer_classlike_storage->parent_class
: $guide_classlike_storage->parent_class,
);

$builder = $guide_method_storage_param_type->getBuilder();
foreach ($builder->getAtomicTypes() as $k => $t) {
if ($t instanceof TTemplateParam) {
$builder->removeType($k);

foreach ($t->as->getAtomicTypes() as $as_t) {
$builder->addType($as_t);
}
}
}

if ($builder->hasMixed()) {
foreach ($builder->getAtomicTypes() as $k => $_) {
if ($k !== 'mixed') {
$builder->removeType($k);
}
}
}
$guide_method_storage_param_type = $builder->freeze();
unset($builder);

if (!$guide_method_storage_param_type->hasMixed() || $codebase->analysis_php_version_id >= 8_00_00) {
$guide_param_signature_type = $guide_method_storage_param_type;
}
}

$implementer_param_signature_type = TypeExpander::expandUnion(
$codebase,
$implementer_param_signature_type,
Expand Down Expand Up @@ -896,12 +945,18 @@ private static function compareMethodSignatureReturnTypes(
: UnionTypeComparator::isContainedByInPhp($implementer_signature_return_type, $guide_signature_return_type);

if (!$is_contained_by) {
if ($codebase->analysis_php_version_id >= 8_00_00
|| $guide_classlike_storage->is_trait === $implementer_classlike_storage->is_trait
|| !in_array($guide_classlike_storage->name, $implementer_classlike_storage->used_traits)
|| $implementer_method_storage->defining_fqcln !== $implementer_classlike_storage->name
|| (!$implementer_method_storage->abstract
&& !$guide_method_storage->abstract)
if ($implementer_signature_return_type === null
&& array_filter(
$implementer_method_storage->attributes,
static fn(AttributeStorage $s): bool => $s->fq_class_name === 'ReturnTypeWillChange',
)) {
// no error if return type will change and no signature set at all
} elseif ($codebase->analysis_php_version_id >= 8_00_00
|| $guide_classlike_storage->is_trait === $implementer_classlike_storage->is_trait
|| !in_array($guide_classlike_storage->name, $implementer_classlike_storage->used_traits)
|| $implementer_method_storage->defining_fqcln !== $implementer_classlike_storage->name
|| (!$implementer_method_storage->abstract
&& !$guide_method_storage->abstract)
) {
IssueBuffer::maybeAdd(
new MethodSignatureMismatch(
Expand Down