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

Remove special handling for elseifs that breaks for else if #8273

Merged
merged 3 commits into from Jul 18, 2022
Merged

Remove special handling for elseifs that breaks for else if #8273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7d9ddc
Remove special handling for elseifs that breaks for else if
muglug Jul 17, 2022
a32e63f
Simplify context updates even more
muglug Jul 17, 2022
e4f73be
Add comment for skipped test
muglug Jul 17, 2022
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
28 changes: 3 additions & 25 deletions src/Psalm/Context.php
View file
Open in desktop
Expand Up @@ -459,7 +459,7 @@ public function update(
array $vars_to_update,
array &$updated_vars
): void {
foreach ($start_context->vars_in_scope as $var_id => $old_type) {
foreach ($start_context->vars_in_scope as $var_id => $_) {
// this is only true if there was some sort of type negation
if (in_array($var_id, $vars_to_update, true)) {
// if we're leaving, we're effectively deleting the possibility of the if types
Expand All @@ -469,32 +469,10 @@ public function update(

$existing_type = $this->vars_in_scope[$var_id] ?? null;

if (!$existing_type) {
if ($new_type) {
$this->vars_in_scope[$var_id] = clone $new_type;
$updated_vars[$var_id] = true;
}

continue;
}

$existing_type = clone $existing_type;

// if the type changed within the block of statements, process the replacement
// also never allow ourselves to remove all types from a union
if ((!$new_type || !$old_type->equals($new_type))
&& ($new_type || count($existing_type->getAtomicTypes()) > 1)
) {
$existing_type->substitute($old_type, $new_type);

if ($new_type && $new_type->from_docblock) {
$existing_type->setFromDocblock();
}

if (!$existing_type && $new_type) {
$this->vars_in_scope[$var_id] = clone $new_type;
$updated_vars[$var_id] = true;
}

$this->vars_in_scope[$var_id] = $existing_type;
}
}
}
Expand Down
44 changes: 1 addition & 43 deletions src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php
View file
Open in desktop
Expand Up @@ -23,34 +23,27 @@
use Psalm\Node\VirtualArg;
use Psalm\Type;
use Psalm\Type\Reconciler;
use Psalm\Type\Union;

use function array_combine;
use function array_diff_key;
use function array_filter;
use function array_intersect;
use function array_intersect_key;
use function array_key_exists;
use function array_keys;
use function array_merge;
use function array_reduce;
use function array_unique;
use function count;
use function in_array;
use function preg_match;
use function preg_quote;
use function spl_object_id;
use function strpos;
use function substr;

/**
* @internal
*/
class IfAnalyzer
{
/**
* @param array<string, Union> $pre_assignment_else_redefined_vars
*
* @return false|null
*/
public static function analyze(
Expand All @@ -59,8 +52,7 @@ public static function analyze(
IfScope $if_scope,
IfConditionalScope $if_conditional_scope,
Context $if_context,
Context $outer_context,
array $pre_assignment_else_redefined_vars
Context $outer_context
): ?bool {
$cond_referenced_var_ids = $if_conditional_scope->cond_referenced_var_ids;

Expand Down Expand Up @@ -153,8 +145,6 @@ public static function analyze(
$outer_context->vars_possibly_in_scope
);

$old_if_context = clone $if_context;

$codebase = $statements_analyzer->getCodebase();

$assigned_var_ids = $if_context->assigned_var_ids;
Expand Down Expand Up @@ -267,38 +257,6 @@ public static function analyze(
}
}

// update the parent context as necessary, but only if we can safely reason about type negation.
// We only update vars that changed both at the start of the if block and then again by an assignment
// in the if statement.
if ($if_scope->negated_types) {
$vars_to_update = array_intersect(
array_keys($pre_assignment_else_redefined_vars),
array_keys($if_scope->negated_types)
);

$extra_vars_to_update = [];

// if there's an object-like array in there, we also need to update the root array variable
foreach ($vars_to_update as $var_id) {
$bracked_pos = strpos($var_id, '[');
if ($bracked_pos !== false) {
$extra_vars_to_update[] = substr($var_id, 0, $bracked_pos);
}
}

if ($extra_vars_to_update) {
$vars_to_update = array_unique(array_merge($extra_vars_to_update, $vars_to_update));
}

$outer_context->update(
$old_if_context,
$if_context,
$has_leaving_statements,
$vars_to_update,
$if_scope->updated_vars
);
}

if (!$has_ending_statements) {
$vars_possibly_in_scope = array_diff_key(
$if_context->vars_possibly_in_scope,
Expand Down
3 changes: 1 addition & 2 deletions src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php
View file
Open in desktop
Expand Up @@ -265,8 +265,7 @@ public static function analyze(
$if_scope,
$if_conditional_scope,
$if_context,
$context,
$pre_assignment_else_redefined_vars
$context
) === false) {
return false;
}
Expand Down
5 changes: 4 additions & 1 deletion tests/TypeReconciliation/TypeAlgebraTest.php
View file
Open in desktop
Expand Up @@ -212,7 +212,10 @@ function foo(?string $a): void {
}
}',
],
'twoVarLogicNotNestedWithElseifCorrectlyReinforcedInIf' => [
// skipping this old test because it breaks if the elseif becomes
// an else if, and I don't believe it's a pattern that happens often
// enough to warrant special-casing
'SKIPPED-twoVarLogicNotNestedWithElseifCorrectlyReinforcedInIf' => [
'code' => '<?php
class A {}
class B extends A {}
Expand Down