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

Fix psalm annotation on Enum::from #146

Merged
merged 1 commit into from
Jul 4, 2021

Conversation

kamazee
Copy link
Contributor

@kamazee kamazee commented Jul 4, 2021

  • @return static is accurate enough, so it doesn't need a clarification for psalm. static<T> triggers false-positive alerts from static analyzers and seems to be just a mistake;
  • enum entry's template type T is more precise as a parameter than mixed: it might help catch more errors when using Enum::from.

@mnapoli
Copy link
Member

mnapoli commented Jul 4, 2021

Thanks!

@mnapoli mnapoli merged commit 8bef486 into myclabs:master Jul 4, 2021
@kamazee
Copy link
Contributor Author

kamazee commented Jul 4, 2021

This was a bit bold: the fix of return type was correct but I shouldn't have changed the param. Ironically, phpstan picks it up correctly (despite it's psalm- prefixed) but psalm itself doesn't, likely because of vimeo/psalm#2571 .

Will send another PR in a moment. Sorry about that :(

@Ocramius
Copy link
Contributor

Ocramius commented Jul 4, 2021

triggers false-positive alerts from static analyzers and seems to be just a mistake;

FYI, no mistake there: TheActualEnum<string|int> was intentional.

I did not finish up my work in #143 though, sorry

@kamazee
Copy link
Contributor Author

kamazee commented Jul 4, 2021

@Ocramius I'll double check tomorrow but it didn't work for me (either psalm or phpstan complained). So, I thought it would be enough to set return type to TheActualEnum which already stated that it extended Enum<int|string>.

I'll follow up when I recheck it.

@kamazee
Copy link
Contributor Author

kamazee commented Jul 5, 2021

On the following code:

<?php

/**
 * @psalm-immutable
 * @psalm-template T
 * @psalm-consistent-constructor
 */
class Enum {
    /**
     * Cache of instances of the Enum class
     *
     * @var array
     * @psalm-var array<class-string, array<string, static>>
     */
    protected static $instances = [];

    /**
     * @param mixed $value
     * @return static
     * @psalm-return static<T>
     */
    public static function from($value): self
    {
        throw new RuntimeException();
    }
}

/**
 * @method static self RED()
 * @psalm-immutable
 * @template-extends Enum<string>
 */
class Colours extends Enum {
    protected const RED = '#f00';
}

function foo(Colours $entry): void {
    var_export($entry);
}

foo(Colours::from('red'));

phpstan chokes on static<T> and is okay with just static that extends Enum<string>:
https://phpstan.org/r/7b434d16-9edc-4d44-8304-14f152cb15eb

Psalm detects no issues in such code but shows type of the from result as Colours<empty> which doesn't look right to me:
https://psalm.dev/r/5e1d95b337 (hover a pointer over the last line of the snippet)

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.

3 participants