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

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

Open
azjezz opened this issue Jul 3, 2022 · 14 comments

Comments

@azjezz
Copy link
Contributor

azjezz commented Jul 3, 2022

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.

@psalm-github-bot
Copy link

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!

@azjezz
Copy link
Contributor Author

azjezz commented Jul 3, 2022

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.

@psalm-github-bot
Copy link

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

@azjezz
Copy link
Contributor Author

azjezz commented Jul 3, 2022

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.

@psalm-github-bot
Copy link

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!

@AndrolGenhald
Copy link
Collaborator

Seems like the same sort of problem as #8116.

@azjezz
Copy link
Contributor Author

azjezz commented Jul 3, 2022

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 ^^"

@someniatko
Copy link
Contributor

someniatko commented Jul 3, 2022

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?

@psalm-github-bot
Copy link

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

@azjezz
Copy link
Contributor Author

azjezz commented Jul 3, 2022

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

@psalm-github-bot
Copy link

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

@someniatko
Copy link
Contributor

someniatko commented Jul 21, 2022

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

@AndrolGenhald
Copy link
Collaborator

@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...

@psalm-github-bot
Copy link

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!

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

No branches or pull requests

3 participants