Skip to content

[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

Closed
wants to merge 10 commits into from

Conversation

DerManoMann
Copy link
Contributor

@DerManoMann DerManoMann commented Feb 2, 2025

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
License MIT

Adds IntRangeType that extends BuiltinType and holds specific int range details for int builtin types.

Includes explicit int types like positive-int or non-negative-int and also custom ranges like int<3, 12> or int<min, 5>.

@carsonbot carsonbot added this to the 7.3 milestone Feb 2, 2025
@DerManoMann DerManoMann changed the title [TypeInfo] Add specific (boudn) type details to builtin int and string type [TypeInfo] Add specific (bound) type details to builtin int and string type Feb 2, 2025
@DerManoMann
Copy link
Contributor Author

No takers?

@OskarStark
Copy link
Contributor

friendly ping @mtarld

Copy link
Contributor

@mtarld mtarld left a 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @extends BoundBuiltinType<BoundBuiltinTypeIdentifier::RANGE_INT>
* @extends BuiltinType<TypeIdentifier::INT>

Comment on lines 173 to 174
'never' => Type::never(),
'never-return', 'never-returns', 'no-return' => Type::never(new BoundBuiltinType(BoundBuiltinTypeIdentifier::tryFrom($node->name))),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'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.

Comment on lines 28 to 39
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';
Copy link
Contributor

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.

Copy link
Contributor Author

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 :)

Copy link
Contributor Author

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.

Comment on lines 199 to 205
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)));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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);
Copy link
Contributor

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.

Copy link
Contributor Author

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;
}
}
Copy link
Contributor

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.

@Neirda24
Copy link
Contributor

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 list<array{plop: string}> is a prototype array with a scalar node plop which seems to be required. without this feature I would simply have a prototype array with a prototype variable

@Neirda24
Copy link
Contributor

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.

@Neirda24
Copy link
Contributor

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 FormTypeGuesser or something that based on Assert / ORM attributes it guesses what kind of attributes to apply to html inputs.

@DerManoMann
Copy link
Contributor Author

I'm not against this addition, but I think we need to decide if the use cases really worth it. 🙂

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 ;)

@DerManoMann
Copy link
Contributor Author

Deng! Will fix the PropertyInfo tests tomorrow :{

Copy link
Contributor

@mtarld mtarld left a 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return Type::intRange(...$range);
return Type::intRange($boudaries[0], $boundaries[1]);


public function accepts(mixed $value): bool
{
return \is_int($value)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return \is_int($value)
return parent::accepts($value)

*
* @extends BuiltinType<TypeIdentifier::INT>
*/
class IntRangeType extends BuiltinType
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Contributor Author

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?

Copy link
Contributor

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

Copy link
Contributor Author

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.

Comment on lines 100 to 104
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'];
Copy link
Contributor

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

Comment on lines 108 to 110
yield [Type::explicitString('class-string'), 'class-string'];
yield [Type::explicitString('literal-string'), 'literal-string'];
yield [Type::explicitString('html-escaped-string'), 'html-escaped-string'];
Copy link
Contributor

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

Comment on lines 32 to 41
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'));
}
Copy link
Contributor

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough, removing

Comment on lines 43 to 29
public function testIsNullable()
{
$this->assertFalse((new IntRangeType(0, 5))->isNullable());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@DerManoMann
Copy link
Contributor Author

Next round 😉

Keep it coming...

@DerManoMann
Copy link
Contributor Author

Hmm, I wonder how this is going to work with PropertyInfo...

PropertyInfo does require "symfony/type-info": "~7.1.9|^7.2.2" - that means I shouldn't update the tests as this PR is against 7.3.
OTOH, running all tests on 7.3 will fail then...

@mtarld
Copy link
Contributor

mtarld commented Feb 21, 2025

^7.2.2 doesn't restrict TypeInfo to 7.2, see https://getcomposer.org/doc/articles/versions.md#caret-version-range-

Copy link
Contributor

@mtarld mtarld left a 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>'];
Copy link
Contributor

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',
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Getting late here...

@DerManoMann
Copy link
Contributor Author

Can you update the PR title and description as well?

Tried :) Let me know if you need more.

@mtarld
Copy link
Contributor

mtarld commented Feb 21, 2025

It misses the title update as well 🙂

@DerManoMann DerManoMann changed the title [TypeInfo] Add specific (bound) type details to builtin int and string type [TypeInfo] Add IntRangeType and ExplicitStringType classes to hold specific type details for int and string builtin types Feb 21, 2025
@DerManoMann DerManoMann changed the title [TypeInfo] Add IntRangeType and ExplicitStringType classes to hold specific type details for int and string builtin types [TypeInfo] Add IntRangeType to hold specific type details for int builtin types Feb 22, 2025
@DerManoMann
Copy link
Contributor Author

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* 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')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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)];
}
Copy link
Contributor

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')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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) {
Copy link
Contributor

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

Copy link
Contributor Author

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...

@DerManoMann
Copy link
Contributor Author

Right, so far so good.
I am just a bit at a loss as to how to replicate the test failures locally now. (8.2, high-deps) looks like it has the branch changes, but doesn't use them somehow.
Didn't have time to look into 8.5, but that looks somewhat unrelated; might be wrong, though ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants