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

Improve return types for psalm #321

Merged
merged 1 commit into from
Nov 25, 2023

Conversation

Matth--
Copy link
Contributor

@Matth-- Matth-- commented Nov 23, 2023

Psalm cannot always infer the types when not defining the CollectionInterface<Key, Value> type. This makes sure that psalm is aware of the implemented interface and the Collection class.

This PR:

Still open for debate as it changes the static constructor types.

@Matth-- Matth-- requested a review from drupol as a code owner November 23, 2023 19:58
@drupol
Copy link
Collaborator

drupol commented Nov 23, 2023

Psalm 5.16 has been released yesterday and adds many new issues. I guess we will have to add them to the baseline.

@drupol
Copy link
Collaborator

drupol commented Nov 23, 2023

Can you please run:

  1. Fix CS: ./vendor/bin/grumphp run --tasks=phpcsfixer
  2. Commit
  3. Push

Psalm cannot always infer the types when not defining the CollectionInterface<Key, Value>
type. This makes sure that psalm is aware of the implemented interface and the Collection
class.
Copy link
Collaborator

@drupol drupol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! Much appreciated 👍

There's one missing thing. The CollectionDecorator class also needs to be updated with the same changes, and then it's good to go.

@Matth--
Copy link
Contributor Author

Matth-- commented Nov 24, 2023

@drupol Missed that one as I haven't used it before 😅 . Will update soon!

@drupol
Copy link
Collaborator

drupol commented Nov 24, 2023

Merci ❤️

@Matth--
Copy link
Contributor Author

Matth-- commented Nov 24, 2023

Tried to implement something similar for the CollectionDecorator but now phpstan is complaining. I will need to do more investigation to be able to fix this.

@drupol
Copy link
Collaborator

drupol commented Nov 24, 2023

Oh curious, what are the issues?

@Matth--
Copy link
Contributor Author

Matth-- commented Nov 24, 2023

After changing the fromIterable method to this in the CollectionDecorator class (so even changing the static to CollectionInterface

  /**
   * @template UKey
   * @template U
   *
   * @param iterable<UKey, U> $iterable
   *
   * @return CollectionInterface<UKey, U>&static<UKey, U>
   */
  public static function fromIterable(iterable $iterable): CollectionInterface
  {
      return new static(Collection::fromIterable($iterable));
  }

I get the psalm error:

ERROR: MixedArgument
at /path/collection/tests/static-analysis/fromIterableAssociateAll.php:31:11
Argument 1 of checklist cannot be mixed, expecting array<string, int> (see https://psalm.dev/030)
checklist(CollectionDecorator::fromIterable(fromIterableAssociateAll::cases())->associate(
    static fn (int $_, fromIterableAssociateAll $item): int => $item->value,
    static fn (fromIterableAssociateAll $item, int $_): string => $item->name,
)->flip()->all(false));

And phpstan spits out:

 ------ ----------------------------------------------------------------------- 
  Line   tests/static-analysis/fromIterableAssociateAll.php                     
 ------ ----------------------------------------------------------------------- 
  31     Parameter #1 $array of function checklist expects array<string, int>,  
         array<int, fromIterableAssociateAll> given.                            
 ------ ----------------------------------------------------------------------- 

@Matth-- Matth-- closed this Nov 24, 2023
@drupol
Copy link
Collaborator

drupol commented Nov 24, 2023

OK fair enough, let's not update CollectionDecorator in this PR then.

Was the closing of the PR intentional ?

@Matth-- Matth-- reopened this Nov 24, 2023
@Matth--
Copy link
Contributor Author

Matth-- commented Nov 24, 2023

Nope sorry :)

@drupol
Copy link
Collaborator

drupol commented Nov 24, 2023

OK cool ! :) Let me know when it's done for you and I'll take care of the rest.

@drupol
Copy link
Collaborator

drupol commented Nov 24, 2023

@Matth-- Is it ready to be merged for you?

@Matth--
Copy link
Contributor Author

Matth-- commented Nov 25, 2023

I would say yes for now. 🚀

I'd like to find the root of the problem when i have some more time. Might have to look at the psalm package then.

@drupol drupol merged commit 81fa92d into loophp:master Nov 25, 2023
22 of 23 checks passed
@drupol
Copy link
Collaborator

drupol commented Nov 25, 2023

OK, let's do that. Let me know if I can do further things to improve this project!

Thanks again for your contribution, keep them coming 👍

@Matth-- Matth-- deleted the fix/psalm-type-inference branch December 13, 2023 12:11
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.

Palm cannot infer types when using some operations
2 participants