Skip to content

Improved type of callable-array #9794

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.

Sign up for GitHub

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

Jump to bottom
Merged
orklah merged 6 commits into from
Oct 17, 2023
Merged

Improved type of callable-array #9794

orklah merged 6 commits into from
Oct 17, 2023

Conversation

@robchett
Copy link
Contributor

@robchett robchett commented May 16, 2023

BC: Removes TCallableArray & TCallableList in favour of TCallableKeyedArray
Closes #7778

Also improves the type of callable-array so that it is interpreted as a list{class-string|object, string} instead of non-empty-array<array-key, mixed>

@robchett robchett force-pushed the remove_TCallableArray_and_TCallableList branch 3 times, most recently from 92f0e59 to 6b4f84f Compare May 16, 2023 17:33
@robchett
Copy link
Contributor Author

robchett commented May 16, 2023

@orklah what would be preferred here

-'callable-array{0: class-string, 1: string}'
+'callable-array{class-string, string}'

As this is now a list of size 2 I'm not sure which better defines the shape.

@orklah
Copy link
Collaborator

orklah commented May 16, 2023

Semantically, the second syntax implies that the order is locked while the first doesn't (meaning [1=>'method', 0=>stdClass::class] would be valid, but it shouldn't). So the second syntax is best

@robchett robchett force-pushed the remove_TCallableArray_and_TCallableList branch from 6b4f84f to aa17161 Compare May 16, 2023 18:22
@orklah orklah added the release:feature The PR will be included in 'Features' section of the release notes label May 16, 2023
@orklah orklah added this to the Psalm 6 milestone May 16, 2023
@kkmuffme
Copy link
Contributor

kkmuffme commented Jun 9, 2023

The callable method is always non-falsy-string too, not just string might want to change that too? @robchett

@robchett robchett force-pushed the remove_TCallableArray_and_TCallableList branch from 354a018 to 99054d4 Compare June 17, 2023 15:43
@robchett robchett force-pushed the remove_TCallableArray_and_TCallableList branch from 99054d4 to 780375a Compare October 9, 2023 18:24
@robchett robchett force-pushed the remove_TCallableArray_and_TCallableList branch from 780375a to 889bdca Compare October 9, 2023 20:09
@orklah
Copy link
Collaborator

orklah commented Oct 16, 2023

I see TCallableArray being removed but I don't see TCallableList being removed as well, did I miss something?

robchett
robchett commented Oct 16, 2023
View reviewed changes
Copy link
Contributor Author

@robchett robchett left a comment

Choose a reason for hiding this comment

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

TCallableList was removed upstream - remove BC note about it

@robchett
Copy link
Contributor Author

robchett commented Oct 16, 2023

I rebased this branch last week as it was a long way behind. Looks like TCallableList was removed elsewhere upstream. Tracked that down to cca2767, removed the note about that in the BC note

orklah
orklah reviewed Oct 17, 2023
View reviewed changes
src/Psalm/Internal/Type/SimpleAssertionReconciler.php
) {
$callable_types[] = $type;
$redundant = false;
} elseif ($type instanceof TArray) {
Copy link
Collaborator

@orklah orklah Oct 17, 2023

Choose a reason for hiding this comment

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

Shouldn't this create a TCallableKeyedArray now that TCallableArray doesn't exists?

Copy link
Contributor Author

@robchett robchett Oct 17, 2023

Choose a reason for hiding this comment

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

Yes, that would make more sense and I don't remember why I made that change to begin with. It doesn't appear to effect any of the tests either way which

orklah
orklah reviewed Oct 17, 2023
View reviewed changes
src/Psalm/Internal/Type/TypeParser.php
!is_numeric($property_key)
|| ($had_optional && !$property_maybe_undefined)
|| $type === 'array'
|| $type === 'callable-array'
Copy link
Collaborator

@orklah orklah Oct 17, 2023

Choose a reason for hiding this comment

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

why remove this? TCallableKeyedArray still have a callable-array key, no?

Copy link
Contributor Author

@robchett robchett Oct 17, 2023

Choose a reason for hiding this comment

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

I have no good reason why it was removed. Now back and tests are unaffected,

@orklah
Copy link
Collaborator

orklah commented Oct 17, 2023

Thanks!

@orklah orklah merged commit 24168f6 into vimeo:master Oct 17, 2023
@weirdan weirdan mentioned this pull request Feb 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@orklah orklah orklah left review comments

Assignees

No one assigned

Labels

release:feature The PR will be included in 'Features' section of the release notes

Projects

None yet

Milestone

Psalm 6

Development

Successfully merging this pull request may close these issues.

Stricter typing of callable-array

3 participants

@robchett @orklah @kkmuffme