Skip to content

Conversation

schlndh
Copy link
Contributor

@schlndh schlndh commented May 2, 2025

Motivation: I refactored code to use BcMath\Number instead of float, and I didn't notice a printf usage with %f, which lead to incorrect result. PHPStan didn't complain, because BcMath\Number has __toString().

Here is an executed version of the test file: https://3v4l.org/3pXMc

I left some things for possible future PRs to keep things simple:

  • Strict mode: enforce int for %d, int|float for %f, __stringandstringable for %s. This may be too strict for default PHPStan, but IMO it would be a good candidate for phpstan-strict-rules.
  • Handle multiple possibilities for format string.
  • Add support for vprintf/vsprintf.

@schlndh schlndh force-pushed the feature-checkPrintfParameterTypes branch from 8866323 to 9a4e490 Compare May 2, 2025 06:53
@schlndh schlndh force-pushed the feature-checkPrintfParameterTypes branch from 9a4e490 to ca3477d Compare July 17, 2025 15:57
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.

I'm a bit skeptical here. I think that a lot more types should be allowed in the placeholders. There are probably many combinations allowed by PHP but disallowed by this new rule. For example - you're allowing only int in %d but it also allows float: https://3v4l.org/XOB5o

@ondrejmirtes
Copy link
Member

If it's not the case, the message 'Placeholder #1 of function printf expects int, PrintfParamTypes\\FooStringable given' (the first assertion) is misleading.

@schlndh schlndh force-pushed the feature-checkPrintfParameterTypes branch from ca3477d to fd12705 Compare August 31, 2025 12:26
@schlndh
Copy link
Contributor Author

schlndh commented Aug 31, 2025

I improved the error messages. The rule allows passing float to %d (and hopefully other meaningfully usable cases as well), it was just not clear from the previous (quite poor) error messages.

Some edge-cases:

  • One parameter can be used by multiple placeholders. I decided to generate a separate error for each placeholder, instead of trying to combine them into one (especially since the placeholder is now a part of the error message).
  • One placeholder can reference multiple parameters (e.g. %*.*f - the *s each consume a positional parameter). So for cases where a * is used, I clarified whether it's the width, precision or value that's the problem. E.g. "%*.*f" (width).

Tests (8.2, ubuntu-latest) fail, but it seems to be unrelated to my PR ("UnitEnum" could not be found in the located source).

@schlndh schlndh requested a review from ondrejmirtes August 31, 2025 12:49
@schlndh schlndh force-pushed the feature-checkPrintfParameterTypes branch from 7e59347 to 89641e5 Compare September 5, 2025 11:27
@schlndh schlndh requested a review from ondrejmirtes September 5, 2025 11:45
@ondrejmirtes ondrejmirtes merged commit 4dd9bba into phpstan:2.1.x Sep 5, 2025
439 of 453 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants