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

Static analysis: Update Map operation. #192

Closed
wants to merge 1 commit into from
Closed

Conversation

drupol
Copy link
Collaborator

@drupol drupol commented Aug 27, 2021

This PR:

  • Fix Map operation
  • Has static analysis tests (psalm, phpstan)
  • Is an experimental thing

Psalm snippets:

In the end, I'm here (https://psalm.dev/r/33088712ff), there's only one issue remaining.

I asked @weirdan and he proposed:

I'm still experimenting on the best way to fix this issue.

@@ -192,7 +193,7 @@
* @template-extends Lastable<TKey, T>
* @template-extends Limitable<TKey, T>
* @template-extends Linesable<TKey, T>
* @template-extends Mapable<TKey, T>
* @template-extends Mapable<TKey, T, V>
Copy link
Collaborator

Choose a reason for hiding this comment

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

seeing the errors on this I'm thinking it's not the right way.

Looking at the Doctrine ArrayCollection the template variable is defined on the method.

I played with the Psalm example a bit more and came up with another version which works while keeping the template param at the same level:
https://psalm.dev/r/e840743851
https://phpstan.org/r/5166ee20-1783-4d1d-93f6-91e4ac3570b4

Note the callback is passed from the invoke method. I think the issue with the initial one might be that the template is defined at a different level from where the callback is passed, so it "can't know" the return type of the callback. This is just my assumption, I can't claim to understand this fully 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Going to analyze this later in the day for sure!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Voila, I updated the top message with the relevant PSalm snippets.

I'm still searching for the best way to implement this.

@github-actions
Copy link

github-actions bot commented Sep 4, 2021

Since this pull request has not had any activity within the last 5 days, I have marked it as stale.
I will close it if no further activity occurs within the next 5 days.

@github-actions github-actions bot added the stale label Sep 4, 2021
@drupol drupol removed the stale label Sep 5, 2021
@github-actions
Copy link

Since this pull request has not had any activity within the last 5 days, I have marked it as stale.
I will close it if no further activity occurs within the next 5 days.

@github-actions github-actions bot added the stale label Sep 10, 2021
@drupol drupol removed the stale label Sep 13, 2021
@github-actions
Copy link

Since this pull request has not had any activity within the last 5 days, I have marked it as stale.
I will close it if no further activity occurs within the next 5 days.

@github-actions github-actions bot added the stale label Sep 19, 2021
@drupol drupol removed the stale label Sep 19, 2021
@github-actions
Copy link

Since this pull request has not had any activity within the last 5 days, I have marked it as stale.
I will close it if no further activity occurs within the next 5 days.

@github-actions github-actions bot added the stale label Sep 25, 2021
@github-actions github-actions bot closed this Sep 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants