psalm icon indicating copy to clipboard operation
psalm copied to clipboard

incorrect `InvalidTemplateParam` using when a `Closure` argument uses `T` as an argument.

Open azjezz opened this issue 3 years ago • 14 comments

given the following example: https://psalm.dev/r/7f1d36d1f8, the error here is incorrect.

Option type is immutable by itself, and adding @psalm-immutable annotation solves the problem: https://psalm.dev/r/3deed330bf

however, this leads to another issue, at this point, Option<T>::map<Tu>((Closure(T): Tu) $f): Option<Tu> is considered pure, allowing to declare get_organization_owner() function below as pure ( see: https://psalm.dev/r/4396258d6f ), this leads to another problem: get_organization_owner() is not actually pure, neither is map() method, get_organization_owner() calls Option::map(), which calls $f, which is an impure function ( in the example above it is indeed pure, but Organization::getOwner() can have side effects ), declaring just Option::map() as impure based on the fact that it calls an impure function ( $f ) that could result in side effects elsewhere but not in the option object itself is not possible, meaning we are forced to remove the @psalm-immutable annotation from the class Option, leading us back to the first issue.

azjezz avatar Jul 03 '22 01:07 azjezz

I found these snippets:

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

/**
 * @template-covariant T
 */
interface Option
{
    /**
     * Maps an `Option<T>` to `Option<Tu>` by applying a function to a contained value.
     *
     * @template Tu
     *
     * @param (Closure(T): Tu) $closure
     *
     * @return Option<Tu>
     */
    public function map(Closure $closure): Option;
}

class Owner {}
class Organization { public function getOwner(): Owner { return new Owner(); } }
class GovernmentOrganization extends Organization {}

/** @return Option<Organization> */
function get_org(): Option { exit(0); }

/** @return Option<GovernmentOrganization> */
function get_gov_org(): Option { exit(0); }

/**
 * @param Option<Organization> $organization
 *
 * @return Option<Owner>
 */
function get_organization_owner(Option $organization): Option
{
    return $organization->map(static fn(Organization $organization) => $organization->getOwner());
}

$_ = get_organization_owner(get_org());
$_ = get_organization_owner(get_gov_org());
Psalm output (using commit fbd240b):

ERROR: InvalidTemplateParam - 13:15 - Template param T of Option is marked covariant and cannot be used here
https://psalm.dev/r/3deed330bf
<?php

/**
 * @template-covariant T
 * @psalm-immutable
 */
interface Option
{
    /**
     * Maps an `Option<T>` to `Option<Tu>` by applying a function to a contained value.
     *
     * @template Tu
     *
     * @param (Closure(T): Tu) $closure
     *
     * @return Option<Tu>
     */
    public function map(Closure $closure): Option;
}

class Owner {}
class Organization { public function getOwner(): Owner { return new Owner(); } }
class GovernmentOrganization extends Organization {}

/** @return Option<Organization> */
function get_org(): Option { exit(0); }

/** @return Option<GovernmentOrganization> */
function get_gov_org(): Option { exit(0); }

/**
 * @param Option<Organization> $organization
 *
 * @return Option<Owner>
 */
function get_organization_owner(Option $organization): Option
{
    return $organization->map(static fn(Organization $organization) => $organization->getOwner());
}

$_ = get_organization_owner(get_org());
$_ = get_organization_owner(get_gov_org());
Psalm output (using commit fbd240b):

No issues!
https://psalm.dev/r/4396258d6f
<?php

/**
 * @template-covariant T
 * @psalm-immutable
 */
interface Option
{
    /**
     * Maps an `Option<T>` to `Option<Tu>` by applying a function to a contained value.
     *
     * @template Tu
     *
     * @param (Closure(T): Tu) $closure
     *
     * @return Option<Tu>
     */
    public function map(Closure $closure): Option;
}

class Owner {}
class Organization { public function getOwner(): Owner { return new Owner(); } }
class GovernmentOrganization extends Organization {}

/** @return Option<Organization> */
function get_org(): Option { exit(0); }

/** @return Option<GovernmentOrganization> */
function get_gov_org(): Option { exit(0); }

/**
 * @param Option<Organization> $organization
 *
 * @return Option<Owner>
 *
 * @pure
 */
function get_organization_owner(Option $organization): Option
{
    return $organization->map(static fn(Organization $organization) => $organization->getOwner());
}

$_ = get_organization_owner(get_org());
$_ = get_organization_owner(get_gov_org());
Psalm output (using commit fbd240b):

No issues!

psalm-github-bot[bot] avatar Jul 03 '22 01:07 psalm-github-bot[bot]

I think the issue boils down to this: https://psalm.dev/r/eb422e250d

not allowing an immutable object to call an impure function, even though we know for a fact that it cannot change the state of the object.

maybe a new annotation ( e.g: @psalm-internal-mutation-free ) can be used in such a function to declare that it is not totally pure, but it won't change the object itself, allowing the use of it in @psalm-immutable classes only.

azjezz avatar Jul 03 '22 02:07 azjezz

I found these snippets:

https://psalm.dev/r/eb422e250d
<?php

/**
 * @psalm-template-covariant T
 * @psalm-immutable
 */
final class ValueReference {
    /**
     * @param T $value
     */
    public function __construct(
        public readonly mixed $value
    ) {}

    /**
     * @template Tu
     *
     * @param (Closure(T): Tu) $f
     *
     * @return ValueReference<Tu>
     */
    public function map(Closure $f): ValueReference
    {
        $new = $f($this->value);
        
        return new ValueReference($new);
    }
}

$a = new ValueReference('hello');
$b = $a->map(fn(string $val) => $val . ' world');

print $b->value; // hello world
Psalm output (using commit fbd240b):

ERROR: ImpureFunctionCall - 24:16 - Cannot call an impure function from a mutation-free context

psalm-github-bot[bot] avatar Jul 03 '22 02:07 psalm-github-bot[bot]

for example, there's no difference technically between: https://psalm.dev/r/eb422e250d and https://psalm.dev/r/7be3725e19, but one is valid, while the other is not.

azjezz avatar Jul 03 '22 02:07 azjezz

I found these snippets:

https://psalm.dev/r/eb422e250d
<?php

/**
 * @psalm-template-covariant T
 * @psalm-immutable
 */
final class ValueReference {
    /**
     * @param T $value
     */
    public function __construct(
        public readonly mixed $value
    ) {}

    /**
     * @template Tu
     *
     * @param (Closure(T): Tu) $f
     *
     * @return ValueReference<Tu>
     */
    public function map(Closure $f): ValueReference
    {
        $new = $f($this->value);
        
        return new ValueReference($new);
    }
}

$a = new ValueReference('hello');
$b = $a->map(fn(string $val) => $val . ' world');

print $b->value; // hello world
Psalm output (using commit fbd240b):

ERROR: ImpureFunctionCall - 24:16 - Cannot call an impure function from a mutation-free context
https://psalm.dev/r/7be3725e19
<?php

/**
 * @psalm-template-covariant T
 * @psalm-immutable
 */
final class ValueReference {
    /**
     * @param T $value
     */
    public function __construct(
        public readonly mixed $value
    ) {}
}

/**
 * @template T
 * @template Tu
 *
 * @param ValueReference<T> $ref
 * @param (Closure(T): Tu) $f
 *
 * @return ValueReference<Tu>
 */
function map_reference(ValueReference $ref, Closure $f): ValueReference
{
  $new = $f($ref->value);
        
  return new ValueReference($new);
}

$a = new ValueReference('hello');
$b = map_reference($a, fn(string $val) => $val . ' world');

print $b->value; // hello world
Psalm output (using commit fbd240b):

No issues!

psalm-github-bot[bot] avatar Jul 03 '22 02:07 psalm-github-bot[bot]

Seems like the same sort of problem as #8116.

AndrolGenhald avatar Jul 03 '22 07:07 AndrolGenhald

I think i have a better solution for this problem.

The Option class given in the first example, is not immutable ( by psalm definition, which is completely side-effect free ), but rather a readonly class, what we can do is add support for @psalm-readonly annotation on classes ( same as readonly class Foo { .. } in PHP 8.2 ), so we would allow Closure arguments to take T as an argument themselves, and we know that the class is not side effect free, it's just that it's own state doesn't change.

I'll send a PR with tests which might help you understand what i mean more ^^"

azjezz avatar Jul 03 '22 08:07 azjezz

I think the issue boils down to this: https://psalm.dev/r/eb422e250d

not allowing an immutable object to call an impure function, even though we know for a fact that it cannot change the state of the object.

If $this->value is an object, it is passed by reference to $f. Therefore, it may be mutated. I think Psalm behaves correctly, and map() has to accept something like pure-callable but for Closure. Introduce pure-Closure type?

someniatko avatar Jul 03 '22 08:07 someniatko

I found these snippets:

https://psalm.dev/r/eb422e250d
<?php

/**
 * @psalm-template-covariant T
 * @psalm-immutable
 */
final class ValueReference {
    /**
     * @param T $value
     */
    public function __construct(
        public readonly mixed $value
    ) {}

    /**
     * @template Tu
     *
     * @param (Closure(T): Tu) $f
     *
     * @return ValueReference<Tu>
     */
    public function map(Closure $f): ValueReference
    {
        $new = $f($this->value);
        
        return new ValueReference($new);
    }
}

$a = new ValueReference('hello');
$b = $a->map(fn(string $val) => $val . ' world');

print $b->value; // hello world
Psalm output (using commit fbd240b):

ERROR: ImpureFunctionCall - 24:16 - Cannot call an impure function from a mutation-free context

psalm-github-bot[bot] avatar Jul 03 '22 08:07 psalm-github-bot[bot]

No, it actually has nothing to do with objects: https://psalm.dev/r/3033cae1ca

azjezz avatar Jul 03 '22 08:07 azjezz

I found these snippets:

https://psalm.dev/r/3033cae1ca
<?php

/**
 * @psalm-template-covariant T of string|int|float
 * @psalm-immutable
 */
final class ValueReference {
    /**
     * @param T $value
     */
    public function __construct(
        public readonly mixed $value
    ) {}

    /**
     * @template Tu of string|int|float
     *
     * @param (Closure(T): Tu) $f
     *
     * @return ValueReference<Tu>
     */
    public function map(Closure $f): ValueReference
    {
        $new = $f($this->value);

        return new ValueReference($new);
    }
}

$a = new ValueReference('hello');
$b = $a->map(fn(string $val) => $val . ' world');

print $b->value; // hello world
Psalm output (using commit fbd240b):

ERROR: ImpureFunctionCall - 24:16 - Cannot call an impure function from a mutation-free context

psalm-github-bot[bot] avatar Jul 03 '22 08:07 psalm-github-bot[bot]

Closure could still take its argument by reference though, and again mutate it. https://psalm.dev/r/3423b8e3e9

someniatko avatar Jul 21 '22 06:07 someniatko

@someniatko huh, you're right. I'd consider that a separate bug though. I think we need to add annotation for references in callables and make callable(&int) not be a subtype of callable(int), because the example I just gave crashes...

AndrolGenhald avatar Jul 21 '22 07:07 AndrolGenhald

I found these snippets:

https://psalm.dev/r/e59d68ba57
<?php

/** @param callable(int): int $closure */
function takesClosure($closure): int
{
    return $closure(123);
}

function takesIntByRef(int &$i): int
{
    return $i**2;
}

takesClosure("takesIntByRef");
Psalm output (using commit 85fe7e8):

No issues!

psalm-github-bot[bot] avatar Jul 21 '22 07:07 psalm-github-bot[bot]