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

How to properly annotate @param class-string<T>|null as fallback to object? #6638

Closed
donquixote opened this issue Oct 10, 2021 · 14 comments
Closed

Comments

@donquixote
Copy link
Contributor

I have a function/method with an optional @param class-string<T>|null $class parameter.
The function should return T or T[] if $class is provided, or object if $class is NULL.

https://psalm.dev/r/bea75ab401

Psalm output (using commit 5134a92):

INFO: MixedAssignment - 26:46 - Unable to determine the type that $item is being assigned to

The MixedAssignment only happens in foreach() from @return T[], not on the single-value case.
However, in both cases the return value is resolved as empty instead of object.

(heads up: I might get my hands dirty with development at some point, but I need to make time for it. For now I am simply reporting what I find)

@psalm-github-bot
Copy link

I found these snippets:

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

abstract class C {
    
    /**
     * @template T as object
     *
     * @param class-string<T>|null $class
     *
     * @return T
     */
    abstract public function createInstance(string $class = NULL);
    
    /**
     * @template T as object
     *
     * @param class-string<T>|null $class
     *
     * @return T[]
     */
    abstract public function createInstances(string $class = NULL): array;

    public function f(): void {
        $single = $this->createInstance();  // No MixedAssignment warning.
        unset($single);
        foreach ($this->createInstances() as $item) {  // MixedAssignment on 'as $item'.
            unset($item);
        }
    }

}
Psalm output (using commit 5134a92):

INFO: MixedAssignment - 26:46 - Unable to determine the type that $item is being assigned to

@donquixote
Copy link
Contributor Author

donquixote commented Oct 10, 2021

I could do @return T|object, but then psalm would have to assume that even if $class is provided, it could still return any object.

I could make the param default to string $class = 'object', but that would make the implementation more confusing. Also it creates a warning "ERROR: InvalidParamDefault - 17:15 - Default value type "object" for argument 1 of method C::createInstances does not match the given type class-string<T:fn-c::createinstances as object>"

Adding or removing the |null from the @param class-string<T>|null does not seem to make a difference.

@donquixote
Copy link
Contributor Author

The current behavior kind of makes sense, because as is not meant as a fallback, but rather as a constraint.
Still, a fallback behavior would be useful, and as is the closest we have right now. Perhaps we need a different syntax?

@weirdan
Copy link
Collaborator

weirdan commented Oct 12, 2021

@psalm-github-bot
Copy link

I found these snippets:

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

abstract class C {
    
    /**
     * @template T as object
     *
     * @param class-string<T>|null $class
     *
     * @return ($class is null ? object[] : T[])
     */
    abstract public function createInstances(string $class = NULL): array;

    public function f(): void {
        foreach ($this->createInstances() as $item) {  // MixedAssignment on 'as $item'.
            unset($item);
        }
    }

}
Psalm output (using commit 0379b03):

No issues!

@donquixote
Copy link
Contributor Author

Nice! Yes, exactly this.
I wonder if PhpStorm understands this.

@weirdan
Copy link
Collaborator

weirdan commented Oct 12, 2021

I don't think it does (I wasn't able to find a corresponding ticket on youtrack), neither does PHPStan (see phpstan/phpstan#3853).

With that said, it's up to those tools to catch up.

@weirdan weirdan closed this as completed Oct 12, 2021
@donquixote
Copy link
Contributor Author

donquixote commented Oct 31, 2021

@weirdan I noticed that even the built-in stubs in psalm don't follow this syntax.
They would have to be like this:
master...donquixote:patch-1

In ReflectionMethod:

      * @since 8.0
      * @template TClass as object
      * @param class-string<TClass>|null $name
-     * @return array<ReflectionAttribute<TClass>>
+     * @return array<ReflectionAttribute<$name is null ? object : TClass>>
      */
     public function getAttributes(?string $name = null, int $flags = 0): array {}

I think it would be simpler to just treat the as object in @template T as object as a fallback type, or introduce a new syntax for this purpose, instead of introducing the conditional syntax in all the stubs.

@orklah
Copy link
Collaborator

orklah commented Oct 31, 2021

Please read https://psalm.dev/docs/annotating_code/type_syntax/conditional_types/ again, your syntax is not valid

@donquixote
Copy link
Contributor Author

donquixote commented Oct 31, 2021

@orklah I noticed one mistake: I swapped object and TClass. Corrected in my message, but not in the linked diff.

Otherwise, my code is based on the comment by @weirdan.

The docs suggest that I can only use template types in the condition, not parameter names.
E.g. T is null but not $name is null. But weirdan's post suggests otherwise, and it works.

The docs also say I need to use parentheses around the ternary expression. But it works fine without.
https://psalm.dev/r/7b0ec389fa

Seems more like a case of incomplete docs. Or perhaps I am relying on undocumented features that might be removed in a future version?

@psalm-github-bot
Copy link

I found these snippets:

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

interface I {
    
  /**
   * Returns an array of constant attributes.
   *
   * @template T as object
   *
   * @param class-string<T>|null $name
   *   Name of an attribute class.
   *
   * @return list<\ReflectionAttribute<T>>
   * @psalm-return list<\ReflectionAttribute<$name is null ? object : T>>
   *
   * @noinspection PhpUndefinedClassInspection
   */
  public function getAttributes(string $name = null): array;
}

interface J {
    function f(): void;
}

function f(I $obj): void {
    foreach ($obj->getAttributes(J::class) as $attr) {
    	$attr->newInstance()->f();
    }
}
Psalm output (using commit 7f14d09):

No issues!

@orklah
Copy link
Collaborator

orklah commented Oct 31, 2021

Yeah, I was referring to the parentheses at least because I thought conditional inside the generic type was not working either.

Turns out it does. I'm quite surprised, I never saw that before. I think you found some feature that was maybe not meant to work.

I think it'd be safer to use something like
($name is null ? array<ReflectionAttribute<object> : array<ReflectionAttribute<TClass>)

@donquixote
Copy link
Contributor Author

@orklah The docs suggest that some kind of nesting is supported..
The nesting example in the docs uses multiple layers of ternaries. But one could also expect that nesting ternaries inside other type expressions should work.

Turns out it does. I'm quite surprised, I never saw that before. I think you found some feature that was maybe not meant to work.

Perhaps create test cases for this and then make sure it continues to work?

@orklah
Copy link
Collaborator

orklah commented Oct 31, 2021

Perhaps create test cases for this and then make sure it continues to work?

Psalm is very complicated and its author is not there anymore to maintain it. I won't do anything to break it, but if it's just a different syntax to express the same thing, I'm not sure it's a good idea to add constraints like that. Especially so if it was not by design

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