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

Forbid template annotation on closures #5499

Merged
merged 1 commit into from Mar 29, 2021

Conversation

weirdan
Copy link
Collaborator

@weirdan weirdan commented Mar 29, 2021

They don't work properly anyway.

Fixes #5472

@weirdan
Copy link
Collaborator Author

weirdan commented Mar 29, 2021

Apparently people use them.

/cc: @azjezz

@azjezz
Copy link
Contributor

azjezz commented Mar 29, 2021

I'm pretty sure hack does support generics in closure ( () => {} syntax, not php closure syntax since that is "deprecated", hence why generics weren't added there ).

checked again, hack allows generic closures but implicitly, e.g:

function main(): void {
  $c = ($v, $val): bool ==> {
    foreach($v as $t) {
      if ($t === $val) { return true; }
    }

    return false;
  };

  $c(vec[1, 2, 3], 3);
}

the hack type checker doesn't complain about anything for that example, with psalm, this would be an error because of the missing parameter types.

@weirdan weirdan marked this pull request as draft March 29, 2021 05:50
@azjezz
Copy link
Contributor

azjezz commented Mar 29, 2021

so technically, the code that is currently failing in PSL, would pass in Hack if i remove all parameter types, as the type checker would get that it is generic ( even tho not explicitly ), but this would result in an error in psalm.

i personally like having support for generic in closures ( even variables/constants, see #3963 )

@weirdan
Copy link
Collaborator Author

weirdan commented Mar 29, 2021

How do you know it's actually generic and not just untyped?

@azjezz
Copy link
Contributor

azjezz commented Mar 29, 2021

It's not untyped, hack detects the type automatically depending on the usage of the closure.

e.g:
a

if we add $c(vec['a', 'b', 'c'], 'c'); under $c(vec[1, 2, 3], 3);, it becomes:

b

and it can't be un-typed since hack doesn't allow thing to be un-typed ( at least at the strictest level )


so if psalm is to remove generics, it needs to add the ability to automatically detect type of callables without having to specify them based on usage.

@azjezz
Copy link
Contributor

azjezz commented Mar 29, 2021

hm, actually, looking at it, i think one of the failed cases in psl can be fixed without using generics in closure, however, i still need to specify the type: https://psalm.dev/r/65b9344e48

here's a re-implementation with hack that passes:

image

@psalm-github-bot
Copy link

psalm-github-bot bot commented Mar 29, 2021

I found these snippets:

https://psalm.dev/r/65b9344e48
<?php


/**
 * Performs left-to-right function composition.
 *
 * Example:
 *
 *      $greet = Fun\pipe(
 *          static fn(string $value): string => 'Hello' . $value,
 *          static fn(string $value): string => $value . '!',
 *          static fn(string $value): string => '¡' . $value
 *      );
 *
 *      $greet('World')
 *      => Str('¡Hello World!');
 *
 *      $greet('Jim')
 *      => Str('¡Hello Jim!');
 *
 * @template T
 *
 * @param callable(T): T ...$stages
 *
 * @return callable(T): T
 *
 * @pure
 */
function pipe(callable ...$stages): callable
{
    return static fn ($input) => reduce(
        $stages,
        /**
         * @param T $input
         * @param (callable(T): T) $next
         *
         * @return T
         */
        static fn ($input, callable $next) => $next($input),
        $input
    );
}


/**
 * Reduce iterable using a function.
 *
 * The reduction function is passed an accumulator value and the current
 * iterator value and returns a new accumulator. The accumulator is initialized
 * to $initial.
 *
 * Examples:
 *
 *      Iter\reduce(Iter\range(1, 5), fn($accumulator, $value) => $accumulator + $value, 0)
 *      => 15
 *
 *      Iter\reduce(Iter\range(1, 5), fn($accumulator, $value) => $accumulator * $value, 1)
 *      => 120
 *
 * @template Tk
 * @template Tv
 * @template Ts
 *
 * @param iterable<Tk, Tv> $iterable
 * @param (callable(Ts, Tv): Ts) $function
 * @param Ts $initial
 *
 * @return Ts
 */
function reduce(iterable $iterable, callable $function, $initial)
{
    $accumulator = $initial;
    foreach ($iterable as $v) {
        $accumulator = $function($accumulator, $v);
    }

    return $accumulator;
}
Psalm output (using commit d57dde0):

No issues!

@azjezz
Copy link
Contributor

azjezz commented Mar 29, 2021

okay, all PSL issues have been fixed in azjezz/psl#176, i think the "detect callable type based on usage" feature can be tracked in another issue.

( @muglug if you are update the PSL fork, note that psalm configs are now in tools/psalm/psalm.xml, and not the source directory anymore, see azjezz/psl#174 )

@muglug
Copy link
Collaborator

muglug commented Mar 29, 2021

@azjezz @weirdan I've updated my fork!

They don't work properly anyway.

Fixes vimeo#5472
@weirdan weirdan marked this pull request as ready for review March 29, 2021 15:06
@muglug muglug merged commit 6bd7f5b into vimeo:master Mar 29, 2021
@muglug
Copy link
Collaborator

muglug commented Mar 29, 2021

Thanks!

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

Successfully merging this pull request may close these issues.

Forbid @template annotation on closures
3 participants