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.

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

Update ConstantArrayType::accepts() for lists #3381

Open
wants to merge 2 commits into
base: 1.12.x
Choose a base branch
from

Conversation

VincentLanglet
Copy link
Contributor

First fix of phpstan/phpstan#11600
but does not close the issue since the second part is not fixed (and is a hard one...).

@@ -39,7 +39,6 @@ public function testRule(): void
[
'Generator expects value type array{DateTime, DateTime, stdClass, DateTimeImmutable}, array{0: DateTime, 1: DateTime, 2: stdClass, 4: DateTimeImmutable} given.',
74,
'Array does not have offset 3.',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's decided by the early return about list we don't have the information anymore.

  • Not sure if it's an ok loss or we need to keep this (and how)
  • Not sure if we need an help "This must be a list"

Your call.

@@ -257,6 +255,8 @@ public function setOffsetValueType(?Type $offsetType, Type $valueType, bool $opt
return;
}
}

$this->isList = TrinaryLogic::createNo();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fix is needed or

will fail.

PHPStan was considering that

$this->tuple = [true, true, true];

		for ($i = 0; $i < 3; ++$i)
		{
			$this->tuple[$i] = false;
		}

// Here, `$this->tuple` is array{bool, bool, bool} but is not considered as a list

I added a non regression test array-is-list-offset.php which fails without this fix.

The idea is to not touch the list property if we're in the case

if ($match) {
					$this->valueTypes = $valueTypes;
					return;
				}

were all the constantScalar keys are already matching.

@VincentLanglet VincentLanglet marked this pull request as draft September 1, 2024 11:34
@VincentLanglet
Copy link
Contributor Author

This assertion is false

assertType('false', array_is_list($a));

cf https://onlinephp.io/c/28b76

@VincentLanglet VincentLanglet marked this pull request as ready for review September 1, 2024 13:24
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Please separate the changes outside of ConstantArrayType::accepts() to another PR and add proper tests there.

I'd expect new test methods added to ConstantArrayTypeBuilderTest.

src/Type/Constant/ConstantArrayType.php Outdated Show resolved Hide resolved
@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Sep 19, 2024

With all the existing fix merged, this PR is now minimal.

This commit does the fix with a test 13b6747

But as shown by https://github.com/phpstan/phpstan-src/actions/runs/10943488747/job/30383079314?pr=3381 we're getting an error

Parameter #2 $expectedErrors of method                                                                 
         PHPStan\Testing\RuleTestCase<PHPStan\Rules\Comparison\ImpossibleCheckTypeFunctionCallRule>::analyse()  
         expects list<array{0: string, 1: int, 2?: string|null}>,                                               
         list<array{0: string, 1: int, 2?: string}> given.         

because of the code

static function (array $i): array {
    if (($i[2] ?? null) === 'BUG') {
         unset($i[2]);
     }

     return $i;
},

This is because of the unset call which means the array may not be a list in the PHPStan definition.

It's easily fixed with an array_values call but

  • The error message is not helping
  • Technically the PHPdoc ask for array{0: string, 1: int, 2?: string|null} but never asked for a "PHPStan-list" in the way that the next offset might be something else than 3.

I didn't see an easy way to solve this situation unless PHPStan stop to consider that

list{string, string}

and

array{0: string, 1: string}

are the same thing when it comes from the phpdoc. list need to be explicit.

That would require to change

$builder = ConstantArrayTypeBuilder::createEmpty();

to something like

$builder = ConstantArrayTypeBuilder::createEmpty();
if ($typeNode->kind !== ArrayShapeNode::KIND_LIST) {
     $builder->degradeListCertainty();
}

which would do $this->list = $this->list->and(TrinaryLogic::createMaybe()).

I can try create another PR with such change but I'd like your opinion on this @ondrejmirtes.
It's an "important" change in the way PHPStan understand array vs list...

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

Successfully merging this pull request may close these issues.

3 participants