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

Enums: Implement callmap Enum::from, Enum::tryFrom and Enum::cases #6429

Closed
Tracked by #6425
weirdan opened this issue Sep 5, 2021 · 10 comments · Fixed by #7011
Closed
Tracked by #6425

Enums: Implement callmap Enum::from, Enum::tryFrom and Enum::cases #6429

weirdan opened this issue Sep 5, 2021 · 10 comments · Fixed by #7011

Comments

@weirdan
Copy link
Collaborator

weirdan commented Sep 5, 2021

https://psalm.dev/r/3a77b77825?php=8.1

https://wiki.php.net/rfc/enumerations

We need a stub for BackedEnum, like this:

interface BackedEnum<T of int|string> extends UnitEnum {
   public static function from(T $value): static;
   public static function tryFrom(T $value): ?static;
   public static function cases(): list<static>; // inherited from `UnitEnum`
}

And make Psalm infer that any backed enum implement corresponding type instance (BackedEnum<int> or BackedEnum<string>), based on backing type.

This is a problem, though, as we currently don't allow static methods to reference class type parameters.

@weirdan weirdan mentioned this issue Sep 5, 2021
15 tasks
@weirdan weirdan changed the title Implement callmap Enum::from, Enum::tryFrom and Enum::cases: psalm.dev/r/3a77b77825 Implement callmap Enum::from, Enum::tryFrom and Enum::cases Sep 5, 2021
@weirdan weirdan added this to the PHP 8.1 milestone Sep 5, 2021
@weirdan weirdan changed the title Implement callmap Enum::from, Enum::tryFrom and Enum::cases Enums: Implement callmap Enum::from, Enum::tryFrom and Enum::cases Sep 5, 2021
@vimeo vimeo deleted a comment from psalm-github-bot bot Sep 6, 2021
@weirdan
Copy link
Collaborator Author

weirdan commented Sep 6, 2021

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/3a77b77825
<?php

enum Status: int
{
    case FOO = 1;
    case BAR = 2;
}

// Returns Status::FOO
Status::from(1);

// Throw ValueError Exception
Status::from(3);

// Retunrs null
Status::tryFrom(3);

// Returns [Status::FOO, Status::BAR]
Status::cases();
Psalm output (using commit e3e6676):

ERROR: UndefinedMethod - 10:1 - Method Status::from does not exist

ERROR: UndefinedMethod - 13:1 - Method Status::from does not exist

ERROR: UndefinedMethod - 16:1 - Method Status::tryfrom does not exist

@weirdan
Copy link
Collaborator Author

weirdan commented Sep 6, 2021

@orklah, @muglug any thoughts on the point of static methods accessing class/interface type params? I know we had quite a few requests for that to be used for userland classes, but now we're facing the need for that for a core interface.

PHPStan allows that (https://phpstan.org/r/130ede95-e35a-4dc0-883b-76e6b2b0550f), so it's one of the points where tools have diverged.

@muglug
Copy link
Collaborator

muglug commented Sep 6, 2021

I had strong feelings about this when generics was a fairly new feature and where I thought this might be a source of bugs for users coming from languages with reified generics. Now I’m not really bothered — there are enough people around the community who have a basic understanding of generics and could point out common pitfalls, so it might be OK to relax.

@orklah
Copy link
Collaborator

orklah commented Sep 6, 2021

I think the limitation was added to avoid confusion with this kind of code: https://psalm.dev/r/ea77a57f0f. It makes sense for me that a class level template would be filled differently in different objects and as such, it's weird to see it in a static method.

But your example is correct and there is no good way to make it in Psalm.

In my head, the best way to solve that would be a tag that defines the template on method level: https://psalm.dev/r/9fdff9bdfe

But the added value compared to added complexity is discutable...

@psalm-github-bot
Copy link

I found these snippets:

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

/** @template T */
class Collection{
    /** @param T $a */
    public function __construct($a){
        
    }
    
    /** @return T */
    public static function get() {
        //magic happens?
    }
}
Psalm output (using commit e3e6676):

ERROR: InvalidReturnType - 10:17 - No return statements were found for method Collection::get but return type 'T' was expected
https://psalm.dev/r/9fdff9bdfe
<?php declare(strict_types = 1);


class C
{
	/** 
     * @template T
	 * @param T $p
	 * @return T
	 */
	public static function i($p)
	{
		return $p;
	}
}

class D extends C {
    /**
     * @template-define int T
     */
	public static function i($p): int {
		return $p; 
	}
}
Psalm output (using commit e3e6676):

INFO: MixedReturnTypeCoercion - 22:10 - The type 'T:fn-c::i as mixed' is more general than the declared return type 'int' for D::i

INFO: MixedReturnTypeCoercion - 21:32 - The declared return type 'int' for D::i is more specific than the inferred return type 'T:fn-c::i as mixed'

@orklah
Copy link
Collaborator

orklah commented Oct 11, 2021

related to #6017

@zmitic
Copy link

zmitic commented Oct 29, 2021

@weirdan

This is a problem, though, as we currently don't allow static methods to reference class type parameters.

Sorry if this is a dumb question but is this generic for static methods really necessary when there are only 2 types ever to be allowed?

//stubs
interface BackedEnum
{
   // nothing here, just a marker
}

// copied from PHPStorm
interface IntBackedEnum extends BackedEnum  
{
    public string $name;

    /** @return static[] */
    public static function cases(): array;

    public int $value;

    public static function from(int $value): static;

    public static function tryFrom(int $value): ?static;
}

the same for StringBackedEnum.


Sure it does duplicate methods but it is not something that will ever expend, right?

@ricardoboss
Copy link
Contributor

@weirdan shouldn't these methods also be considered #[Pure]?

@weirdan
Copy link
Collaborator Author

weirdan commented Dec 7, 2021

They should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants