-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[TypeInfo] Add IntRangeType
to hold specific type details for int
builtin types
#59676
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
Conversation
int
and string
typeint
and string
type
ca577cc
to
26806cc
Compare
No takers? |
friendly ping @mtarld |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR @DerManoMann!
On a general note, I'm wondering if we should add such precise types in TypeInfo
.
I'm a bit afraid to "re-do" PHPStan in Symfony, which will introduce a lot of complexity. Indeed, I'm not sure that knowing that a string type is a callable string, or an integer type is positive is a really common use case.
I'm not against this addition, but I think we need to decide if the use cases really worth it. 🙂
*/ | ||
class IntRangeBoundBuiltinType extends BoundBuiltinType | ||
{ | ||
public function __construct(private array $range) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function __construct(private array $range) | |
public function __construct( | |
private int $from = \PHP_INT_MIN, | |
private int $to = \PHP_INT_MAX, | |
) { | |
parent::__construct(TypeIndentifier::INT); | |
} |
* | ||
* @extends BoundBuiltinType<BoundBuiltinTypeIdentifier::RANGE_INT> | ||
*/ | ||
class IntRangeBoundBuiltinType extends BoundBuiltinType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class IntRangeBoundBuiltinType extends BoundBuiltinType | |
final class IntRangeType extends BuiltinType |
I'd rather make the BuiltinType
non-final, and then created a IntRangeType
instead. It'll make it more logic, as an integer range is indeed a subtype of integer.
If in the future there are other subtypes, such as key-of
, value-of
, class-string
, ... (see https://phpstan.org/writing-php-code/phpdoc-types), I think it'll be clearer to create a dedicated class for each use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought about that. If we do go down that path, how would you see simple bound type enum values (for example html-escaped-string
) be attached to builtin types in general? Optionally passed in directly into the constructor? (removing the need for my BoundBuiltinType
)- something like:
// TypeFactoryTrait
/**
* @return BuiltinType<TypeIdentifier::STRING>
*/
public static function string(?BoundBuiltinTypeIdentifier $boundBuiltinType = null): BuiltinType
{
return self::builtin(TypeIdentifier::STRING, $boundBuiltinType);
}
* | ||
* @author Martin Rademacher <mano@radebatz.net> | ||
* | ||
* @extends BoundBuiltinType<BoundBuiltinTypeIdentifier::RANGE_INT> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @extends BoundBuiltinType<BoundBuiltinTypeIdentifier::RANGE_INT> | |
* @extends BuiltinType<TypeIdentifier::INT> |
'never' => Type::never(), | ||
'never-return', 'never-returns', 'no-return' => Type::never(new BoundBuiltinType(BoundBuiltinTypeIdentifier::tryFrom($node->name))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'never' => Type::never(), | |
'never-return', 'never-returns', 'no-return' => Type::never(new BoundBuiltinType(BoundBuiltinTypeIdentifier::tryFrom($node->name))), | |
'never', 'never-return', 'never-returns', 'no-return' => Type::never(), |
I don't think there is any value to be this precise, never
should be enough.
case CLASS_STRING = 'class-string'; | ||
case TRAIT_STRING = 'trait-string'; | ||
case INTERFACE_STRING = 'interface-string'; | ||
case CALLABLE_STRING = 'callable-string'; | ||
case NUMERIC_STRING = 'numeric-string'; | ||
case LOWERCASE_STRING = 'lowercase-string'; | ||
case NON_EMPTY_LOWERCASE_STRING = 'non-empty-lowercase-string'; | ||
case NON_EMPTY_STRING = 'non-empty-string'; | ||
case NON_FALSY_STRING = 'non-falsy-string'; | ||
case TRUTHY_STRING = 'truthy-string'; | ||
case LITERAL_STRING = 'literal-string'; | ||
case HTML_ESCAPED_STRING = 'html-escaped-string'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should result in a ExplicitStringType
IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean here. Are you suggesting to rename the enum to ExplicitStringType
? TBH I think the current name it too long too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think I see. It is the subclass of the builtin string. Yes, makes sense.
switch (\get_class($node->constExpr)) { | ||
case ConstExprIntegerNode::class: return (int) $node->constExpr->value; | ||
default: throw new \DomainException(\sprintf('Unhandled "%s" constant expression.', \get_class($node->constExpr))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch (\get_class($node->constExpr)) { | |
case ConstExprIntegerNode::class: return (int) $node->constExpr->value; | |
default: throw new \DomainException(\sprintf('Unhandled "%s" constant expression.', \get_class($node->constExpr))); | |
} | |
if ($node->constExpr instanceof ConstExprIntegerNode:) { | |
return (int) $node->constExpr->value; | |
} | |
throw new \DomainException(\sprintf('Unhandled "%s" constant expression.', $node->constExpr::class)); | |
} |
} | ||
}; | ||
|
||
$variableValues = array_map(fn (TypeNode $t): string|int => $getValueFromNode($t), $node->genericTypes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are string allowed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The range allows for int value and the string literals min
and max
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there is one more peculiarity with the range; non-zero-int
cannot be expressed as just a min/max as it is either a single range with exclusion, or two ranges 🤷
I suggest adding a $zeroIncluded
or similar property in addition to min
and max
.
} | ||
}; | ||
|
||
$variableValues = array_map(fn (TypeNode $t): string|int => $getValueFromNode($t), $node->genericTypes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The $getValueFromNode
can be inlined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left as-is as it feels quite big already. Is this a hard CS requirement?
{ | ||
return $this->range; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The accepts
method must be overridden as well.
Hi @mtarld . Thank you for the component. I think it would be worth adding this. IMO it does not collide (yet) with phpstan features. PHPStan allows to collect this kind of data only through Modules (executed with the cli) not in projects at runtime. One use case I have at the moment would be to extract informations about methods and its arguments to try to generate a semantic configuration dynamically. To do so we do need to extract the array shape. For example |
Another use case I can think of is for the validator component. We could think of a "auto guesser" which using this type info component would create the constraints dynamically. |
I think it is the |
Glad to hear :) It is a tricky situation. For my use case the alternative is to clone and add a few things, so it made sense to try a PR first ;) |
Deng! Will fix the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next round 😉
@@ -4,6 +4,7 @@ CHANGELOG | |||
7.3 | |||
--- | |||
|
|||
* Add `IntRangeType` and `ExplicitStringType` classes to capture more specific type detals for `int` and `string` builtin types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Add `IntRangeType` and `ExplicitStringType` classes to capture more specific type detals for `int` and `string` builtin types | |
* Add `IntRangeType` and `ExplicitStringType` classes to hold specific type details for `int` and `string` builtin types |
throw new \DomainException(\sprintf('Invalid int range expression "%s".', \get_class($node->constExpr))); | ||
}; | ||
|
||
$range = array_map(fn (TypeNode $t): int => $getValueFromNode($t), $node->genericTypes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$range = array_map(fn (TypeNode $t): int => $getValueFromNode($t), $node->genericTypes); | |
$boundaries = array_map(static fn (TypeNode $t): int => $getBoundaryFromNode($t), $node->genericTypes); |
|
||
$range = array_map(fn (TypeNode $t): int => $getValueFromNode($t), $node->genericTypes); | ||
|
||
return Type::intRange(...$range); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return Type::intRange(...$range); | |
return Type::intRange($boudaries[0], $boundaries[1]); |
|
||
public function accepts(mixed $value): bool | ||
{ | ||
return \is_int($value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return \is_int($value) | |
return parent::accepts($value) |
* | ||
* @extends BuiltinType<TypeIdentifier::INT> | ||
*/ | ||
class IntRangeType extends BuiltinType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class IntRangeType extends BuiltinType | |
final class IntRangeType extends BuiltinType |
@@ -54,6 +56,11 @@ public static function int(): BuiltinType | |||
return self::builtin(TypeIdentifier::INT); | |||
} | |||
|
|||
public static function intRange(int $from, int $to, bool $zeroIncluded = true): IntRangeType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static function intRange(int $from, int $to, bool $zeroIncluded = true): IntRangeType | |
public static function intRange(int $from = \PHP_INT_MIN, int $to = \PHP_INT_MAX): IntRangeType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but keeing $zeroIncluded
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of #59676 (comment), I think we can remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. Sorry, I missed that one.
yield [Type::intRange(1, \PHP_INT_MAX), 'positive-int']; | ||
yield [Type::intRange(\PHP_INT_MIN, -1), 'negative-int']; | ||
yield [Type::intRange(\PHP_INT_MIN, 0), 'non-positive-int']; | ||
yield [Type::intRange(0, \PHP_INT_MAX), 'non-negative-int']; | ||
yield [Type::intRange(\PHP_INT_MIN, \PHP_INT_MAX, false), 'non-zero-int']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can be moved to // int range
section
yield [Type::explicitString('class-string'), 'class-string']; | ||
yield [Type::explicitString('literal-string'), 'literal-string']; | ||
yield [Type::explicitString('html-escaped-string'), 'html-escaped-string']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have a section for these as well
public function testIsIdentifiedBy() | ||
{ | ||
$this->assertFalse((new IntRangeType(0, 5))->isIdentifiedBy(TypeIdentifier::ARRAY)); | ||
$this->assertTrue((new BuiltinType(TypeIdentifier::INT))->isIdentifiedBy(TypeIdentifier::INT)); | ||
|
||
$this->assertFalse((new IntRangeType(0, 5))->isIdentifiedBy('array')); | ||
$this->assertTrue((new IntRangeType(0, 5))->isIdentifiedBy('int')); | ||
|
||
$this->assertTrue((new IntRangeType(0, 5))->isIdentifiedBy('string', 'int')); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this test is needed (we are testing that IntRangeType
is extending BuiltinType<int>
here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough, removing
public function testIsNullable() | ||
{ | ||
$this->assertFalse((new IntRangeType(0, 5))->isNullable()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Keep it coming... |
Hmm, I wonder how this is going to work with
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the PR title and description as well?
@@ -151,7 +142,8 @@ public static function resolveDataProvider(): iterable | |||
// generic | |||
yield [Type::generic(Type::object(\DateTime::class), Type::string(), Type::bool()), \DateTime::class.'<string, bool>']; | |||
yield [Type::generic(Type::object(\DateTime::class), Type::generic(Type::object(\Stringable::class), Type::bool())), \sprintf('%s<%s<bool>>', \DateTime::class, \Stringable::class)]; | |||
yield [Type::int(), 'int<0, 100>']; | |||
yield [Type::intRange(0, 100), 'int<0, 100>']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be put under // int range
section
'float', 'double' => Type::float(), | ||
'string', | ||
'string' => Type::string(), | ||
'class-string', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll need to handle class-string<Foo>
, and interface-string<Bar>
in a specific way.
IMHO, we should split the work in 2 PRs, on for the integers stuff, and on other for the strings stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was wondering about that. Seems wrong to leave things half finished.
Happy to create a second PR. Not sure what the pattern is here - wait until this is merged, or base thge second off this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant to move all the string related stuff (ie: ExplicitString
) in another PR. By doing that, you won't have to base the second PR on the first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Getting late here...
Tried :) Let me know if you need more. |
It misses the title update as well 🙂 |
int
and string
typeIntRangeType
and ExplicitStringType
classes to hold specific type details for int
and string
builtin types
712ba1d
to
e76cbc1
Compare
IntRangeType
and ExplicitStringType
classes to hold specific type details for int
and string
builtin typesIntRangeType
to hold specific type details for int
builtin types
Some more cleanup and moved string handloing into #59833 |
@@ -9,6 +9,7 @@ CHANGELOG | |||
* Deprecate constructing a `CollectionType` instance as a list that is not an array | |||
* Deprecate the third `$asList` argument of `TypeFactoryTrait::iterable()`, use `TypeFactoryTrait::list()` instead | |||
* Add type alias support in `TypeContext` and `StringTypeResolver` | |||
* Add `IntRangeType` class to hold specific type details for `int` builtin types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Add `IntRangeType` class to hold specific type details for `int` builtin types | |
* Add `IntRangeType` class that represents a range of integer values |
yield ['nonEmptyArray', Type::array()]; | ||
yield ['nonEmptyList', Type::list()]; | ||
yield ['scalar', Type::union(Type::int(), Type::float(), Type::string(), Type::bool())]; | ||
yield ['number', Type::union(Type::int(), Type::float())]; | ||
yield ['numeric', Type::union(Type::int(), Type::float(), Type::string())]; | ||
yield ['arrayKey', Type::union(Type::int(), Type::string())]; | ||
yield ['double', Type::float()]; | ||
|
||
if (method_exists(Type::class, 'intRange')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (method_exists(Type::class, 'intRange')) { | |
// BC layer for symfony/type-info < 7.3 | |
if (method_exists(Type::class, 'intRange')) { |
if (method_exists(Type::class, 'intRange')) { | ||
yield ['positiveInt', Type::intRange(1)]; | ||
yield ['negativeInt', Type::intRange(\PHP_INT_MIN, -1)]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It misses the else
yield ['a', Type::int()]; | ||
yield ['b', Type::nullable(Type::int())]; | ||
yield ['c', Type::int()]; | ||
if (method_exists(Type::class, 'intRange')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (method_exists(Type::class, 'intRange')) { | |
// BC layer for symfony/type-info < 7.3 | |
if (method_exists(Type::class, 'intRange')) { |
return \sprintf($template, $min, $max); | ||
} | ||
|
||
if (\in_array($min, [0, 1], true) && 'max' === $max) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd have removed these conditions to only have a int<foo, bar>
canonical string representation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be more in line with what we do with non-zero-int
. Also, come to think of it, there is no telling where we started anyway...
Right, so far so good. |
Adds
IntRangeType
that extendsBuiltinType
and holds specificint
range details forint
builtin types.Includes explicit
int
types likepositive-int
ornon-negative-int
and also custom ranges likeint<3, 12>
orint<min, 5>
.