Skip to content

Improved sprintf() inference #3232

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jul 14, 2024
Merged

Improved sprintf() inference #3232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9b5a686
Improved sprintf() inference
staabm Jul 13, 2024
8b93038
fix
staabm Jul 13, 2024
fd5b5c9
Update SprintfFunctionDynamicReturnTypeExtension.php
staabm Jul 13, 2024
b3cfcb3
infer constant types first
staabm Jul 13, 2024
33c415d
simplify
staabm Jul 13, 2024
93ce92a
more int range tests
staabm Jul 13, 2024
83b20de
better int range support
staabm Jul 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 17 additions & 15 deletions src/Type/Php/SprintfFunctionDynamicReturnTypeExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,19 @@ public function getTypeFromFunctionCall(
return null;
}

$formatType = $scope->getType($args[0]->value);
if (count($args) === 1) {
return $this->getConstantType($args, null, $functionReflection, $scope);
$constantType = $this->getConstantType($args, $functionReflection, $scope);
if ($constantType !== null) {
return $constantType;
}

$formatType = $scope->getType($args[0]->value);
$formatStrings = $formatType->getConstantStrings();
if (count($formatStrings) === 0) {
return null;
}

$singlePlaceholderEarlyReturn = null;
foreach ($formatType->getConstantStrings() as $constantString) {
foreach ($formatStrings as $constantString) {
// The printf format is %[argnum$][flags][width][.precision]
if (preg_match('/^%([0-9]*\$)?[0-9]*\.?[0-9]*([sbdeEfFgGhHouxX])$/', $constantString->getValue(), $matches) === 1) {
if ($matches[1] !== '') {
Expand All @@ -80,9 +81,10 @@ public function getTypeFromFunctionCall(
// if the format string is just a placeholder and specified an argument
// of stringy type, then the return value will be of the same type
$checkArgType = $scope->getType($args[$checkArg]->value);

if ($matches[2] === 's' && $checkArgType->isString()->yes()) {
$singlePlaceholderEarlyReturn = $checkArgType;
if ($matches[2] === 's'
&& ($checkArgType->isString()->yes() || $checkArgType->isInteger()->yes())
) {
$singlePlaceholderEarlyReturn = $checkArgType->toString();
} elseif ($matches[2] !== 's') {
$singlePlaceholderEarlyReturn = new IntersectionType([
new StringType(),
Expand Down Expand Up @@ -115,19 +117,19 @@ public function getTypeFromFunctionCall(
$returnType = new StringType();
}

return $this->getConstantType($args, $returnType, $functionReflection, $scope);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instead of building the general return type first and pass it as a default-return into getConstantType() we now invoke getConstantType() first and build the general return type only if we were not able to infer a constant type in the first place

return $returnType;
}

/**
* @param Arg[] $args
*/
private function getConstantType(array $args, ?Type $fallbackReturnType, FunctionReflection $functionReflection, Scope $scope): ?Type
private function getConstantType(array $args, FunctionReflection $functionReflection, Scope $scope): ?Type
{
$values = [];
$combinationsCount = 1;
foreach ($args as $arg) {
if ($arg->unpack) {
return $fallbackReturnType;
return null;
}

$argType = $scope->getType($arg->value);
Expand All @@ -142,23 +144,23 @@ private function getConstantType(array $args, ?Type $fallbackReturnType, Functio
}

if (count($constantScalarValues) === 0) {
return $fallbackReturnType;
return null;
}

$values[] = $constantScalarValues;
$combinationsCount *= count($constantScalarValues);
}

if ($combinationsCount > InitializerExprTypeResolver::CALCULATE_SCALARS_LIMIT) {
return $fallbackReturnType;
return null;
}

$combinations = CombinationsHelper::combinations($values);
$returnTypes = [];
foreach ($combinations as $combination) {
$format = array_shift($combination);
if (!is_string($format)) {
return $fallbackReturnType;
return null;
}

try {
Expand All @@ -168,12 +170,12 @@ private function getConstantType(array $args, ?Type $fallbackReturnType, Functio
$returnTypes[] = $scope->getTypeFromValue(@vsprintf($format, $combination));
}
} catch (Throwable) {
return $fallbackReturnType;
return null;
}
}

if (count($returnTypes) > InitializerExprTypeResolver::CALCULATE_SCALARS_LIMIT) {
return $fallbackReturnType;
return null;
}

return TypeCombinator::union(...$returnTypes);
Expand Down
13 changes: 13 additions & 0 deletions tests/PHPStan/Analyser/nsrt/bug-11201.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ function returnsJustString(): string
return rand(0,1) === 1 ? 'foo' : '';
}

function returnsBool(): bool {
return true;
}

$s = sprintf("%s", returnsNonEmptyString());
assertType('non-empty-string', $s);

Expand All @@ -41,3 +45,12 @@ function returnsJustString(): string

$s = sprintf('%2$s', 1234, returnsNonFalsyString());
assertType('non-falsy-string', $s);

$s = sprintf('%20s', 'abc');
assertType("' abc'", $s);

$s = sprintf('%20s', true);
assertType("' 1'", $s);

$s = sprintf('%20s', returnsBool());
assertType("non-falsy-string", $s);
Comment on lines +49 to +56
Copy link
Contributor Author

Choose a reason for hiding this comment

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

regressions from #3167 which made sprintf 1:1 return constant values from arguments (bypassing sprintf formatting rules)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

//cc @pilif

16 changes: 13 additions & 3 deletions tests/PHPStan/Analyser/nsrt/bug-7387.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public function inputTypes(int $i, float $f, string $s) {

public function specifiers(int $i) {
// https://3v4l.org/fmVIg
assertType('non-falsy-string', sprintf('%14s', $i));
assertType('numeric-string', sprintf('%14s', $i));

assertType('numeric-string', sprintf('%d', $i));

Expand All @@ -45,9 +45,19 @@ public function specifiers(int $i) {

}

public function positionalArgs($mixed, int $i, float $f, string $s) {
/**
* @param positive-int $posInt
* @param negative-int $negInt
* @param int<1, 5> $nonZeroIntRange
* @param int<-1, 5> $intRange
*/
public function positionalArgs($mixed, int $i, float $f, string $s, int $posInt, int $negInt, int $nonZeroIntRange, int $intRange) {
// https://3v4l.org/vVL0c
assertType('non-falsy-string', sprintf('%2$14s', $mixed, $i));
assertType('numeric-string', sprintf('%2$14s', $mixed, $i));
assertType('non-falsy-string&numeric-string', sprintf('%2$14s', $mixed, $posInt));
assertType('non-falsy-string&numeric-string', sprintf('%2$14s', $mixed, $negInt));
Comment on lines +57 to +58
Copy link
Contributor Author

Choose a reason for hiding this comment

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

improvements because of the added toString() handling

assertType('numeric-string', sprintf('%2$14s', $mixed, $intRange));
assertType('non-falsy-string&numeric-string', sprintf('%2$14s', $mixed, $nonZeroIntRange));

assertType('numeric-string', sprintf('%2$.14F', $mixed, $i));
assertType('numeric-string', sprintf('%2$.14F', $mixed, $f));
Expand Down
Loading