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

support constant string-unions in substr() #1532

Merged
merged 11 commits into from Jul 22, 2022

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jul 20, 2022


assertType("'DE'|'US'", substr($language, 3));
assertType("'_DE'|'_US'", substr($language, -3));
assertType("'_'", substr($language, -3, 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be worth adding an assertType for the false return case when asking for invalid length offsets?

also substr in php7 and php8 return different values when given invalid $length or $offsets; 7 returns false whereas 8 returns '':

https://www.php.net/manual/en/function.substr.php#refsect1-function.substr-changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed, added a few cases.

@staabm staabm force-pushed the constant-substr branch 2 times, most recently from 71f3506 to 220b57c Compare July 20, 2022 21:17
@staabm staabm marked this pull request as ready for review July 21, 2022 07:33
@staabm
Copy link
Contributor Author

staabm commented Jul 21, 2022

I think its good to go.


the composer error is pretty exotic, as it is related to some kind of build step involved (files getting copied into different directories) and therefore the related line if (substr(__DIR__, -8, 1) !== 'C') { does resolve to something different at runtime, as it does at analysis time

related composer commit which introduced this exotic logic composer/composer@e087a4a

related composer issue which required the workaround in composer: composer/composer#9937


the PrestaShop error relates to these lines

https://github.com/PrestaShop/PrestaShop/blob/6c3797271697278b31e2bd0905d54cea6131d335/classes/controller/FrontController.php#L1437-L1473

in which phpstan seems to be able to infer the constant values involved and therefore now figures out that a if-case will always be false.

seems presta shop needs to configured those constants involved to be dynamic in the phpstan.neon

Comment on lines +76 to +80
if (is_bool($substr)) {
$results[] = new ConstantBooleanType($substr);
} else {
$results[] = new ConstantStringType($substr);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use ConstantTypeHelper::getTypeFromValue() instead (even though it's deprecated)

Copy link
Contributor Author

@staabm staabm Jul 21, 2022

Choose a reason for hiding this comment

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

I searched the code-base yesterday for this method .. I knew we had such thing, but wasn't able to find it :)

as you mentioned - the method is deprecated and seems to be a rather big canon for the rather small problem I solve here - I feel the PR should stay as is in this regard.

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.

Otherwise 👍

$positiveLength = false;

if (count($args) === 3) {
$length = $scope->getType($args[2]->value);
$positiveLength = IntegerRangeType::fromInterval(1, null)->isSuperTypeOf($length)->yes();
}

$constantStrings = TypeUtils::getConstantStrings($string);
if (count($constantStrings) > 0 &&
Copy link
Member

Choose a reason for hiding this comment

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

I format these differently:

			if (
						count($constantStrings) > 0
						&& $offset instanceof ConstantIntegerType
						&& ($length === null || $length instanceof ConstantIntegerType)
			) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@ondrejmirtes ondrejmirtes merged commit a5f20c4 into phpstan:1.8.x Jul 22, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the constant-substr branch July 22, 2022 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants