Skip to content

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Sep 21, 2024

A offset array fetch on a constant array can return null if the offset does not exist, see https://3v4l.org/us7UK

triggered by #3453 (comment)

closes phpstan/phpstan#10847

@staabm
Copy link
Contributor Author

staabm commented Sep 21, 2024

we now see errors popping up in cases, where the implementation did not verify before whether the offset actually exists before fetching it (at least not in the method local code):

public function specifyTypes(FunctionReflection $functionReflection, FuncCall $node, Scope $scope, TypeSpecifierContext $context): SpecifiedTypes
{
$args = $node->getArgs();
if (count($args) >= 2) {
[$hackstackArg, $needleArg] = self::STR_CONTAINING_FUNCTIONS[strtolower($functionReflection->getName())];
$haystackType = $scope->getType($args[$hackstackArg]->value);
$needleType = $scope->getType($args[$needleArg]->value);

I am not sure this was done on purpose to prevent annoying PHPStan errors?
(or its just an oversight in the implemetations of the 2 extensions and its missing a offset-assuring narrowing?)

@staabm
Copy link
Contributor Author

staabm commented Sep 21, 2024

I am not sure this was done on purpose to prevent annoying PHPStan errors?
(or its just an oversight in the implemetations of the 2 extensions and its missing a offset-assuring narrowing?)

just fixed the 2 extensions. as this change also fixes phpstan/phpstan#10847 I think its the right thing todo

@staabm staabm marked this pull request as ready for review September 21, 2024 07:08
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@staabm
Copy link
Contributor Author

staabm commented Sep 21, 2024

there is another ClassConstFetch bug for scalar constants:
https://phpstan.org/r/25b31c30-04d0-4b02-bc41-f5c8d47af2dd

fix it here or in a new PR?

@VincentLanglet
Copy link
Contributor

A offset array fetch on a constant array can return null if the offset does not exist, see 3v4l.org/us7UK

There is a warning Warning: Undefined array key "something", so it could be considered as an error by PHPStan instead of a valid case which can return NULL.

I feel like this PR doesn't go in the right direction since it basically enforce reportPossiblyNonexistentConstantArrayOffset: true on every project https://phpstan.org/config-reference#reportpossiblynonexistentconstantarrayoffset.

According to the doc with reportPossiblyNonexistentConstantArrayOffset: false, the code

public function doFoo(string $s): int
{
    $a = ['foo' => 1];
    return $a[$s];
}

is valid; but it won't with your PR, since it will report doFoo reports int|null.

@staabm
Copy link
Contributor Author

staabm commented Sep 21, 2024

I think this PR is related but different.

I am doing this logic on ClassConstFetch only, which means its about hardcoded constant arrays only. I think it won't affect your example - I will test in a few hours when in the train.

@VincentLanglet
Copy link
Contributor

I think this PR is related but different.

I am doing this logic on ClassConstFetch only, which means its about hardcoded constant arrays only. I think it won't affect your example - I will test in a few hours when in the train.

I gave an example based on your example https://3v4l.org/us7UK and the PHPStan doc.

But I can give another. https://phpstan.org/r/cf71d857-d4a7-4d98-ac07-4eabf80e1623
IMHO this snippet:

  • Should be valid with reportPossiblyNonexistentConstantArrayOffset: false
  • Should report an error on reportPossiblyNonexistentConstantArrayOffset: true

And I assume with your PR, it will report an error because it may returns int|null (therefor reportPossiblyNonexistentConstantArrayOffset will become kinda useless)

@staabm staabm closed this Sep 21, 2024
@staabm
Copy link
Contributor Author

staabm commented Sep 21, 2024

less invasive fix in #3460

thanks for your feedback

@staabm staabm deleted the class-const branch September 21, 2024 12:15
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