Support class const covariance checks #5589
Comments
|
I found these snippets: https://psalm.dev/r/e287a07e3f<?php
class A {
/**
* @var array
*
* It's a const, not a variable, but there should be some
* way to tell psalm that children must keep ARR as an array
*/
public const ARR = [];
public static function foo(): void
{
foreach (static::ARR as $_key => $_val) {
}
}
}
class B extends A {
public const ARR = "";
} |
|
blocked by #942 |
|
Not sure they should be covariant though. Constant inheritance is a thing, so it seems like they should be invariant, like properties |
|
I believe properties must be invariant because a parent method can be called on a child class and change a child property, potentially violating its type on the child (eg parent defines property as To quote the first paragraph of the issue I linked:
Consts also need to be covariant for reading, but since they can't be written they have no need for contravariance. |
|
Then what about |
|
I found these snippets: https://psalm.dev/r/63d21b3f4c<?php
class A{
const C = 'hello';
public static function run(): void{
echo strpos(static::C, 'e');
}
}
class B extends A{
const C = [];
}
B::run(); |
|
Yeah, that should show an error because it's not covariant. Child classes should be allowed to define more strict requirements, but not less string requirements for the type. Basically, if it works for return types, it should work for consts. https://psalm.dev/r/7e497434f3
|
|
I found these snippets: https://psalm.dev/r/7e497434f3<?php
abstract class A
{
/** @var non-empty-string */
public const ERROR_CONST_CONTRAVARIANT = "str";
/** @var string */
public const CONST_COVARIANT = "";
/** @var string */
public $errorPropertyCovariant;
/** @var non-empty-string */
public $errorPropertyContravariant;
/** @var string */
public $propertyInvariant;
/** @param string $arg */
abstract public function errorArgumentNotContravariant(string $arg): string;
/** @param non-empty-string $arg */
abstract public function argumentContravariant(string $arg): string;
/** @return non-empty-string */
abstract public function errorReturnTypeNotCovariant(): string;
/** @return string */
abstract public function returnTypeCovariant(): string;
}
abstract class B extends A
{
/** @var string */
public const ERROR_CONST_CONTRAVARIANT = "";
/** @var non-empty-string */
public const CONST_COVARIANT = "";
/** @var non-empty-string */
public $errorPropertyCovariant;
/** @var string */
public $errorPropertyContravariant;
/** @var string */
public $propertyInvariant;
/** @param non-empty-string $arg */
abstract public function errorArgumentNotContravariant(string $arg): string;
/** @param string $arg */
abstract public function argumentContravariant(string $arg): string;
/** @return string */
abstract public function errorReturnTypeNotCovariant(): string;
/** @return non-empty-string */
abstract public function returnTypeCovariant(): string;
} |
Add class const covariance support (fixes vimeo#5589). Add check for overriding const from interface in PHP < 8.1 (fixes vimeo#7108). Add check for ambiguous const inheritance.
|
Right, I had a slight déja-vu about this discussion :) I'm not sure I follow your argument that class constants should behave like return types. They're more related to property types, no? And those are invariants (I have a possible legit explanation why we should consider them covariant, but I need to make sure I'm not just rationalizing here) |
|
@orklah My argument is here. Since properties can be written there's a risk of passing a child to a function taking the parent as an argument, and that function could change the property to something that matches the parent type but not the child type. Constants can't be written so there's no such risk. If I'm right, the same also applies to read-only properties, they could be made covariant, but I don't care enough to do it personally. |
|
Ok, I arrived to the same conclusion, I'm convinced :) I didn't understood the last time |
Add class const covariance support (fixes vimeo#5589). Add check for overriding const from interface in PHP < 8.1 (fixes vimeo#7108). Add check for ambiguous const inheritance.
Add class const covariance support (fixes vimeo#5589). Add check for overriding const from interface in PHP < 8.1 (fixes vimeo#7108). Add check for ambiguous const inheritance.
|
Looks like this is now caught? |
|
Yes, looks like GitHub stopped recognizing "fixes #..." comments recently. Maybe it's because things are being merged to master which is no longer the default branch? |
|
@orklah Since you changed literal strings to use single quotes, maybe types should be quoted with double quotes now? https://psalm.dev/r/e287a07e3f |
|
I found these snippets: https://psalm.dev/r/e287a07e3f<?php
class A {
/**
* @var array
*
* It's a const, not a variable, but there should be some
* way to tell psalm that children must keep ARR as an array
*/
public const ARR = [];
public static function foo(): void
{
foreach (static::ARR as $_key => $_val) {
}
}
}
class B extends A {
public const ARR = "";
} |
|
Yeah, that could be great |
|
I think the same probably doesn't apply generally to read-only properties. Read-only really means write-once. If it's not contravariant there still seems to be a risk that the parent could write it before the child does, and set it to something not assignable to the type defined in the child. |
Non-private class constants should be covariant, so a child can't change the type of a parent's const: https://psalm.dev/r/e287a07e3f
Same reasoning as #4184, but since consts can't be changed there's no need for contravariance, hence they should be covariant rather than invariant.
@varor@constshould also be used to specify the type (@varseems wrong but seems to be what's used other places).The text was updated successfully, but these errors were encountered: