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

Psalm 5.4 upgrade #287

Merged
merged 50 commits into from
Jan 13, 2023
Merged

Psalm 5.4 upgrade #287

merged 50 commits into from
Jan 13, 2023

Conversation

drupol
Copy link
Collaborator

@drupol drupol commented Dec 26, 2022

This PR:

  • Fix issues introduced by Psalm 5.4
  • Has static analysis tests (psalm, phpstan)
  • Needs PSalm > 5.4.0 to work (use dev version in the meantime)

@what-the-diff
Copy link

what-the-diff bot commented Dec 26, 2022

  • Added a new function to generate random letters

  • Changed the type of collection from CollectionInterface<int, int> to CollectionInterface<int, string|null> in scanLeft_checkListStringWithNull()

  • Added a new function squash_checkList2 and squash_checkStringList2.
    This is because the original functions were not able to handle collections with non-int keys.

    The reason for this was that we had an explicit typehint of int in our collection interface, which meant
    that psalm would only allow us to pass integers as keys.

@drupol drupol force-pushed the psalm-5.4-upgrade branch 12 times, most recently from 08e60cc to 5dc98cf Compare January 2, 2023 21:41
@drupol
Copy link
Collaborator Author

drupol commented Jan 2, 2023

This PR needs to be merged once the upcoming version of PSalm is out (5.4.1).

@drupol drupol force-pushed the psalm-5.4-upgrade branch 6 times, most recently from 2f2083d to 0bbe180 Compare January 3, 2023 20:24
Copy link
Collaborator

@AlexandruGG AlexandruGG left a comment

Choose a reason for hiding this comment

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

I think we need to be careful not to make things way too restrictive -> I see lots of changes needed in the static analysis checks - these are all potential changes users would need to make in their own codebases.

If some of changes are unavoidable simply due to Psalm 5.4 then we can perhaps use a baseline and introduce them more gradually? Would be easier if we would only do it for a set of operations at a time so we can carefully think about how the change affects their usage.

composer.json Outdated Show resolved Hide resolved
src/Contract/Operation/Unfoldable.php Outdated Show resolved Hide resolved
* @return Collection<mixed, mixed>
* @return Collection<TKey, T>
Copy link
Collaborator

Choose a reason for hiding this comment

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

not too sure about this change -> it's not a given that the elements in the unwrapped collection will be all of the same type 🤔

Collection::fromIterable([['a'], [1], [true]])
    ->unwrap(); // ['a', 1, true]

unless static analysis is able to interpret the template T as the union string|int|bool in this case. I'm not sure but you could test this with a static analysis check

{
}
/**
* @param CollectionInterface<string, string> $collection
* @psalm-param CollectionInterface<int<0, 2>, 2|3|list{1}> $collection
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not good, and it shows that the new type hints have made unwrap too restrictive and incorrect in this case

Collection::fromIterable([[1], 2, 3])->unwrap() 
// will result in Collection(1, 2, 3) aka `Collection<int, int>`, but the type hints think the resulting types are the same 
// as were passed in the original collection

* @return Collection<mixed, mixed>
* @return Collection<TKey, T>
Copy link
Collaborator

Choose a reason for hiding this comment

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

same issue here as with the unwrap

src/Operation/Wrap.php Outdated Show resolved Hide resolved
@drupol drupol force-pushed the psalm-5.4-upgrade branch 6 times, most recently from 7bbd7ed to 5a9fa80 Compare January 5, 2023 07:58
@drupol
Copy link
Collaborator Author

drupol commented Jan 6, 2023

@AlexandruGG All good for you now?

@drupol drupol force-pushed the psalm-5.4-upgrade branch 2 times, most recently from 341f72b to e1f174e Compare January 6, 2023 20:55
@drupol drupol force-pushed the psalm-5.4-upgrade branch from a522160 to f355cb3 Compare January 13, 2023 09:32
@drupol
Copy link
Collaborator Author

drupol commented Jan 13, 2023

@AlexandruGG Good for you?

Copy link
Collaborator

@AlexandruGG AlexandruGG left a comment

Choose a reason for hiding this comment

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

Thanks, I know this took lots of work!

@drupol drupol merged commit 117351d into master Jan 13, 2023
@drupol drupol deleted the psalm-5.4-upgrade branch January 13, 2023 09:56
@drupol
Copy link
Collaborator Author

drupol commented Jan 13, 2023

Thanks! Indeed it was painful !

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.

2 participants