Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong template infer inheritance #4524

Closed
thomasvargiu opened this issue Nov 9, 2020 · 16 comments
Closed

Wrong template infer inheritance #4524

thomasvargiu opened this issue Nov 9, 2020 · 16 comments
Labels

Comments

@thomasvargiu
Copy link
Contributor

thomasvargiu commented Nov 9, 2020

EDIT: see comment below, this isn't a problem with stubs, I can reproduce it on playground: https://psalm.dev/r/72dd0c2084

I'm creating a plugin with stubs, and I created a dedicated branch for this issue.

The problem is with method Identity::map().
It's the implementation from the interface Functor, which is another stub with the same signature.

Psalm doesn't recognize the return type as you can see from the CI (https://github.com/thomasvargiu/psalm-plugin-fantasy-land/runs/1376311199?check_suite_focus=true#step:8:50).

So, calling the following method with fn (T): int returns a Functor<empty> instead a Functor<int>.

/**
 * @template T
 */
interface Functor
{
    /**
     * @template F
     * @psalm-param callable(T): F $function
     * @psalm-return Functor<F>
     */
    public function map(callable $function) : Functor;
}

The weird thing is that if I remove the docblock from the the original interface file it works as expected.

@psalm-github-bot
Copy link

Hey @thomasvargiu, can you reproduce the issue on https://psalm.dev ?

@thomasvargiu thomasvargiu changed the title Wrong template infer with stubs and original docblock Wrong template infer Nov 10, 2020
@thomasvargiu
Copy link
Contributor Author

thomasvargiu commented Nov 10, 2020

The problem isn't just on stubs:

Skipping the empty Monad interface and using Applicative directly it works as expected.
Removing param docblock from Pointed works as expected.

https://psalm.dev/r/72dd0c2084

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/72dd0c2084
<?php

interface Pointed
{
    /**
     * @param mixed $value
     * @return mixed
     */
    public static function of($value);
}

/**
 * @template T
 */
interface Applicative extends Pointed
{
    /**
     * @template U
     * @param U $value
     * @return Applicative<U>
     */
    public static function of($value);
}

/**
 * @template T
 * @template-extends Applicative<T>
 */
interface Monad extends Applicative
{
}

/**
 * @template T
 * @template-implements Monad<T>
 */
class FakeMonad implements Monad
{
  /** @psalm-suppress InvalidReturnType */
  public static function of($value)
  {
  }
}

/**
 * @template T
 * @template-implements Applicative<T>
 */
class FakeMonad2 implements Applicative
{
  /** @psalm-suppress InvalidReturnType */
  public static function of($value)
  {
  }
}

/** @psalm-trace $value */
$value = FakeMonad::of('foo');

/** @psalm-trace $value2 */
$value2 = FakeMonad2::of('foo');
Psalm output (using commit 4cb8e86):

INFO: Trace - 58:1 - $value: Applicative<empty>

INFO: Trace - 61:1 - $value2: Applicative<string(foo)>

@thomasvargiu thomasvargiu changed the title Wrong template infer Wrong template infer inheritance Nov 10, 2020
@thomasvargiu
Copy link
Contributor Author

Simplified: https://psalm.dev/r/0b06235633

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/0b06235633
<?php

/**
 * @template T
 */
interface A {
    /**
     * @template U
     * @param callable(T): U $func
     * @return U
     */
    function test(callable $func);
}

/**
 * @template T
 * @template-extends A<T>
 */
interface B extends A
{
}

/**
 * @template T
 * @template-extends B<T>
 */
interface C extends B
{
}


/** @var B<mixed> $interface */
$c = $interface->test(fn (int $foo) => 5); // error is thrown correctly using B

/** @var C<mixed> $interface */
$c = $interface->test(fn (int $foo) => 5); // no errors using C
Psalm output (using commit 5ad1e80):

INFO: MixedArgumentTypeCoercion - 33:18 - Type mixed should be a subtype of int

@muglug muglug added the bug label Nov 11, 2020
@muglug
Copy link
Collaborator

muglug commented Nov 11, 2020

Even more concise version:

<?php

class Foo {}
class FooChild extends Foo {}

/**
 * @template T
 */
interface A {
    /**
     * @template U
     * @param callable(T): U $func
     * @return U
     */
    function test(callable $func);
}

/**
 * @template T
 * @template-extends A<T>
 */
interface B extends A {}

/**
 * @template T
 * @template-extends B<T>
 */
interface C extends B {}

/**
 * @param B<Foo> $b
 */
function first(B $b) : void {
    $f = fn (FooChild $foo) : FooChild => $foo;
    $b->test($f); // fails
}

/**
 * @param C<Foo> $c
 */
function second(C $c) : void {
    $f = fn (FooChild $foo) : FooChild => $foo;
    $c->test($f); // should fail but does not
}

@thomasvargiu
Copy link
Contributor Author

thomasvargiu commented Nov 11, 2020

@muglug Isn't resolved yet.

If Pointed has a a docblock, it doesn work: https://psalm.dev/r/40e629c932

It works:

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/40e629c932
<?php

interface Pointed
{
    /**
     * @param mixed $value
     * @return mixed
     */
    public static function of($value);
}

/**
 * @template T
 */
interface A extends Pointed
{
    /**
     * @template U
     * @param U $value
     * @return A<U>
     */
    public static function of($value);
}

/**
 * @template T
 * @template-extends A<T>
 */
interface B extends A
{
}

/**
 * @template T
 * @template-implements B<T>
 */
class Foo implements B
{
    /** @psalm-suppress InvalidReturnType */
    public static function of($value)
    {
    }
}

/** @psalm-trace $foo */
$foo = Foo::of('foo');
Psalm output (using commit a8d7248):

INFO: Trace - 46:1 - $foo: A<empty>
https://psalm.dev/r/948622b9f6
<?php

interface Pointed
{
    public static function of($value);
}

/**
 * @template T
 */
interface A extends Pointed
{
    /**
     * @template U
     * @param U $value
     * @return A<U>
     */
    public static function of($value);
}

/**
 * @template T
 * @template-extends A<T>
 */
interface B extends A
{
}

/**
 * @template T
 * @template-implements B<T>
 */
class Foo implements B
{
    /** @psalm-suppress InvalidReturnType */
    public static function of($value)
    {
    }
}

/** @psalm-trace $foo */
$foo = Foo::of('foo');
Psalm output (using commit a8d7248):

INFO: Trace - 42:1 - $foo: A<string(foo)>

INFO: MissingParamType - 5:31 - Parameter $value has no provided type

INFO: MissingReturnType - 5:28 - Method Pointed::of does not have a return type
https://psalm.dev/r/ff098b1e0a
<?php

interface Pointed
{
    /**
     * @param mixed $value
     * @return mixed
     */
    public static function of($value);
}

/**
 * @template T
 */
interface A extends Pointed
{
    /**
     * @template U
     * @param U $value
     * @return A<U>
     */
    public static function of($value);
}

/**
 * @template T
 * @template-implements A<T>
 */
class Foo implements A
{
    /** @psalm-suppress InvalidReturnType */
    public static function of($value)
    {
    }
}

/** @psalm-trace $foo */
$foo = Foo::of('foo');
Psalm output (using commit a8d7248):

INFO: Trace - 38:1 - $foo: A<string(foo)>

@muglug
Copy link
Collaborator

muglug commented Nov 11, 2020

Yeah, two inherited interfaces each with their own docblock confuses Psalm – it's not immediately clear which docblock should take precedence.

@thomasvargiu
Copy link
Contributor Author

thomasvargiu commented Nov 11, 2020

So, should you open this issue again? Or are you working on it?

@muglug muglug reopened this Nov 12, 2020
@muglug
Copy link
Collaborator

muglug commented Nov 12, 2020

Sorry, I was being a bit lazy.

@muglug muglug closed this as completed in b7551e7 Nov 12, 2020
@thomasvargiu
Copy link
Contributor Author

thomasvargiu commented Nov 12, 2020

@muglug I was about to open a PR.

I was fixing it too (I was running tests), changing the following line inverting parameters to mantain an ordered list of parent_interfaces and consequentially of documenting_method_ids respecting the interface chain.

$storage->parent_interfaces = array_merge($parent_interfaces, $storage->parent_interfaces);

@muglug
Copy link
Collaborator

muglug commented Nov 12, 2020

Problem is you can't always rely on that ordering – tests/ArrayAssignmentTest.php was breaking in that scenario because there's something that re-adds a parent interface.

@muglug
Copy link
Collaborator

muglug commented Nov 12, 2020

But thanks, I always appreciate PRs!

@thomasvargiu
Copy link
Contributor Author

@muglug I think I found another issue about infer and inheritance:

https://psalm.dev/r/7f937bb1d0

Since Foo::test() returns Foo, which implements B, which implements A, the signature should be valid.
Instead Psalm requires that Foo::test() returns a type of B.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/7f937bb1d0
<?php

/**
 * @template T
 */
interface A
{
    /**
     * @template F
     * @param F $value
     * @return A<F>
     */
    public function map($value): A;
}

/**
 * @template T
 * @template-extends A<T>
 */
interface B extends A
{
    /**
     * @template F
     * @param F $value
     * @return B<F>
     */
    public function map($value): A;
}

/**
 * @template T
 * @template-implements B<T>
 */
class Foo implements B
{
    /**
     * @var T
     */
    private $value;

    /**
     * @param T $value
     */
    public function __construct($value)
    {
        $this->value = $value;
    }

    
    public function map($value): A
    {
        return new Foo($value);
    }
}

$foo = new Foo('str');
/** @psalm-trace $value */
$value = $foo->map('string');
Psalm output (using commit b7551e7):

INFO: Trace - 58:1 - $value: A<empty>

danog pushed a commit to danog/psalm that referenced this issue Jan 29, 2021
danog pushed a commit to danog/psalm that referenced this issue Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants