diff --git a/config.xsd b/config.xsd index 682153bf078..634b3de26a1 100644 --- a/config.xsd +++ b/config.xsd @@ -198,9 +198,9 @@ - + @@ -292,6 +292,7 @@ + @@ -338,10 +339,10 @@ - - + + @@ -352,11 +353,13 @@ + + @@ -407,8 +410,8 @@ - + @@ -466,8 +469,8 @@ - + diff --git a/docs/running_psalm/issues.md b/docs/running_psalm/issues.md index dca69c9dda2..a57d76b9e4f 100644 --- a/docs/running_psalm/issues.md +++ b/docs/running_psalm/issues.md @@ -2,6 +2,7 @@ - [AbstractInstantiation](issues/AbstractInstantiation.md) - [AbstractMethodCall](issues/AbstractMethodCall.md) + - [AmbiguousConstantInheritance](issues/AmbiguousConstantInheritance.md) - [ArgumentTypeCoercion](issues/ArgumentTypeCoercion.md) - [AssignmentToVoid](issues/AssignmentToVoid.md) - [CircularReference](issues/CircularReference.md) @@ -93,6 +94,7 @@ - [InvalidToString](issues/InvalidToString.md) - [InvalidTraversableImplementation](issues/InvalidTraversableImplementation.md) - [InvalidTypeImport](issues/InvalidTypeImport.md) + - [LessSpecificClassConstantType](issues/LessSpecificClassConstantType.md) - [LessSpecificImplementedReturnType](issues/LessSpecificImplementedReturnType.md) - [LessSpecificReturnStatement](issues/LessSpecificReturnStatement.md) - [LessSpecificReturnType](issues/LessSpecificReturnType.md) @@ -153,6 +155,7 @@ - [NullPropertyAssignment](issues/NullPropertyAssignment.md) - [NullPropertyFetch](issues/NullPropertyFetch.md) - [NullReference](issues/NullReference.md) + - [OverriddenInterfaceConstant](issues/OverriddenInterfaceConstant.md) - [OverriddenMethodAccess](issues/OverriddenMethodAccess.md) - [OverriddenPropertyAccess](issues/OverriddenPropertyAccess.md) - [ParadoxicalCondition](issues/ParadoxicalCondition.md) diff --git a/docs/running_psalm/issues/AmbiguousConstantInheritance.md b/docs/running_psalm/issues/AmbiguousConstantInheritance.md new file mode 100644 index 00000000000..52576502344 --- /dev/null +++ b/docs/running_psalm/issues/AmbiguousConstantInheritance.md @@ -0,0 +1,41 @@ +# AmbiguousConstantInheritance + +Emitted when a constant is inherited from multiple sources. + +```php + */ + public const CONSTANT = 3; + + public static function bar(): array + { + return str_split("foobar", static::CONSTANT); + } +} + +class Bar extends Foo +{ + /** @var int */ + public const CONSTANT = -1; +} + +Bar::bar(); // Error: str_split argument 2 must be greater than 0 +``` + +This issue will always show up when overriding a constant that doesn't have a docblock type. Psalm will infer the most specific type for the constant that it can, you have to add a type annotation to tell it what type constraint you wish to be applied. Otherwise Psalm has no way of telling if you mean for the constant to be a literal `1`, `int<1, max>`, `int`, `numeric`, etc. diff --git a/docs/running_psalm/issues/OverriddenInterfaceConstant.md b/docs/running_psalm/issues/OverriddenInterfaceConstant.md new file mode 100644 index 00000000000..2e36de280de --- /dev/null +++ b/docs/running_psalm/issues/OverriddenInterfaceConstant.md @@ -0,0 +1,17 @@ +# OverriddenInterfaceConstant + +Emitted when a constant declared on an interface is overridden by a child (illegal in PHP < 8.1). + +```php +analyze($member_stmts, $class_context, $global_context, true); + ClassConstAnalyzer::analyze($storage, $this->getCodebase()); + $config = Config::getInstance(); if ($class instanceof PhpParser\Node\Stmt\Class_) { diff --git a/src/Psalm/Internal/Analyzer/InterfaceAnalyzer.php b/src/Psalm/Internal/Analyzer/InterfaceAnalyzer.php index 0e9b11e4abb..53135a7ac9b 100644 --- a/src/Psalm/Internal/Analyzer/InterfaceAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/InterfaceAnalyzer.php @@ -7,12 +7,17 @@ use PhpParser; use Psalm\CodeLocation; use Psalm\Context; +use Psalm\FileManipulation; +use Psalm\Internal\Analyzer\Statements\Expression\ClassConstAnalyzer; +use Psalm\Internal\FileManipulation\FileManipulationBuffer; use Psalm\Internal\Provider\NodeDataProvider; use Psalm\Issue\ParseError; use Psalm\Issue\UndefinedInterface; use Psalm\IssueBuffer; use UnexpectedValueException; +use function strtolower; + /** * @internal */ @@ -32,6 +37,8 @@ public function analyze(): void throw new LogicException('Something went badly wrong'); } + $interface_context = new Context($this->fq_class_name); + $project_analyzer = $this->file_analyzer->project_analyzer; $codebase = $project_analyzer->getCodebase(); $config = $project_analyzer->getConfig(); @@ -106,6 +113,7 @@ public function analyze(): void ); } + $member_stmts = []; foreach ($this->class->stmts as $stmt) { if ($stmt instanceof PhpParser\Node\Stmt\ClassMethod) { $method_analyzer = new MethodAnalyzer($stmt, $this); @@ -141,7 +149,35 @@ public function analyze(): void ); return; + } elseif ($stmt instanceof PhpParser\Node\Stmt\ClassConst) { + $member_stmts[] = $stmt; + + foreach ($stmt->consts as $const) { + $const_id = strtolower($this->fq_class_name) . '::' . $const->name; + + foreach ($codebase->class_constants_to_rename as $original_const_id => $new_const_name) { + if ($const_id === $original_const_id) { + $file_manipulations = [ + new FileManipulation( + (int) $const->name->getAttribute('startFilePos'), + (int) $const->name->getAttribute('endFilePos') + 1, + $new_const_name + ) + ]; + + FileManipulationBuffer::add( + $this->getFilePath(), + $file_manipulations + ); + } + } + } } } + + $statements_analyzer = new StatementsAnalyzer($this, new NodeDataProvider()); + $statements_analyzer->analyze($member_stmts, $interface_context, null, true); + + ClassConstAnalyzer::analyze($this->storage, $this->getCodebase()); } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ClassConstFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/ClassConstAnalyzer.php similarity index 79% rename from src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ClassConstFetchAnalyzer.php rename to src/Psalm/Internal/Analyzer/Statements/Expression/ClassConstAnalyzer.php index 10ef6434885..958c144f95b 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ClassConstFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/ClassConstAnalyzer.php @@ -1,10 +1,11 @@ class and * analyse the ::class int $stmt->name separately */ - public static function analyze( + public static function analyzeFetch( StatementsAnalyzer $statements_analyzer, PhpParser\Node\Expr\ClassConstFetch $stmt, Context $context @@ -672,17 +678,19 @@ public static function analyze( return true; } - public static function analyzeClassConstAssignment( + public static function analyzeAssignment( StatementsAnalyzer $statements_analyzer, PhpParser\Node\Stmt\ClassConst $stmt, Context $context ): void { + assert($context->self !== null); + $class_storage = $statements_analyzer->getCodebase()->classlike_storage_provider->get($context->self); + foreach ($stmt->consts as $const) { ExpressionAnalyzer::analyze($statements_analyzer, $const->value, $context); - - assert($context->self !== null); - $class_storage = $statements_analyzer->getCodebase()->classlike_storage_provider->get($context->self); $const_storage = $class_storage->constants[$const->name->name]; + + // Check assigned type matches docblock type if ($assigned_type = $statements_analyzer->node_data->getType($const->value)) { if ($const_storage->type !== null && $const_storage->stmt_location !== null @@ -695,10 +703,10 @@ public static function analyzeClassConstAssignment( ) { IssueBuffer::maybeAdd( new InvalidConstantAssignmentValue( - "{$context->self}::{$const->name->name} with declared type {$const_storage->type->getId()} " - . "cannot be assigned type {$assigned_type->getId()}", + "{$class_storage->name}::{$const->name->name} with declared type " + . "{$const_storage->type->getId()} cannot be assigned type {$assigned_type->getId()}", $const_storage->stmt_location, - "{$context->self}::{$const->name->name}" + "{$class_storage->name}::{$const->name->name}" ), $const_storage->suppressed_issues, true @@ -707,4 +715,139 @@ public static function analyzeClassConstAssignment( } } } + + public static function analyze( + ClassLikeStorage $class_storage, + Codebase $codebase + ): void { + foreach ($class_storage->constants as $const_name => $const_storage) { + // Check covariance + [$parent_classlike_storage, $parent_const_storage] = self::getOverriddenConstant( + $class_storage, + $const_storage, + $const_name, + $codebase + ); + if ($parent_const_storage !== null) { + assert($parent_classlike_storage !== null); + $location = $const_storage->type_location ?? $const_storage->stmt_location; + if ($location !== null + && $const_storage->type !== null + && $parent_const_storage->type !== null + && !UnionTypeComparator::isContainedBy( + $codebase, + $const_storage->type, + $parent_const_storage->type + ) + ) { + IssueBuffer::maybeAdd( + new LessSpecificClassConstantType( + "The type '{$const_storage->type->getId()}' for {$class_storage->name}::" + . "{$const_name} is more general than the type " + . "'{$parent_const_storage->type->getId()}' inherited from " + . "{$parent_classlike_storage->name}::{$const_name}", + $location, + "{$class_storage->name}::{$const_name}" + ), + $const_storage->suppressed_issues + ); + } + } + } + } + + /** + * Get the const storage from the parent or interface that this class is overriding. + * + * @return array{ClassLikeStorage, ClassConstantStorage}|null + */ + private static function getOverriddenConstant( + ClassLikeStorage $class_storage, + ClassConstantStorage $const_storage, + string $const_name, + Codebase $codebase + ): ?array { + $parent_classlike_storage = $interface_const_storage = $parent_const_storage = null; + $interface_overrides = []; + foreach ($class_storage->class_implements ?: $class_storage->direct_interface_parents as $interface) { + $interface_storage = $codebase->classlike_storage_provider->get($interface); + $parent_const_storage = $interface_storage->constants[$const_name] ?? null; + if ($parent_const_storage !== null) { + if ($const_storage->location + && $const_storage !== $parent_const_storage + && $codebase->analysis_php_version_id < 8_01_00 + ) { + $interface_overrides[strtolower($interface)] = new OverriddenInterfaceConstant( + "{$class_storage->name}::{$const_name} cannot override constant from $interface", + $const_storage->location, + "{$class_storage->name}::{$const_name}" + ); + } + if ($interface_const_storage !== null && $const_storage->location !== null) { + assert($parent_classlike_storage !== null); + if (!isset($parent_classlike_storage->parent_interfaces[strtolower($interface)]) + && !isset($interface_storage->parent_interfaces[strtolower($parent_classlike_storage->name)]) + ) { + IssueBuffer::maybeAdd( + new AmbiguousConstantInheritance( + "Ambiguous inheritance of {$class_storage->name}::{$const_name} from $interface and " + . $parent_classlike_storage->name, + $const_storage->location, + "{$class_storage->name}::{$const_name}" + ), + $const_storage->suppressed_issues + ); + } + } + $interface_const_storage = $parent_const_storage; + $parent_classlike_storage = $interface_storage; + } + } + + foreach ($class_storage->parent_classes as $parent_class) { + $parent_class_storage = $codebase->classlike_storage_provider->get($parent_class); + $parent_const_storage = $parent_class_storage->constants[$const_name] ?? null; + if ($parent_const_storage !== null) { + if ($const_storage->location !== null && $interface_const_storage !== null) { + assert($parent_classlike_storage !== null); + if (!isset($parent_class_storage->class_implements[strtolower($parent_classlike_storage->name)])) { + IssueBuffer::maybeAdd( + new AmbiguousConstantInheritance( + "Ambiguous inheritance of {$class_storage->name}::{$const_name} from " + . "$parent_classlike_storage->name and $parent_class", + $const_storage->location, + "{$class_storage->name}::{$const_name}" + ), + $const_storage->suppressed_issues + ); + } + } + foreach ($interface_overrides as $interface_lc => $_) { + // If the parent is the one with the const that's overriding the interface const, and the parent + // doesn't implement the interface, it's just an AmbiguousConstantInheritance, not an + // OverriddenInterfaceConstant + if (!isset($parent_class_storage->class_implements[$interface_lc]) + && $parent_const_storage === $const_storage + ) { + unset($interface_overrides[$interface_lc]); + } + } + $parent_classlike_storage = $parent_class_storage; + break; + } + } + + foreach ($interface_overrides as $_ => $issue) { + IssueBuffer::maybeAdd( + $issue, + $const_storage->suppressed_issues + ); + } + + if ($parent_classlike_storage !== null) { + assert($parent_const_storage !== null); + return [$parent_classlike_storage, $parent_const_storage]; + } + return null; + } } diff --git a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php index 354a473dc4c..a1a4483b717 100644 --- a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php @@ -19,13 +19,13 @@ use Psalm\Internal\Analyzer\Statements\Expression\Call\NewAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\Call\StaticCallAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\CastAnalyzer; +use Psalm\Internal\Analyzer\Statements\Expression\ClassConstAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\CloneAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\EmptyAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\EncapsulatedStringAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\EvalAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\ExitAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\Fetch\ArrayFetchAnalyzer; -use Psalm\Internal\Analyzer\Statements\Expression\Fetch\ClassConstFetchAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\Fetch\ConstFetchAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\Fetch\InstancePropertyFetchAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\Fetch\StaticPropertyFetchAnalyzer; @@ -240,7 +240,7 @@ private static function handleExpression( } if ($stmt instanceof PhpParser\Node\Expr\ClassConstFetch) { - return ClassConstFetchAnalyzer::analyze($statements_analyzer, $stmt, $context); + return ClassConstAnalyzer::analyzeFetch($statements_analyzer, $stmt, $context); } if ($stmt instanceof PhpParser\Node\Expr\PropertyFetch) { diff --git a/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php b/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php index 10ea738065a..952d8c5ac3f 100644 --- a/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php @@ -23,7 +23,7 @@ use Psalm\Internal\Analyzer\Statements\EchoAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\Assignment\InstancePropertyAssignmentAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\AssignmentAnalyzer; -use Psalm\Internal\Analyzer\Statements\Expression\Fetch\ClassConstFetchAnalyzer; +use Psalm\Internal\Analyzer\Statements\Expression\ClassConstAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\Fetch\ConstFetchAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\Fetch\VariableFetchAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\SimpleTypeInferer; @@ -577,7 +577,7 @@ private static function analyzeStatement( } elseif ($stmt instanceof PhpParser\Node\Stmt\Property) { InstancePropertyAssignmentAnalyzer::analyzeStatement($statements_analyzer, $stmt, $context); } elseif ($stmt instanceof PhpParser\Node\Stmt\ClassConst) { - ClassConstFetchAnalyzer::analyzeClassConstAssignment($statements_analyzer, $stmt, $context); + ClassConstAnalyzer::analyzeAssignment($statements_analyzer, $stmt, $context); } elseif ($stmt instanceof PhpParser\Node\Stmt\Class_) { try { $class_analyzer = new ClassAnalyzer( diff --git a/src/Psalm/Issue/AmbiguousConstantInheritance.php b/src/Psalm/Issue/AmbiguousConstantInheritance.php new file mode 100644 index 00000000000..9da852f6b5b --- /dev/null +++ b/src/Psalm/Issue/AmbiguousConstantInheritance.php @@ -0,0 +1,9 @@ + */ public const SHORTCODE = 205; /** diff --git a/src/Psalm/Type/Atomic/TKeyedArray.php b/src/Psalm/Type/Atomic/TKeyedArray.php index 124673e65e4..e2acadf12ed 100644 --- a/src/Psalm/Type/Atomic/TKeyedArray.php +++ b/src/Psalm/Type/Atomic/TKeyedArray.php @@ -64,6 +64,7 @@ class TKeyedArray extends Atomic */ public $is_list = false; + /** @var non-empty-lowercase-string */ public const KEY = 'array'; /** @@ -104,7 +105,6 @@ function ($name, Union $type) use ($exact): string { sort($property_strings); } - /** @psalm-suppress MixedOperand */ return static::KEY . '{' . implode(', ', $property_strings) . '}' @@ -135,7 +135,6 @@ public function toNamespacedString( ); } - /** @psalm-suppress MixedOperand */ return static::KEY . '{' . implode( ', ', diff --git a/src/Psalm/Type/Atomic/TList.php b/src/Psalm/Type/Atomic/TList.php index e5389afaf30..44d6578ee41 100644 --- a/src/Psalm/Type/Atomic/TList.php +++ b/src/Psalm/Type/Atomic/TList.php @@ -26,6 +26,7 @@ class TList extends Atomic */ public $type_param; + /** @var non-empty-lowercase-string */ public const KEY = 'list'; /** @@ -38,7 +39,6 @@ public function __construct(Union $type_param) public function getId(bool $exact = true, bool $nested = false): string { - /** @psalm-suppress MixedOperand */ return static::KEY . '<' . $this->type_param->getId($exact) . '>'; } @@ -67,7 +67,6 @@ public function toNamespacedString( ); } - /** @psalm-suppress MixedOperand */ return static::KEY . '<' . $this->type_param->toNamespacedString( diff --git a/src/Psalm/Type/Atomic/TNonEmptyList.php b/src/Psalm/Type/Atomic/TNonEmptyList.php index 8b94f6e5a2a..9e6892ba855 100644 --- a/src/Psalm/Type/Atomic/TNonEmptyList.php +++ b/src/Psalm/Type/Atomic/TNonEmptyList.php @@ -17,6 +17,7 @@ class TNonEmptyList extends TList */ public $min_count; + /** @var non-empty-lowercase-string */ public const KEY = 'non-empty-list'; public function getAssertionString(): string diff --git a/tests/ConstantTest.php b/tests/ConstantTest.php index 0dcd0630670..ba15fb23a4f 100644 --- a/tests/ConstantTest.php +++ b/tests/ConstantTest.php @@ -428,6 +428,7 @@ function foo(string $s) : void { 'resolveClassConstToCurrentClass' => [ 'code' => ' [ 'code' => ' [ 'code' => ' */ protected const A = 1; public static function test(): void { @@ -972,7 +977,9 @@ class B extends A { 'referenceClassConstantWithSelf' => [ 'code' => ' */ public const KEYS = []; + /** @var array */ public const VALUES = []; } @@ -1320,6 +1327,57 @@ function foo(array $arg): void {} foo([...A::ARR]); ', ], + 'classConstCovariant' => [ + 'code' => ' [ + 'code' => ' [ + 'code' => ' 'InvalidReturnStatement', + 'ignored_issues' => [], + 'php_version' => '8.1', ], 'outOfScopeDefinedConstant' => [ 'code' => ' "InvalidConstantAssignmentValue", ], + 'classConstContravariant' => [ + 'code' => ' "LessSpecificClassConstantType", + ], + 'classConstAmbiguousInherit' => [ + 'code' => ' 'AmbiguousConstantInheritance', + ], + 'overrideClassConstFromInterface' => [ + 'code' => ' 'OverriddenInterfaceConstant', + ], + 'overrideClassConstFromInterfaceWithInterface' => [ + 'code' => ' 'OverriddenInterfaceConstant', + ], ]; } } diff --git a/tests/DocumentationTest.php b/tests/DocumentationTest.php index c24ff0462d0..aa2207bd2e5 100644 --- a/tests/DocumentationTest.php +++ b/tests/DocumentationTest.php @@ -319,6 +319,7 @@ public function providerInvalidCodeParse(): array $ignored_issues = ['UnusedVariable']; break; + case 'AmbiguousConstantInheritance': case 'InvalidEnumBackingType': case 'InvalidEnumCaseValue': case 'DuplicateEnumCase':