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

Address flaw in type reconciliation on array references #8290

Merged
merged 3 commits into from Jul 21, 2022
Merged

Address flaw in type reconciliation on array references #8290

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
ac315fb
Demonstrate flaw in type reconciliation on array references
ohader Jul 20, 2022
6c5e5f7
Avoid cloning undefined array index
ohader Jul 20, 2022
98502d4
Assert ReconcilerTest::arrayReferencesAreHandled reaches the end
ohader Jul 20, 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
2 changes: 1 addition & 1 deletion src/Psalm/Type/Reconciler.php
View file
Open in desktop
Expand Up @@ -124,7 +124,7 @@ public static function reconcileKeyedTypes(

$cloned_referenceds = [];
foreach ($existing_references as $reference => $referenced) {
if (!isset($cloned_referenceds[$referenced])) {
if (!isset($cloned_referenceds[$referenced]) && isset($old_existing_types[$referenced])) {
Copy link
Contributor Author

@ohader ohader Jul 20, 2022 •
edited

Choose a reason for hiding this comment

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

@AndrolGenhald I'm not sure whether this is enough, or

  • whether there should be a fallback union type assigned to $existing_types[$referenced], or
  • whether the $referenced value (e.g. '$doesNotMatter[$a]') should to be split to corresponding types of $doesNotMatter + $a and traversed

Thanks in advance for any hint.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll step through it with your test tonight or tomorrow and see what's going on. Thanks for the reproduction and possible fix!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like it needs to have a type, I just ran into another error in a project having this patch applied:

Uncaught Error: Call to a member function equals() on null in vendor/vimeo/psalm/src/Psalm/Context.php:524

psalm/src/Psalm/Context.php

Line 521 in a32e63f

if (!$this_type->equals($new_type)) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, yeah, having nulls in there is not a good idea. I think I know what's going on though; when we do the check against $a it potentially changes $a's type, which means we remove everything dependent on $a from Context::$vars_in_scope (somewhere, can't remember where right now) because $doesNotMatter[$a] doesn't have the same type if $a has changed. When that happens, we also need to do something to any references that exist to $doesNotMatter[$a], probably by changing whatever code removes $doesNotMatter[$a] to use the normal process by calling Context::remove() instead of whatever it's doing now.

Of course this means the fact that it's a reference will be lost, but like I said before, combining references with arrays and objects is just asking for trouble, and there's not much we can do about it.

Edit: oh, this is actually because it's removed from $existing_types in the same method further down, before it even gets to the context. Line 339 (and possibly 724) are where the issue is happening. I think it needs to do something sort of like Context:569-588, but probably a bit different using $reference_map.

$existing_types[$referenced] = clone $old_existing_types[$referenced];
$cloned_referenceds[$referenced] = true;
}
Expand Down
28 changes: 28 additions & 0 deletions tests/TypeReconciliation/ReconcilerTest.php
View file
Open in desktop
Expand Up @@ -18,6 +18,7 @@
use Psalm\Storage\Assertion\NonEmpty;
use Psalm\Storage\Assertion\Truthy;
use Psalm\Tests\TestCase;
use Psalm\Tests\TestConfig;
use Psalm\Type;
use Psalm\Type\Atomic\TArray;
use Psalm\Type\Atomic\TClassConstant;
Expand Down Expand Up @@ -281,4 +282,31 @@ public function constantAssertions(): array
],
];
}

/**
* @test
*/
public function arrayReferencesAreHandled(): void
{
$this->addFile(
'somefile.php',
<<< 'EOT'
<?php
$a = 'a';
$b = false;
$doesNotMatter = ['a' => ['id' => 1]];
$reference = &$doesNotMatter[$a];
$result = ($a === 'not-a' && ($b || false));
EOT
);

TestConfig::getInstance()->throw_exception = false;
$codebase = $this->project_analyzer->getCodebase();
$codebase->scanner->addFileToDeepScan('somefile.php');
$codebase->addFilesToAnalyze(['somefile.php' => 'somefile.php']);
$codebase->scanFiles();
$this->file_analyzer->analyze($this->file_analyzer->context);
// just asserting this test reaches the end
self::assertTrue(true);
}
}