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

AssertListItem: fix passing incorrect item type to inner validator #23

Merged
merged 4 commits into from
Aug 15, 2023
Merged

AssertListItem: fix passing incorrect item type to inner validator #23

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5d1222d
PhpDocTypeUtils: add inferGenericParameter
JanTvrdik Aug 11, 2023
b357101
AssertListItem: fix passing incorrect item type to inner validator
JanTvrdik Aug 11, 2023
4c9dbe7
AssertListItem: do not emit empty foreach
JanTvrdik Aug 11, 2023
62b13e2
PhpDocTypeUtils: restrict array key to int|string
JanTvrdik Aug 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 52 additions & 9 deletions src/Compiler/Type/PhpDocTypeUtils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,17 +251,22 @@ public static function resolve(mixed $type, ReflectionClass $context): void
}
}

public static function intersect(TypeNode ...$types): TypeNode
public static function union(TypeNode ...$types): TypeNode
{
if (count($types) === 0) {
return new IdentifierTypeNode('mixed');
}

if (count($types) === 1) {
return $types[0];
}
return match (count($types)) {
0 => new IdentifierTypeNode('never'),
1 => $types[0],
default => new UnionTypeNode($types),
};
}

return new IntersectionTypeNode($types);
public static function intersect(TypeNode ...$types): TypeNode
{
return match (count($types)) {
0 => new IdentifierTypeNode('mixed'),
1 => $types[0],
default => new IntersectionTypeNode($types),
};
}

/**
Expand Down Expand Up @@ -451,6 +456,44 @@ public static function isSubTypeOf(TypeNode $a, TypeNode $b): bool
return false;
}

public static function inferGenericParameter(TypeNode $type, string $typeName, int $parameter): TypeNode
{
$type = self::normalizeType($type);

if ($type instanceof UnionTypeNode) {
return self::union(...Arrays::map(
$type->types,
static fn(TypeNode $type) => self::inferGenericParameter($type, $typeName, $parameter),
));
}

if ($type instanceof IntersectionTypeNode) {
return self::intersect(...Arrays::map(
$type->types,
static fn(TypeNode $type) => self::inferGenericParameter($type, $typeName, $parameter),
));
}

if ($type instanceof GenericTypeNode) {
$typeDef = self::getGenericTypeDefinition($type);

if (strcasecmp($type->type->name, $typeName) === 0) {
return $type->genericTypes[$parameter] ?? $typeDef['parameters'][$parameter]['bound'] ?? new IdentifierTypeNode('mixed');
}

$superTypes = isset($typeDef['superTypes']) ? $typeDef['superTypes']($type->genericTypes) : [];

if (count($superTypes) > 0) {
return self::union(...Arrays::map(
$superTypes,
static fn(TypeNode $superType) => self::inferGenericParameter($superType, $typeName, $parameter),
));
}
}

throw new LogicException("Unable to infer generic parameter, {$type} is not subtype of {$typeName}");
}

private static function isSubTypeOfGeneric(GenericTypeNode $a, GenericTypeNode $b): bool
{
$typeDef = self::getGenericTypeDefinition($a);
Expand Down
8 changes: 7 additions & 1 deletion src/Compiler/Validator/Array/AssertListItem.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use ShipMonk\InputMapper\Compiler\Type\PhpDocTypeUtils;
use ShipMonk\InputMapper\Compiler\Validator\ValidatorCompiler;
use function array_map;
use function count;

#[Attribute(Attribute::TARGET_PARAMETER | Attribute::TARGET_PROPERTY)]
class AssertListItem implements ValidatorCompiler
Expand All @@ -36,13 +37,18 @@ public function compile(Expr $value, TypeNode $type, Expr $path, PhpCodeBuilder

foreach ($this->validators as $validator) {
$itemValue = $builder->var($itemVariableName);
$itemType = PhpDocTypeUtils::inferGenericParameter($type, 'list', 0);
$itemPath = $builder->arrayImmutableAppend($path, $builder->var($indexVariableName));

foreach ($validator->compile($itemValue, $type, $itemPath, $builder) as $statement) {
foreach ($validator->compile($itemValue, $itemType, $itemPath, $builder) as $statement) {
$foreachBody[] = $statement;
}
}

if (count($foreachBody) === 0) {
return [];
}

return [
$builder->foreach(
$value,
Expand Down
10 changes: 5 additions & 5 deletions tests/Compiler/Mapper/MapperCompilerTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,12 @@ protected function compileMapper(
$mapperDir = strtr(str_replace('ShipMonkTests\InputMapper', __DIR__ . '/../..', $mapperNamespace), '\\', '/');
$mapperPath = "{$mapperDir}/{$mapperShortClassName}.php";

if (!class_exists($mapperClassName, autoload: false)) {
$builder = new PhpCodeBuilder();
$printer = new PhpCodePrinter();
$mapperCode = $printer->prettyPrintFile($builder->mapperFile($mapperClassName, $mapperCompiler));
$builder = new PhpCodeBuilder();
$printer = new PhpCodePrinter();
$mapperCode = $printer->prettyPrintFile($builder->mapperFile($mapperClassName, $mapperCompiler));
self::assertSnapshot($mapperPath, $mapperCode);

self::assertSnapshot($mapperPath, $mapperCode);
if (!class_exists($mapperClassName, autoload: false)) {
require $mapperPath;
}

Expand Down
95 changes: 82 additions & 13 deletions tests/Compiler/Type/PhpDocTypeUtilsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,19 +431,6 @@ public function testIsSubTypeOf(string $a, string $b, bool $expected): void
self::assertSame($expected, PhpDocTypeUtils::isSubTypeOf($typeNodeA, $typeNodeB));
}

private function parseType(string $type): TypeNode
{
$lexer = new Lexer();
$constExprParser = new ConstExprParser(unescapeStrings: true);
$typeParser = new TypeParser($constExprParser);

$tokens = new TokenIterator($lexer->tokenize($type));
$typeNode = $typeParser->parse($tokens);
$tokens->consumeTokenType(Lexer::TOKEN_END);

return $typeNode;
}

/**
* @return iterable<string, array{a: string, b: string, expected: bool}>
*/
Expand Down Expand Up @@ -1036,4 +1023,86 @@ private static function provideIsSubTypeOfDataInner(): iterable
];
}

#[DataProvider('provideInferGenericParameterData')]
public function testInferGenericParameter(
string $type,
string $genericTypeName,
int $parameter,
string $expectedResult
): void
{
self::assertEquals(
$this->parseType($expectedResult),
PhpDocTypeUtils::inferGenericParameter($this->parseType($type), $genericTypeName, $parameter),
);
}

/**
* @return iterable<array{0: string, 1: string, 2: int, 3: string}>
*/
public static function provideInferGenericParameterData(): iterable
{
yield [
'list<int>',
'list',
0,
'int',
];

yield [
'list<int>|list<string>',
'list',
0,
'int|string',
];

yield [
'array<int>',
'array',
0,
'mixed',
JanTvrdik marked this conversation as resolved.
Show resolved Hide resolved
];

yield [
'array<int>',
'array',
1,
'int',
];

yield [
'list<string>',
'array',
0,
'int',
];

yield [
'list<string>',
'array',
1,
'string',
];

yield [
'ShipMonk\InputMapper\Runtime\Optional<Countable> & ShipMonk\InputMapper\Runtime\Optional<Traversable>',
'ShipMonk\InputMapper\Runtime\Optional',
0,
'Countable & Traversable',
];
Copy link
Member

Choose a reason for hiding this comment

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

Can you add testcase with something similar to this? phpstan/phpstan#9198

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. There is case for union.

        yield [
            'list<int>|list<string>',
            'list',
            0,
            'int|string',
        ];

There is no normalization for unions for now. But it you ask what is the value type of list<int>|list<string>, the answer has to be int|string which is an information loss. To avoid loosing the information, you must never pass union to inferGenericParameter and handle the union on the caller side. But that is not always pratical.

}

private function parseType(string $type): TypeNode
{
$lexer = new Lexer();
$constExprParser = new ConstExprParser(unescapeStrings: true);
$typeParser = new TypeParser($constExprParser);

$tokens = new TokenIterator($lexer->tokenize($type));
$typeNode = $typeParser->parse($tokens);
$tokens->consumeTokenType(Lexer::TOKEN_END);

return $typeNode;
}

}
26 changes: 26 additions & 0 deletions tests/Compiler/Validator/Array/AssertListItemTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@

namespace ShipMonkTests\InputMapper\Compiler\Validator\Array;

use PhpParser\Node\Expr;
use PHPStan\PhpDocParser\Ast\Type\IdentifierTypeNode;
use ShipMonk\InputMapper\Compiler\Mapper\Array\MapList;
use ShipMonk\InputMapper\Compiler\Mapper\Scalar\MapInt;
use ShipMonk\InputMapper\Compiler\Php\PhpCodeBuilder;
use ShipMonk\InputMapper\Compiler\Validator\Array\AssertListItem;
use ShipMonk\InputMapper\Compiler\Validator\Int\AssertIntMultipleOf;
use ShipMonk\InputMapper\Compiler\Validator\Int\AssertPositiveInt;
use ShipMonk\InputMapper\Compiler\Validator\ValidatorCompiler;
use ShipMonk\InputMapper\Runtime\Exception\MappingFailedException;
use ShipMonkTests\InputMapper\Compiler\Validator\ValidatorCompilerTestCase;

Expand Down Expand Up @@ -51,4 +55,26 @@ public function testListItemValidatorWithMultipleValidators(): void
);
}

public function testInnerValidatorIsCalledWithCorrectItemType(): void
{
$itemValidator = $this->createMock(ValidatorCompiler::class);

$itemValidator->expects(self::once())
->method('compile')
->with(
self::isInstanceOf(Expr::class),
self::equalTo(new IdentifierTypeNode('int')),
self::isInstanceOf(Expr::class),
self::isInstanceOf(PhpCodeBuilder::class),
);

$itemValidator->expects(self::once())
->method('getInputType')
->willReturn(new IdentifierTypeNode('int'));

$mapperCompiler = new MapList(new MapInt());
$validatorCompiler = new AssertListItem([$itemValidator]);
$this->compileValidator('ListItemValidatorCalledWithCorrectItemType', $mapperCompiler, $validatorCompiler);
}

}
47 changes: 47 additions & 0 deletions tests/Compiler/Validator/Array/Data/ListItemValidatorCalledWithCorrectItemTypeMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
<?php declare (strict_types=1);

namespace ShipMonkTests\InputMapper\Compiler\Validator\Array\Data;

use ShipMonk\InputMapper\Compiler\Mapper\Wrapper\ValidatedMapperCompiler;
use ShipMonk\InputMapper\Runtime\Exception\MappingFailedException;
use ShipMonk\InputMapper\Runtime\Mapper;
use ShipMonk\InputMapper\Runtime\MapperProvider;
use function array_is_list;
use function is_array;
use function is_int;

/**
* Generated mapper by {@see ValidatedMapperCompiler}. Do not edit directly.
*
* @implements Mapper<list<int>>
*/
class ListItemValidatorCalledWithCorrectItemTypeMapper implements Mapper
{
public function __construct(private readonly MapperProvider $provider)
{
}

/**
* @param list<string|int> $path
* @return list<int>
* @throws MappingFailedException
*/
public function map(mixed $data, array $path = []): array
{
if (!is_array($data) || !array_is_list($data)) {
throw MappingFailedException::incorrectType($data, $path, 'list');
}

$mapped = [];

foreach ($data as $index => $item) {
if (!is_int($item)) {
throw MappingFailedException::incorrectType($item, [...$path, $index], 'int');
}

$mapped[] = $item;
}

return $mapped;
}
}
Loading