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

Jump to bottom

Fix a false flag issue with InvalidConstantAssignmentValue #10738

Merged
merged 1 commit into from Feb 26, 2024
Merged

Fix a false flag issue with InvalidConstantAssignmentValue #10738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 12 additions & 4 deletions src/Psalm/Internal/Analyzer/Statements/Expression/ClassConstAnalyzer.php
View file
Open in desktop
Expand Up @@ -721,19 +721,27 @@ public static function analyzeAssignment(

// Check assigned type matches docblock type
if ($assigned_type = $statements_analyzer->node_data->getType($const->value)) {
if ($const_storage->type !== null
$const_storage_type = $const_storage->type;

if ($const_storage_type !== null
&& $const_storage->stmt_location !== null
&& $assigned_type !== $const_storage->type
&& $assigned_type !== $const_storage_type
// Check if this type was defined via a dockblock or type hint otherwise the inferred type
// should always match the assigned type and we don't even need to do additional checks
// There is an issue with constants over a certain length where additional values
// are added to fallback_params in the assigned_type but not in const_storage_type
// which causes a false flag for this error to appear. Usually happens with arrays
&& ($const_storage_type->from_docblock || $const_storage_type->from_property)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this can be reduced down to just $const_storage_type->from_docblock but this will later cause issues with PHP 8.3 and constant type hinting. There needs to be a way to check if a type came from the docblock or from type hinting to keep this logic into the future.

&& !UnionTypeComparator::isContainedBy(
$statements_analyzer->getCodebase(),
$assigned_type,
$const_storage->type,
$const_storage_type,
)
) {
IssueBuffer::maybeAdd(
new InvalidConstantAssignmentValue(
"{$class_storage->name}::{$const->name->name} with declared type "
. "{$const_storage->type->getId()} cannot be assigned type {$assigned_type->getId()}",
. "{$const_storage_type->getId()} cannot be assigned type {$assigned_type->getId()}",
$const_storage->stmt_location,
"{$class_storage->name}::{$const->name->name}",
),
Expand Down
236 changes: 236 additions & 0 deletions tests/ConstantTest.php
View file
Open in desktop
Expand Up @@ -824,6 +824,242 @@ public static function foo(int $i) : void {}
A::foo(2);
A::foo(3);',
],
'tooLongArrayInvalidConstantAssignmentValueFalsePositiveWithArray' => [
'code' => '<?php
class TestInvalidConstantAssignmentValueFalsePositiveWithArray {
const LOOKUP = [
"00" => null,
"01" => null,
"02" => null,
"03" => null,
"04" => null,
"05" => null,
"06" => null,
"07" => null,
"08" => null,
"09" => null,
"10" => null,
"11" => null,
"12" => null,
"13" => null,
"14" => null,
"15" => null,
"16" => null,
"17" => null,
"18" => null,
"19" => null,
"20" => null,
"21" => null,
"22" => null,
"23" => null,
"24" => null,
"25" => null,
"26" => null,
"27" => null,
"28" => null,
"29" => null,
"30" => null,
"31" => null,
"32" => null,
"33" => null,
"34" => null,
"35" => null,
"36" => null,
"37" => null,
"38" => null,
"39" => null,
"40" => null,
"41" => null,
"42" => null,
"43" => null,
"44" => null,
"45" => null,
"46" => null,
"47" => null,
"48" => null,
"49" => null,
"50" => null,
"51" => null,
"52" => null,
"53" => null,
"54" => null,
"55" => null,
"56" => null,
"57" => null,
"58" => null,
"59" => null,
"60" => null,
"61" => null,
"62" => null,
"63" => null,
"64" => null,
"65" => null,
"66" => null,
"67" => null,
"68" => null,
"69" => null,
"70" => self::SUCCEED,
"71" => self::FAIL,
"72" => null,
"73" => null,
"74" => null,
"75" => null,
"76" => null,
"77" => null,
"78" => null,
"79" => null,
"80" => null,
"81" => null,
"82" => null,
"83" => null,
"84" => null,
"85" => null,
"86" => null,
"87" => null,
"88" => null,
"89" => null,
"90" => null,
"91" => null,
"92" => null,
"93" => null,
"94" => null,
"95" => null,
"96" => null,
"97" => null,
"98" => null,
"99" => null,
"100" => null,
"101" => null,
];

const SUCCEED = "SUCCEED";
const FAIL = "FAIL";

public static function will_succeed(string $code) : bool {
// Seems to fail when the array has over 100+ entries, and at least one value references
// another constant from the same class (even nested)
return (self::LOOKUP[$code] ?? null) === self::SUCCEED;
}
}',
],
'tooLongArrayInvalidConstantAssignmentValueFalsePositiveWithList' => [
'code' => '<?php
class TestInvalidConstantAssignmentValueFalsePositiveWithList {
const LOOKUP = [
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
self::SUCCEED,
self::FAIL,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
null,
];

const SUCCEED = "SUCCEED";
const FAIL = "FAIL";

public static function will_succeed(int $code) : bool {
// Seems to fail when the array has over 100+ entries, and at least one value references
// another constant from the same class (even nested)
return (self::LOOKUP[$code] ?? null) === self::SUCCEED;
}
}',
],
'valueOf' => [
'code' => '<?php
class A {
Expand Down