False positive `Dynamic call to static method` when mixing in another class and implementing `__call` and `__callStatic`

[msbit]

When you've a class Foo that:

• provides implementations of both __call and __callStatic, and
• mixes in another class Bar that has an instance method bar

creating an instance of Foo and calling that instance method results in an error of the form:

Dynamic call to static method Bar::bar(). [identifier=staticMethod.dynamicCall]

Playground: https://phpstan.org/r/bfd29cdb-749b-4b97-bd50-92ecb0fcfc92

Thanks!

[msbit]

Looking over the main repo, I see the following issues that were resolved around this space:

phpstan/phpstan#3641
phpstan/phpstan#5536

With a common mixed in class defined as follows:

class MixedIn
{
  function method(): void {}
}

The first reported false positives with the following:

/** @mixin MixedIn */
class Mixer
{
  static function __callStatic(): void {}
}

Mixer::method();

the second false positives with this modified version:

/** @mixin MixedIn */
class Mixer
{
  function __call(): void {}

  static function __callStatic(): void {}
}

Mixer::method();

and what I'm reporting false positives with is a further modified version:

/** @mixin MixedIn */
class Mixer
{
  function __call(): void {}

  static function __callStatic(): void {}
}

(new Mixer)->method();

It's not clear to me whether DynamicCallOnStaticMethodsRule::processNode has enough information to operate properly at this point; nor even whether MixinMethodsClassReflectionExtension::findMethod does. It feels like somewhere along the line, either:

• asking for a method called Mixer::method could return two versions, one static and one dynamic, or
• the calling context (static/dynamic) could be passed to the method finding logic, so it can return the appropriate one

[jnoordsij]

+1 here

I'm actually wondering if the current behavior for mixins is entirely logical, given from what it looks like it currently simply does: __callStatic is defined implies all mixin calls are static. However, if __call is also present, I think this is simply not correct.

For example, Laravel comes with a Macroable trait, whose implementation effectively allows calling any user-defined macro both dynamically and statically; see https://github.com/laravel/framework/blob/b6ee18a4e2feb3c80790de46d0fd78421afe65ea/src/Illuminate/Macroable/Traits/Macroable.php#L12. For any such class, probably both calls should be fine, so this should not be an immediate issue. However I think it is still somewhat deceptive coding that a static call is required where actually the result is a dynamic call.

Then within some classes, there's further overriding of this behavior, which may introduce dynamic calls, see e.g.
https://github.com/laravel/framework/blob/b6ee18a4e2feb3c80790de46d0fd78421afe65ea/src/Illuminate/Routing/RouteRegistrar.php#L278-L312. For such cases, refactoring to a static call will actually break your code rather than improve it.

As I think it's generally very hard if not impossible to deduce which syntax is correct for mixin calls, as it heavily relies on the actual implementation of both __call and __callStatic. So maybe there should be some 'ambiguity' allowed, i.e. consider the method as neither static nor dynamic and never report issues, as I'd say false positives breaking code outweigh the potential benefits of spotting incorrect semantics.

(If interested in more concrete examples of errors reported on Laravel setups with this, LMK!)