Skip to content

Skip UnusedMethodParametersRule for private methods referenced as callables within their class - #6805

Open
phpstan-bot wants to merge 3 commits into
phpstan:2.3.xfrom
phpstan-bot:create-pull-request/patch-k8557hr
Open

phpstan-bot wants to merge 3 commits into
phpstan:2.3.xfrom
phpstan-bot:create-pull-request/patch-k8557hr

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

With bleeding edge unusedParameters, UnusedMethodParametersRule reported method.unusedParameter on private methods that the class hands out as callables (for example self::plus(...) or Closure::fromCallable([self::class, 'minus'])). Once a method escapes this way, outside code calls it with every argument, so its signature is a contract. The rule now skips private methods that their class references as a callable.

Changes

  • src/Rules/Methods/UnusedMethodParametersRule.php: after computing errors, and only if there are any, scan the class-like AST for a callable reference to the method:
    • first-class callables: self::m(...), static::m(...), $this->m(...) (any MethodCall/StaticCall first-class callable with that name)
    • callable arrays: [self::class, 'm'], [$this, 'm'] (this also covers Closure::fromCallable([...]), array_map([...], ...), and similar)
    • callable strings: 'self::m', 'Foo::m'
    • names match case-insensitively
    • trait methods: both the using class and the trait are scanned
    • anonymous classes: matched by their start line
  • conf/config.level4.neon: inject @defaultAnalysisParser into the rule. The parser is cached, so the file being analysed is not parsed again.
  • Probed and not changed:
    • nullsafe first-class callables ($x?->m(...)): not valid PHP.
    • UnusedFunctionParametersRule: it reports all global functions, public ones included, so being referenced as a callable doesn't change its premise.

Root cause

The rule's premise is that "a private method has no callers outside the class". A callable reference to the method breaks that premise, but the rule never looked for one.

Test

tests/PHPStan/Rules/Methods/data/bug-15405.php covers:

  • the reproducer from the issue
  • the other callable forms: instance first-class callable, [$this, '...'], 'self::...', a mixed-case name
  • a trait method referenced from the using class
  • a trait method referenced from the trait itself
  • an anonymous class
  • negative cases: a private method that is never referenced, and one that is only called directly. Both are still reported.

Fixes phpstan/phpstan#15405

🤖 Generated with Claude Code

…allables within their class

- A private method handed out as a callable is called from outside the class
  with every argument, so its signature is a contract like a public method's.
- Before reporting, the rule scans the declaring class-like AST (via the cached
  `defaultAnalysisParser`) for a first-class callable (`self::m(...)`,
  `static::m(...)`, `$this->m(...)`), a callable array (`[self::class, 'm']`,
  `[$this, 'm']`, also inside `Closure::fromCallable()`) or a callable string
  (`'self::m'`, `'Foo::m'`). Names match case-insensitively.
- For trait methods, both the using class and the trait itself are scanned.
  Anonymous classes are matched by their start line.
- The scan runs only when there would be an error to report, so methods with
  no unused parameters cost nothing extra.
- Probed nullsafe first-class callables: PHP does not allow them, so they are
  not handled. `UnusedFunctionParametersRule` already reports global functions
  whatever their visibility, so it was left alone.

@staabm staabm left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can't this be implemented via ClassStatementsGatherer->methodCalls instead?

phpstan-bot and others added 2 commits October 24, 2026 16:21
ClassMethodsNode now carries the VariableWritesNode of each method body
together with the scope inside the method, so rules listening to it can
combine parameter usage with the method calls gathered in the class.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Instead of re-parsing the file and searching the class AST, the rule now
listens to ClassMethodsNode and finds callable references among the
method calls ClassStatementsGatherer collected. Errors are still
transformed with the scope inside the method, so they stay identical
(including trait context).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@phpstan-bot

Copy link
Copy Markdown
Collaborator Author

Yes, it can be done that way. I've reworked the rule to use the gatherer's methodCalls and pushed it as two commits. One behaviour changes: string callables like 'self::m' are now reported again (details at the end).

Why it needed more than a lookup: the rule listened to VariableWritesNode, which fires once per method body, before the class's method calls have all been gathered. To use getMethodCalls() the rule has to listen to ClassMethodsNode instead. That node didn't carry the per-method parameter-usage data, so the first commit adds it.

Commit 1, 86d32a096 – gather each method's VariableWritesNode:

  • ClassStatementsGatherer now also collects each method's VariableWritesNode, together with the scope inside the method. They're stored in a new PHPStan\Node\Method\MethodVariableWrites value object.
  • ClassMethodsNode takes them as an optional fifth constructor argument (default []) and exposes getMethodVariableWrites().
  • ClassStatementsGatherer has a C++ mirror, so I made the same change in turbo-ext: the gatherer, ClassLikeHandler.cpp, the class-map entries, the regenerated header, and the gatherer comparison test now also compares the new getter.

Commit 2, 81a6afb90 – rewrite UnusedMethodParametersRule on ClassMethodsNode:

  • The re-parsing and AST search are gone, and so is the @defaultAnalysisParser argument in conf/config.level4.neon.
  • A method counts as referenced as a callable if getMethodCalls() contains:
    • a first-class callable with that name;
    • a first-class callable whose name is dynamic and resolves to constant strings. If the name can't be resolved at all, the rule reports nothing for the class.
    • a callable array whose type resolves to that method name.
  • Errors are still built with the scope inside the method (through RuleErrorTransformer), so their output is unchanged. A trait method is still reported in the trait's file, "in context of class C". I checked this with a two-file trait example.
  • Methods declared in traits and anonymous classes are handled by the gatherer itself, so the extra trait scan is gone.

Behaviour change: callable strings like 'self::stringCallable' aren't in methodCalls, so they are reported again. UnusedPrivateMethodRule already reports that same method as unused, so the two rules now agree. The test expects the error at line 54. If you want string callables skipped, that needs a separate addition.

Verification:

  • make tests passes, both without the extension and with it loaded (22,597 tests, 106 skipped).
  • make phpstan reports no errors.
  • The turbo checks pass: side-by-side.php, smoke.php, signature-parity.php and class-statements-gatherer.php.
  • Analysis output on the test data and the trait example is byte-identical with the extension on and off.
  • I didn't benchmark; the change only adds one more collected list to the gatherer.

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.3.0] False positive method.unusedParameter for a private method passed around as a callable

2 participants