From f2168de96ed06a601ff85043055a8dd0aa62908b Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Mon, 12 Jun 2023 18:25:38 +0200 Subject: [PATCH 1/7] fix printf bug --- .../Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php b/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php index 7aa6e9e24..f3c92df02 100644 --- a/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php +++ b/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php @@ -63,7 +63,7 @@ class SprintfReturnTypeProvider implements FunctionReturnTypeProviderInterface $node_type_provider = $statements_source->getNodeTypeProvider(); foreach ($call_args as $index => $call_arg) { $type = $node_type_provider->getType($call_arg->value); - if ($type === null && $event->getFunctionId() === 'printf') { + if ($type === null && $index === 0 && $event->getFunctionId() === 'printf') { break; } From 697d06b24d202322b3e0f76f33d89d064d468b65 Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Mon, 12 Jun 2023 18:30:20 +0200 Subject: [PATCH 2/7] fix empty format return type, add errors for empty format or format without placeholders --- .../SprintfReturnTypeProvider.php | 36 +++++++++++++++++++ tests/ReturnTypeProvider/SprintfTest.php | 24 +++++++++++++ 2 files changed, 60 insertions(+) diff --git a/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php b/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php index f3c92df02..d8f18a407 100644 --- a/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php +++ b/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php @@ -72,6 +72,19 @@ class SprintfReturnTypeProvider implements FunctionReturnTypeProviderInterface } if ($index === 0 && $type->isSingleStringLiteral()) { + if ($type->getSingleStringLiteral()->value === '') { + IssueBuffer::maybeAdd( + new InvalidArgument( + 'Argument 1 of ' . $event->getFunctionId() . ' must not be an empty string', + $event->getCodeLocation(), + $event->getFunctionId(), + ), + $statements_source->getSuppressedIssues(), + ); + + return null; + } + $args_count = count($call_args) - 1; $dummy = array_fill(0, $args_count, ''); @@ -84,6 +97,19 @@ class SprintfReturnTypeProvider implements FunctionReturnTypeProviderInterface $result = @sprintf($type->getSingleStringLiteral()->value, ...$dummy); if ($initial_result === null) { $initial_result = $result; + + if ($result === $type->getSingleStringLiteral()->value) { + IssueBuffer::maybeAdd( + new InvalidArgument( + 'Argument 1 of ' . $event->getFunctionId() . ' does not contain any placeholders', + $event->getCodeLocation(), + $event->getFunctionId(), + ), + $statements_source->getSuppressedIssues(), + ); + + return null; + } } } catch (ValueError $value_error) { // PHP 8 @@ -189,6 +215,16 @@ class SprintfReturnTypeProvider implements FunctionReturnTypeProviderInterface if ($initial_result !== null && $initial_result !== false && $initial_result !== '') { return Type::getNonEmptyString(); } + + /** + * if we didn't have a valid result, the pattern is invalid or not yet supported by the return type provider + * PHP 7 can have false here + * + * @psalm-suppress RedundantConditionGivenDocblockType + */ + if ($initial_result === null || $initial_result === false) { + return null; + } } if ($index === 0 && $event->getFunctionId() === 'printf') { diff --git a/tests/ReturnTypeProvider/SprintfTest.php b/tests/ReturnTypeProvider/SprintfTest.php index d66bd664a..b09193812 100644 --- a/tests/ReturnTypeProvider/SprintfTest.php +++ b/tests/ReturnTypeProvider/SprintfTest.php @@ -131,6 +131,18 @@ class SprintfTest extends TestCase '$val===' => 'int<0, max>', ], ]; + + yield 'sprintfEmptyStringFormat' => [ + 'code' => ' [ + '$val===' => 'string', + ], + 'ignored_issues' => [ + 'InvalidArgument' + ], + ]; } public function providerInvalidCodeParse(): iterable @@ -184,6 +196,18 @@ class SprintfTest extends TestCase ', 'error_message' => 'InvalidArgument', ], + 'sprintfEmptyFormat' => [ + 'code' => ' 'InvalidArgument', + ], + 'sprintfFormatWithoutPlaceholders' => [ + 'code' => ' 'InvalidArgument', + ], ]; } } From 9232e4e76bc7a790b14d462ecceebd318560894f Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Mon, 12 Jun 2023 18:46:15 +0200 Subject: [PATCH 3/7] code style --- .../ReturnTypeProvider/SprintfReturnTypeProvider.php | 11 ++++------- tests/ReturnTypeProvider/SprintfTest.php | 2 +- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php b/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php index d8f18a407..684eaea31 100644 --- a/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php +++ b/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php @@ -101,7 +101,8 @@ class SprintfReturnTypeProvider implements FunctionReturnTypeProviderInterface if ($result === $type->getSingleStringLiteral()->value) { IssueBuffer::maybeAdd( new InvalidArgument( - 'Argument 1 of ' . $event->getFunctionId() . ' does not contain any placeholders', + 'Argument 1 of ' . $event->getFunctionId() + . ' does not contain any placeholders', $event->getCodeLocation(), $event->getFunctionId(), ), @@ -216,12 +217,8 @@ class SprintfReturnTypeProvider implements FunctionReturnTypeProviderInterface return Type::getNonEmptyString(); } - /** - * if we didn't have a valid result, the pattern is invalid or not yet supported by the return type provider - * PHP 7 can have false here - * - * @psalm-suppress RedundantConditionGivenDocblockType - */ + // if we didn't have a valid result + // the pattern is invalid or not yet supported by the return type provider if ($initial_result === null || $initial_result === false) { return null; } diff --git a/tests/ReturnTypeProvider/SprintfTest.php b/tests/ReturnTypeProvider/SprintfTest.php index b09193812..3da9a4847 100644 --- a/tests/ReturnTypeProvider/SprintfTest.php +++ b/tests/ReturnTypeProvider/SprintfTest.php @@ -140,7 +140,7 @@ class SprintfTest extends TestCase '$val===' => 'string', ], 'ignored_issues' => [ - 'InvalidArgument' + 'InvalidArgument', ], ]; } From 9bc4752e29f7854286e38c9c81db596c916a31ce Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Mon, 12 Jun 2023 19:05:35 +0200 Subject: [PATCH 4/7] add checks for complex empty format and make return type more specific --- .../SprintfReturnTypeProvider.php | 24 ++++++++++++++++++- tests/ReturnTypeProvider/SprintfTest.php | 18 ++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php b/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php index 684eaea31..3f0ca8509 100644 --- a/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php +++ b/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php @@ -82,7 +82,29 @@ class SprintfReturnTypeProvider implements FunctionReturnTypeProviderInterface $statements_source->getSuppressedIssues(), ); - return null; + if ($event->getFunctionId() === 'printf') { + return Type::getInt(false, 0); + } + + return Type::getString(''); + } + + // there are probably additional formats that return an empty string, this is just a starting point + if (preg_match('/^%(?:\d+\$)?[-+]?0(\.0)?s$/', $type->getSingleStringLiteral()->value) === 1) { + IssueBuffer::maybeAdd( + new InvalidArgument( + 'The pattern of argument 1 of ' . $event->getFunctionId() . ' will always return an empty string', + $event->getCodeLocation(), + $event->getFunctionId(), + ), + $statements_source->getSuppressedIssues(), + ); + + if ($event->getFunctionId() === 'printf') { + return Type::getInt(false, 0); + } + + return Type::getString(''); } $args_count = count($call_args) - 1; diff --git a/tests/ReturnTypeProvider/SprintfTest.php b/tests/ReturnTypeProvider/SprintfTest.php index 3da9a4847..a27dec6eb 100644 --- a/tests/ReturnTypeProvider/SprintfTest.php +++ b/tests/ReturnTypeProvider/SprintfTest.php @@ -143,6 +143,18 @@ class SprintfTest extends TestCase 'InvalidArgument', ], ]; + + yield 'sprintfPaddedEmptyStringFormat' => [ + 'code' => ' [ + '$val===' => 'string', + ], + 'ignored_issues' => [ + 'InvalidArgument', + ], + ]; } public function providerInvalidCodeParse(): iterable @@ -208,6 +220,12 @@ class SprintfTest extends TestCase ', 'error_message' => 'InvalidArgument', ], + 'sprintfPaddedComplexEmptyStringFormat' => [ + 'code' => ' 'InvalidArgument', + ], ]; } } From d0f14ba19ec9b8134ccad745ced5c28f52cac00a Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Mon, 12 Jun 2023 19:31:11 +0200 Subject: [PATCH 5/7] ignore complex placeholders for now Fix https://github.com/vimeo/psalm/issues/9900 Fix https://github.com/vimeo/psalm/issues/9901 Fix original core stubs sprintf returning a wrong type too (unrelated to return type provider) for placeholders that are always empty --- .../SprintfReturnTypeProvider.php | 12 ++++++++- tests/ReturnTypeProvider/SprintfTest.php | 27 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php b/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php index 3f0ca8509..9344c33b9 100644 --- a/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php +++ b/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php @@ -90,7 +90,7 @@ class SprintfReturnTypeProvider implements FunctionReturnTypeProviderInterface } // there are probably additional formats that return an empty string, this is just a starting point - if (preg_match('/^%(?:\d+\$)?[-+]?0(\.0)?s$/', $type->getSingleStringLiteral()->value) === 1) { + if (preg_match('/^%(?:\d+\$)?[-+]?0(?:\.0)?s$/', $type->getSingleStringLiteral()->value) === 1) { IssueBuffer::maybeAdd( new InvalidArgument( 'The pattern of argument 1 of ' . $event->getFunctionId() . ' will always return an empty string', @@ -107,6 +107,16 @@ class SprintfReturnTypeProvider implements FunctionReturnTypeProviderInterface return Type::getString(''); } + // placeholders are too complex to handle for now + if (preg_match('/%(?:\d+\$)?[-+]?(?:\d+|\*)(?:\.(?:\d+|\*))?[bcdouxXeEfFgGhHs]/', $type->getSingleStringLiteral()->value) === 1) { + if ($event->getFunctionId() === 'printf') { + return null; + } + + // the core stubs are wrong for these too, since these might be empty strings, e.g. sprintf(\'%0.*s\', 0, "abc") + return Type::getString(); + } + $args_count = count($call_args) - 1; $dummy = array_fill(0, $args_count, ''); diff --git a/tests/ReturnTypeProvider/SprintfTest.php b/tests/ReturnTypeProvider/SprintfTest.php index a27dec6eb..445f31890 100644 --- a/tests/ReturnTypeProvider/SprintfTest.php +++ b/tests/ReturnTypeProvider/SprintfTest.php @@ -155,6 +155,33 @@ class SprintfTest extends TestCase 'InvalidArgument', ], ]; + + yield 'sprintfComplexPlaceholderNotYetSupported1' => [ + 'code' => ' [ + '$val===' => 'string', + ], + ]; + + yield 'sprintfComplexPlaceholderNotYetSupported2' => [ + 'code' => ' [ + '$val===' => 'string', + ], + ]; + + yield 'sprintfComplexPlaceholderNotYetSupported3' => [ + 'code' => ' [ + '$val===' => 'string', + ], + ]; } public function providerInvalidCodeParse(): iterable From 96988ea5ec40155ee1aaee25f376a948ac95b59a Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Mon, 12 Jun 2023 19:35:19 +0200 Subject: [PATCH 6/7] code style --- .../SprintfReturnTypeProvider.php | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php b/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php index 9344c33b9..4b770d9e8 100644 --- a/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php +++ b/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php @@ -22,6 +22,7 @@ use ValueError; use function array_fill; use function array_pop; use function count; +use function preg_match; use function sprintf; /** @@ -93,7 +94,8 @@ class SprintfReturnTypeProvider implements FunctionReturnTypeProviderInterface if (preg_match('/^%(?:\d+\$)?[-+]?0(?:\.0)?s$/', $type->getSingleStringLiteral()->value) === 1) { IssueBuffer::maybeAdd( new InvalidArgument( - 'The pattern of argument 1 of ' . $event->getFunctionId() . ' will always return an empty string', + 'The pattern of argument 1 of ' . $event->getFunctionId() + . ' will always return an empty string', $event->getCodeLocation(), $event->getFunctionId(), ), @@ -107,13 +109,17 @@ class SprintfReturnTypeProvider implements FunctionReturnTypeProviderInterface return Type::getString(''); } - // placeholders are too complex to handle for now - if (preg_match('/%(?:\d+\$)?[-+]?(?:\d+|\*)(?:\.(?:\d+|\*))?[bcdouxXeEfFgGhHs]/', $type->getSingleStringLiteral()->value) === 1) { + // these placeholders are too complex to handle for now + if (preg_match( + '/%(?:\d+\$)?[-+]?(?:\d+|\*)(?:\.(?:\d+|\*))?[bcdouxXeEfFgGhHs]/', + $type->getSingleStringLiteral()->value, + ) === 1) { if ($event->getFunctionId() === 'printf') { return null; } - // the core stubs are wrong for these too, since these might be empty strings, e.g. sprintf(\'%0.*s\', 0, "abc") + // the core stubs are wrong for these too, since these might be empty strings + // e.g. sprintf(\'%0.*s\', 0, "abc") return Type::getString(); } From 8a8aeb6452e573b71a6fc04dcc7ad45e138ab43c Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Mon, 12 Jun 2023 19:43:30 +0200 Subject: [PATCH 7/7] fix tests --- tests/ReturnTypeProvider/SprintfTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ReturnTypeProvider/SprintfTest.php b/tests/ReturnTypeProvider/SprintfTest.php index 445f31890..60e112f3e 100644 --- a/tests/ReturnTypeProvider/SprintfTest.php +++ b/tests/ReturnTypeProvider/SprintfTest.php @@ -137,7 +137,7 @@ class SprintfTest extends TestCase $val = sprintf("", "abc"); ', 'assertions' => [ - '$val===' => 'string', + '$val===' => '\'\'', ], 'ignored_issues' => [ 'InvalidArgument', @@ -149,7 +149,7 @@ class SprintfTest extends TestCase $val = sprintf("%0.0s", "abc"); ', 'assertions' => [ - '$val===' => 'string', + '$val===' => '\'\'', ], 'ignored_issues' => [ 'InvalidArgument',