Introduce Type->getConstantArrayKeys()

[staabm]

As discussed in #4161 (review)

Todo

• simplify ConstantArrayType->unsetOffset()
• simplify ArrayType->setOffsetValueType()
• simplify ArrayType->unsetOffset()
• simplify ConstantArrayTypeBuilder
• simplify TypeCombinator ?

[VincentLanglet]

At first, when looking at the method getConstantArrayKeys, I thought it would have return the array keys of the ConstantArray.
ie array{'foo': 1, 'bar': 2}::getConstantArrayKeys returns 'foo'|'bar'

So I dunno if a better naming should be found.

Also, do we need this method:

• Isn't the same than $type->toArrayKey()->getConstantScalarTypes() ?
• Or it might be better to introduce getConstantIntegers (which can be reused for some other situation like DynamicReturnType extension with constantIntegers) and to say that getConstantArrayKeys = array_merge(getConstantIntegers, getConstantStrings)

[staabm]

At first, when looking at the method getConstantArrayKeys, I thought it would have return the array keys of the ConstantArray.

I took inspiration from other existing getConstantStrings(), getConstantScalarTypes() etc.

I agree it could be misleading, but I did not thought about a better name yet.

• Isn't the same than $type->toArrayKey()->getConstantScalarTypes() ?

your example call returns all types of constant scalars (which would again need filtering at all call-sites, as a array key is only interessted in ConstantStringType|ConstantIntegerType)

we are not interessted in ConstantBoolean, ConstantFloat etc.
instead the new method will do the necessary toArrayKey type conversions which happen implictly at php runtime.

• Or it might be better to introduce getConstantIntegers (which can be reused for some other situation like DynamicReturnType extension with constantIntegers) and to say that getConstantArrayKeys = array_merge(getConstantIntegers, getConstantStrings)

the benefit of the new Type method is, that it encapsulates knowledge about which types a array-key can have into the Type-system. Calling array_merge(getConstantIntegers, getConstantStrings) or similar outside the Type classes would spread this type filtering/narrowing into all core-classes, extensions etc. (thats what we have before this PR).

my goal is to get rid of $offsetType instanceof ConstantStringType || $offsetType instanceof ConstantIntegerType logic all over the place and simplify classes which currently do toArrayKey().

[staabm]

hmm I just see my resulting types are wrong after the PR. I need to think more about the semantics

		$c = [false, false, false];
		/** @var 0|1|2 $offset */
		$offset = doFoo();
		$c[$offset] = true;
-		assertType('array{bool, bool, bool}', $c);
+		assertType('array{true, true, true}', $c);

Back to the drawing board ;-)

[VincentLanglet]

your example call returns all types of constant scalars (which would again need filtering at all call-sites, as a array key is only interessted in ConstantStringType|ConstantIntegerType)

we are not interessted in ConstantBoolean, ConstantFloat etc. instead the new method will do the necessary toArrayKey type conversions which happen implictly at php runtime.

Technically with $type->toArrayKey()->getConstantScalarTypes() you will only get ConstantString|ConstantInteger since arrayKey already does the conversion of others constants.
But I assume the issue will be the static analysis won't know it's only those constants.

the benefit of the new Type method is, that it encapsulates knowledge about which types a array-key can have into the Type-system. Calling array_merge(getConstantIntegers, getConstantStrings) or similar outside the Type classes would spread this type filtering/narrowing into all core-classes, extensions etc. (thats what we have before this PR).

my goal is to get rid of $offsetType instanceof ConstantStringType || $offsetType instanceof ConstantIntegerType logic all over the place and simplify classes which currently do toArrayKey().

Yes I understand the purpose, I just thought the getConstantIntegers would have been more useful since it would also have solve checks like

phpstan-src/src/Type/Php/NonEmptyStringFunctionsReturnTypeExtension.php

Lines 90 to 94 in 8ae740c

	$flagsType = $scope->getType($args[1]->value);
	if (!$flagsType instanceof ConstantIntegerType) {
	return false;
	}
	return (bool) ($flagsType->getValue() & ENT_SUBSTITUTE);

And calling array_merge($type->getConstantStrings(), $type->getConstantIntegers()) wasn't that much complicate than type->getConstantArrayKeys() but both method getConstantArrayKeys and getConstantIntegers can exists.

I just don't like the naming (which I find misleading) but it's definitety similar to existing methods and I don't have a better naming yet too.

I stay with your method, I think I'll try to introduce getConstantIntegers in the week. That seems to be a necessary method and nice todo for me since there is less and less easy issues to fix ^^

[VincentLanglet]

Btw @staabm this change is wrong

toArrayKey does the cast
getConstantArrayKeys only return the constants.

So NullType->toArrayKey was ConstantString('')
And NullType::getConstantArrayKeys returns []

You will need

$scope->getType($expr->dim)->toArrayKey->getConstantArrayKeys();

Which make the method less useful. (it becomes the same than ->toArrayKey->getConstantScalarValues(), unless for static analysis).

Or maybe the method you need is toConstantArrayKeys...

[ondrejmirtes]

I've always avoided adding getConstantIntegers because of IntegerRangeType. It'd consume a lot of memory, it would not be efficient.

I don't know about array keys, but I feel like for math we need methods like arithmeticAdd, arithmeticSubtract and so on.

[VincentLanglet]

I've always avoided adding getConstantIntegers because of IntegerRangeType. It'd consume a lot of memory, it would not be efficient.

How would you do for code like this then

phpstan-src/src/Type/Php/NonEmptyStringFunctionsReturnTypeExtension.php

Lines 90 to 94 in 8ae740c

	$flagsType = $scope->getType($args[1]->value);
	if (!$flagsType instanceof ConstantIntegerType) {
	return false;
	}
	return (bool) ($flagsType->getValue() & ENT_SUBSTITUTE);

?

It should be changed to

$flagsType = $scope->getType($args[1]->value);
$constants = $flagsType->getConstantIntegerType();
if (count($constants) === 0) {
     return false;
}

foreach ($constants as $int) {
        if (false === (bool) ($flagsType->getValue() & ENT_SUBSTITUTE)) {
             return false;
        }
}

return true;

[ondrejmirtes]

I'd add something like Type::containsBitwiseAnd(Type $value): TrinaryLogic.

[staabm]

@VincentLanglet for inspiration: we did similar things here

https://github.com/phpstan/phpstan-src/blob/2.1.x/src/Type/BitwiseFlagHelper.php

[ondrejmirtes]

Maybe it should be Type::bitwiseAnd(Type $value): Type instead.