fix return type of sprintf with single %s format #3167
Conversation
|
please note the second commit in the PR that updates an existing test (930266c) That test case was invalid to begin with as the line in question would throw an The other option would be to remove the test-case entirely. I can totally do both. |
|
(heads up: I'm about to force-push a much better test that has many more assertions) |
In the return type extension of sprintf(), there was a shortcut to assume the return type to be non-falsy-string when the format-string is non-falsy string. That's not correct though if the format string is just a single '%s' placeholder (or a single '%x$s' placeholder). In that case, if the requested argument is of stringy-type, then the return type is the type of the stringy argument. This commit extends the existing support for single-placeholder templates that currently deals with numeric placeholders to also deal with %s this fixes #11201
the format string is referencing an invalid argument (argument 10) (which will cause an error at runtime) and then asserts the return type to be `numeric-string`. Now that the previous commit has an improved check for invalid argument number specifiers, the numeric-string shortcut won't work any more. I'm updating this test because the test-case will throw at runtime anyways, so both return-types are pointless anyways.
|
I am also working on this extension for a few hours with some kind of general overhaul. it will also fix your issue. we should sync how we want to proceed :) |
|
up to you. My PR is, by virtue of me being totally unfamiliar with the code base, the minimal change required to fix the problem. Feel free to close this one in preference to your overhaul, or merge this one as a stop-gap (the underlying bug has cased real-life issues for me) and rebase your overhaul on top of this one - I'm totally fine with both. |
|
just opened a draft of my current state. would it be ok for you, that we concentrate on my PR first (because it moves a lot of moving parts, and we put your one later on top of it?). |
|
absolutely. that's good for me too. I'll have a look whether your PR fixes the underlying bug and if so, all that's needed from me is my test-case (66262bb#diff-4d390d0bba719512f2479a712fd2d14e7131ad079d4dd8fcced037744e3f1df8) |
|
Thank you! If you have any improvements, you can send a follow-up PR 😊 |
|
haha - it looks like @ondrejmirtes just caused some additional work for @staabm Thank you for deciding for us - that saves me personally quite a bit of work #3168 doesn't fix phpstan/phpstan#11201 on its own I think. |
In the return type extension of sprintf(), there was a shortcut to assume the return type to be non-falsy-string when the format-string is non-falsy string.
That's not correct though if the format string is just a single '%s' placeholder (or a single '%x$s' placeholder). In that case, if the requested argument is of stringy-type, then the return type is the type of the stringy argument.
This commit extends the existing support for single-placeholder templates that currently deals with numeric placeholders to also deal with %s
this fixes phpstan/phpstan#11201