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

Truthy isset($arr[$k]) should narrow $k #3453

Draft
wants to merge 7 commits into
base: 1.12.x
Choose a base branch
from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Sep 19, 2024

@staabm
Copy link
Contributor Author

staabm commented Sep 19, 2024

I am puzzled by the test and its expectations:

private const MAP = [
'10' => 'Test1',
'20' => 'Test2',
];
public function validate(string $value): void
{
$value = trim($value);
if ($value === '') {
throw new \RuntimeException();
}
$value = self::MAP[$value] ?? $value;
assertType("'Test1'|'Test2'", self::MAP[$value]);
// ...

I feel the assertion in this test is wrong, see https://3v4l.org/WD7OG
after this PR we now get a *ERROR* which is not better either though ;)

Comment on lines 825 to 836
if ($varIterableKeyType->isString()->yes() && $varIterableKeyType->isNumericString()->no()) {
$types = $types->unionWith(
$this->create(
$var->dim,
$varIterableKeyType,
$context,
false,
$scope,
$rootExpr,
),
);
}
Copy link
Contributor Author

@staabm staabm Sep 19, 2024

Choose a reason for hiding this comment

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

the more iterations I do on this thing, the more I feel I need a new Type method, e.g. Type->toIssetOffset()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

memo to me: maybe take inspiration from array_key_exists:

if (!$keyType instanceof ConstantIntegerType
&& !$keyType instanceof ConstantStringType
&& !$arrayType->isIterableAtLeastOnce()->no()) {
if ($context->true()) {
$arrayKeyType = $arrayType->getIterableKeyType();
if ($keyType->isString()->yes()) {
$arrayKeyType = $arrayKeyType->toString();
} elseif ($keyType->isString()->maybe()) {
$arrayKeyType = TypeCombinator::union($arrayKeyType, $arrayKeyType->toString());
}
$specifiedTypes = $this->typeSpecifier->create(
$key,
$arrayKeyType,
$context,
$scope,
);
$arrayDimFetch = new ArrayDimFetch(
$array,
$key,
);
return $specifiedTypes->unionWith($this->typeSpecifier->create(
$arrayDimFetch,
$arrayType->getIterableValueType(),
$context,
$scope,
))->setRootExpr(new Identical($arrayDimFetch, new ConstFetch(new Name('__PHPSTAN_FAUX_CONSTANT'))));
}
return new SpecifiedTypes();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

array_key_exists also needs improvements phpstan/phpstan#11724 :)

*/
function narrowKey($mixed, string $s, int $i, array $generalArr, array $arr, array $numericKeyArray): void {
if (isset($generalArr[$mixed])) {
assertType('mixed~array|object|resource', $mixed);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly related, but I wonder if substraction should be further and always generalized if possible, mixed~type can be always generalized, ie. into string|int|float|bool|null here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it would have a functional difference. or do you have something in mind which would improve?

Copy link
Contributor

Choose a reason for hiding this comment

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

Pure readability of the type.

if (isset($arr[$mixed])) {
assertType("''|'a'|null", $mixed);
} else {
assertType('mixed', $mixed);
Copy link
Contributor

Choose a reason for hiding this comment

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

mixed~(''|'a'|null)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this comment went on the wrong line (or I have puhsed a change at the same time you did the comment) and therefore it does not make sense atm?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see it on correct line, it is for falsy context:

    $arr = [0 => 0, 1 => 1, 2 => 2];
    if (isset($arr[$mixed])) {
    } else {
        // falsy context
    }

if $arr[$k] is set (and not null because of isset), then the consts types in truthy context can be substracted in the falsy context

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 is sealed vs unsealed array territory. I would not do this until we have a clear separation of the 2 types

Copy link
Contributor

Choose a reason for hiding this comment

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

Really? If array specifies some non-optional keys, they must be present (in sealed and unsealed array) and can be substracted in the else branch, can't they?

{
$arr = ['1' => 1, '2' => 2];
if (isset($arr[$mixed])) {
assertType("1|1.0|2|2.0|'1'|'2'|true", $mixed);
Copy link
Contributor

Choose a reason for hiding this comment

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

https://3v4l.org/Rml9s - we need phpstan/phpstan#6963 or sadly float here

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.

2 participants