Report non-zero integer literals passed directly to parameters with allowed constants - #6565
phpstan-bot wants to merge 2 commits into
Conversation
|
I have three thought about this @staabm
|
|
this PR adds one more case to a rule which was implemented in #5256
why do you think its opinionated?
the rules in question are about bitmasks(int) and single-value ints. I think its all about ints atm |
That's not the same idea than #5256 to me. The rule was about reporting because JSON_THROW_ON_ERROR is meant to be used as a flag for the fourth parameter so the developer certainly made a mistake and wants But, if someone writes this is a perfectly valid code which does the same ; that's why it could be a flag only enabled on strict-rules... I agree that it's not recommended to use an integer rather than the constant and I dunno if there is usecase where people does. But, still, to me it's like a new rule (the issue is flagged as feature-request) and not a bugfix. So it will require ondrej review and I prefer it because I don't feel confident merging this by myself. |
There was a problem hiding this comment.
the goal of this PR is not to report any literal value beeing passed into a callable.
instead we want new errors reported when a parameter is known to be bitmask of constants, and the effective value based on literal values beeing passed is not a valid value which can be constructed by the valid constants
I don't think it's the initial request
|
|
I changed the check as staabm asked: an integer literal is now reported only when its value can't be produced from the parameter's allowed constants. I pushed this as one commit, d48c9f2. How it works now (
Behaviour changes to note in the PR thread:
Tests: I rewrote VincentLanglet's point is still open: whether this should sit behind an option or strict-rules, and whether Ondřej should review it. That's for the maintainers to decide. |
…llowed constants - FunctionCallParametersCheck: for parameters that have an allowed-constants definition (single or bitmask), walk the argument's `|` tree and report every hardcoded non-zero `Scalar\Int_` leaf (identifier `argument.integerLiteral`, tip "Use constants instead."). `0` stays allowed as the "no flags" value; variables and other dynamic operands are ignored. - Added the new message to all callers of the shared check: functions, methods, static methods, constructors, attributes, callables and call_user_func(). - Updated existing expectations for round() mode and PDOStatement::setFetchMode() which pass integer literals. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Instead of reporting every non-zero integer literal passed to a parameter with allowed constants, resolve the effective value of the literals in the argument (including literals combined with `|`) and report it only when it does not match an allowed constant (single-value parameters) or cannot be constructed by combining allowed constants (bitmask parameters). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d48c9f2 to
4896818
Compare
Summary
Following up on the allowed-constants checks for flag parameters, this reports hardcoded integer literals that are passed directly (in the AST) to parameters that expect a constant, or a bitmask of constants. For example
json_encode($x, 1),json_encode($x, 1 | 2), and the2injson_encode($x, $one | 2)are now reported.Changes
src/Rules/FunctionCallParametersCheck.php: newfindNonZeroIntegerLiterals()walksBitwiseOrtrees and collects non-zeroScalar\Int_leaves. The check reports each of them asInteger literal %s is not allowed for %s of …(identifierargument.integerLiteral, tip "Use constants instead.") when the parameter hasgetAllowedConstants() !== null.0is still allowed because it is the usual "no flags" value, which an existing test already expects.CallToFunctionParametersRule,CallMethodsRule,CallStaticMethodsRule,InstantiationRule,AttributesCheck,CallCallablesRule, andCallUserFuncRule.Root cause
The allowed-constants check only looked at arguments made entirely of constant fetches (
resolveConstantReflections()). An integer literal, or a|expression that mixed in literals or variables, returnednull, so the argument wasn't checked at all.Test
tests/PHPStan/Rules/Functions/data/bug-14727.php: the playground reproducer, plus cases for0, a mix of constant and literal, named arguments, a single-value parameter (array_unique), andjson_decode's$flags.call_user_func(). They share the same code path and all report the literal.testRoundModePhp84andtestNamedParametersForMultiVariantFunctions, which pass literal modes.Fixes phpstan/phpstan#14727
🤖 Generated with Claude Code