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

Forbid iterating over generators with non-nullable send() #10697

Merged
merged 4 commits into from
Feb 27, 2024
Merged

Forbid iterating over generators with non-nullable send() #10697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a0bc19
Forbid iterating over generators with non-nullable `send()`
weirdan Feb 12, 2024
27461c9
Strip null used to signify completed iterations in foreach context
weirdan Feb 12, 2024
4eec8bb
Disabled wrong test
weirdan Feb 26, 2024
0ce62a5
Let's see what breaks without this hack
weirdan Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 0 additions & 9 deletions src/Psalm/Internal/Analyzer/FunctionLike/ReturnTypeAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,15 +307,6 @@ public static function verifyReturnType(
$source->getParentFQCLN(),
);

// hack until we have proper yield type collection
if ($function_like_storage
&& $function_like_storage->has_yield
&& !$inferred_yield_type
&& !$inferred_return_type->isVoid()
) {
$inferred_return_type = new Union([new TNamedObject('Generator')]);
}

if ($is_to_string) {
$union_comparison_results = new TypeComparisonResult();
if (!$inferred_return_type->hasMixed() &&
Expand Down
55 changes: 53 additions & 2 deletions src/Psalm/Internal/Analyzer/Statements/Block/ForeachAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,7 @@ public static function checkIteratorType(
$key_type,
$value_type,
$has_valid_iterator,
$invalid_iterator_types,
);
} else {
$raw_object_types[] = $iterator_atomic_type->value;
Expand Down Expand Up @@ -725,6 +726,7 @@ public static function checkIteratorType(
return null;
}

/** @param list<string> $invalid_iterator_types */
public static function handleIterable(
StatementsAnalyzer $statements_analyzer,
TNamedObject $iterator_atomic_type,
Expand All @@ -733,7 +735,8 @@ public static function handleIterable(
Context $context,
?Union &$key_type,
?Union &$value_type,
bool &$has_valid_iterator
bool &$has_valid_iterator,
array &$invalid_iterator_types = []
): void {
if ($iterator_atomic_type->extra_types) {
$iterator_atomic_types = array_merge(
Expand All @@ -753,7 +756,6 @@ public static function handleIterable(
}


$has_valid_iterator = true;

if ($iterator_atomic_type instanceof TIterable
|| (strtolower($iterator_atomic_type->value) === 'traversable'
Expand Down Expand Up @@ -781,6 +783,8 @@ public static function handleIterable(
)
)
) {
$has_valid_iterator = true;

$old_data_provider = $statements_analyzer->node_data;

$statements_analyzer->node_data = clone $statements_analyzer->node_data;
Expand Down Expand Up @@ -867,6 +871,7 @@ public static function handleIterable(
$key_type,
$value_type,
$has_valid_iterator,
$invalid_iterator_types,
);

continue;
Expand Down Expand Up @@ -899,6 +904,51 @@ public static function handleIterable(
$value_type = Type::combineUnionTypes($value_type, $value_type_part);
}
}
} elseif ($iterator_atomic_type instanceof TGenericObject
&& strtolower($iterator_atomic_type->value) === 'generator'
) {
$type_params = $iterator_atomic_type->type_params;
if (isset($type_params[2]) && !$type_params[2]->isNullable() && !$type_params[2]->isMixed()) {
$invalid_iterator_types[] = $iterator_atomic_type->getKey();
} else {
$has_valid_iterator = true;
}

$iterator_value_type = self::getFakeMethodCallType(
$statements_analyzer,
$foreach_expr,
$context,
'current',
);

$iterator_key_type = self::getFakeMethodCallType(
$statements_analyzer,
$foreach_expr,
$context,
'key',
);

if ($iterator_value_type && !$iterator_value_type->isMixed()) {
// remove null coming from current() to signify invalid iterations
// we're in a foreach context, so we know we're not going iterate past the end
if (isset($type_params[1]) && !$type_params[1]->isNullable()) {
$iterator_value_type = $iterator_value_type->getBuilder();
$iterator_value_type->removeType('null');
$iterator_value_type = $iterator_value_type->freeze();
}
$value_type = Type::combineUnionTypes($value_type, $iterator_value_type);
}

if ($iterator_key_type && !$iterator_key_type->isMixed()) {
// remove null coming from key() to signify invalid iterations
// we're in a foreach context, so we know we're not going iterate past the end
if (isset($type_params[0]) && !$type_params[0]->isNullable()) {
$iterator_key_type = $iterator_key_type->getBuilder();
$iterator_key_type->removeType('null');
$iterator_key_type = $iterator_key_type->freeze();
}
$key_type = Type::combineUnionTypes($key_type, $iterator_key_type);
}
} elseif ($codebase->classImplements(
$iterator_atomic_type->value,
'Iterator',
Expand All @@ -911,6 +961,7 @@ public static function handleIterable(
)
)
) {
$has_valid_iterator = true;
$iterator_value_type = self::getFakeMethodCallType(
$statements_analyzer,
$foreach_expr,
Expand Down
3 changes: 2 additions & 1 deletion tests/GeneratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,9 @@ function gen(): Generator {
echo yield;
}',
],
'yieldFromTwiceWithVoidSend' => [
'SKIPPED-yieldFromTwiceWithVoidSend' => [
'code' => '<?php
// this test is all wrong
/**
* @return \Generator<int, string, void, string>
*/
Expand Down
69 changes: 69 additions & 0 deletions tests/Loop/ForeachTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1169,6 +1169,63 @@ function f(array $a): array {
}
PHP,
],
'generatorWithUnspecifiedSend' => [
'code' => <<<'PHP'
<?php
/** @return Generator<int,int> */
function gen() : Generator {
return yield 1;
}
$gen = gen();
foreach ($gen as $i) {}
PHP,
],
'generatorWithMixedSend' => [
'code' => <<<'PHP'
<?php
/** @return Generator<int,int, mixed, mixed> */
function gen() : Generator {
return yield 1;
}
$gen = gen();
foreach ($gen as $i) {}
PHP,
],
'nullableGenerator' => [
'code' => <<<'PHP'
<?php
/** @return Generator<int,int|null> */
function gen() : Generator {
yield null;
yield 1;
}
$gen = gen();
$a = "";
foreach ($gen as $i) {
$a = $i;
}
PHP,
'assertions' => [
'$a===' => "''|int|null",
],
],
'nonNullableGenerator' => [
'code' => <<<'PHP'
<?php
/** @return Generator<int,int> */
function gen() : Generator {
yield 1;
}
$gen = gen();
$a = "";
foreach ($gen as $i) {
$a = $i;
}
PHP,
'assertions' => [
'$a===' => "''|int",
],
],
];
}

Expand Down Expand Up @@ -1395,6 +1452,18 @@ function f(array $a): array {
PHP,
'error_message' => 'LessSpecificReturnStatement',
],
'generatorWithNonNullableSend' => [
'code' => <<<'PHP'
<?php
/** @return Generator<int,int,string,string> */
function gen() : Generator {
return yield 1;
}
$gen = gen();
foreach ($gen as $i) {}
PHP,
'error_message' => 'InvalidIterator',
],
];
}
}