vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.55k stars 660 forks source link

[Question] Stubs prioity, MethodReturnTypeProviderInterface for documented magic methods #3937

Open zlodes opened 4 years ago

zlodes commented 4 years ago

Hello!

I have the vendor class (static proxy):


/**
 * @method static mixed bar(Closure $closure)
 */
class Foo {
    public static __callStatic($name, $arguments) { ... }
}

I know that the method return type is same as closure return type. And I made the hook implements MethodReturnTypeProviderInterface, but it doesn't work with @method in docblock.

Tried to use stub, but it doesn't work too. Stub file:

<?php

namespace Illuminate\Support\Facades;

use Closure;

class DB extends Facade
{
    /**
     * @template T
     *
     * @param Closure $closure
     * @param int $attempts
     * @psalm-param Closure(): T $closure
     *
     * @psalm-return T
     */
    public static function transaction(Closure $closure, int $attempts = 1)
    {
        // ...
    }
}

Help please. Thanks! 🙏

Previous question: #3930

weirdan commented 4 years ago

Tried to use stub, but it doesn't work too.

Can you elaborate? I was going to suggest exactly that kind of stub.

zlodes commented 4 years ago

@weirdan there is no effect of stub transactions method. But after removing @method docblock from original (vendor) class it works fine.

Seems that docblock has higher priority and stub is ignored.

MethodReturnTypeProviderInterface hook is ignored to. Method getMethodReturnType was not called.

weirdan commented 4 years ago

Looks like a bug to me, as stubs and providers are the way to override things. Any chance you can assemble a self-contained repro , on psalm.dev or in a separate github repo?

zlodes commented 4 years ago

Okay, I'll do it tomorrow.

zlodes commented 4 years ago

@weirdan here a demo project: https://github.com/zlodes/psalm-stub-bug

weirdan commented 4 years ago

Great, checking. I get the same issues emitted with and without the plugin though, both on Psalm stable and dev-master.

weirdan commented 4 years ago

This can be patched as follows:

diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php
index bbe88d8c5..ab7c4fbad 100644
--- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php
+++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php
@@ -536,7 +536,9 @@ public static function analyze(
                         $statements_analyzer->getSource()
                     )
                     || (isset($class_storage->pseudo_static_methods[$method_name_lc])
-                        && ($config->use_phpdoc_method_without_magic_or_parent || $class_storage->parent_class))
+                        && ($config->use_phpdoc_method_without_magic_or_parent || $class_storage->parent_class)
+                        && !($class_storage->stubbed && $naive_method_exists)
+                    )
                 ) {
                     $callstatic_id = new MethodIdentifier(
                         $fq_class_name,

This makes the repro with plugin enabled pass, but it would introduce inconsistency into how @method and actual methods are handled in stubs comparing to the way they work for real code. I'm afraid I'm out of my depth here, so this would have to wait for when @muglug have some time.

the-toster commented 3 years ago

I'm having same issue.