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

Fix mb_convert_encoding to accept any array. #6910

Merged
merged 5 commits into from
Nov 14, 2021
Merged

Conversation

rarila
Copy link
Contributor

@rarila rarila commented Nov 13, 2021

Fixes #6886

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Nov 13, 2021
@orklah
Copy link
Collaborator

orklah commented Nov 13, 2021

Thanks! Do you know if the shape of the array is preserved?

If it is, we could create a stub in order to make the array shape pass through the function.

something like

/**
 * @template T of array|string
 * @param T $string
 * @return (T is array ? T : string)
 */

should do the trick

@rarila
Copy link
Contributor Author

rarila commented Nov 13, 2021

Running tests locally at the moment…
............................................................. 610 / 6499 ( 9%)
two hours later… :-))

Hmm, no, it isn’t. Look at the last example in the issue. String keys are translated, objects/recursively referenced values are replaced.

So I guess, no

@weirdan
Copy link
Collaborator

weirdan commented Nov 13, 2021

two hours later… :-))

Tests shouldn't be that slow, even with Xdebug enabled (I'd suggest to disable it with, e.g. https://packagist.org/packages/weirdan/run-without-xdebug though).

@orklah
Copy link
Collaborator

orklah commented Nov 13, 2021

String keys are translated, objects/recursively referenced values are replaced.

Yeah, string are transformed, but the general shape of the array is kept, no? I mean, if the array was array<int, string> it will stay a array<int, string> right?

@weirdan
Copy link
Collaborator

weirdan commented Nov 13, 2021

I mean, if the array was array<int, string> it will stay a array<int, string> right?

Yeah, but array{€:string} does not stay that way: https://3v4l.org/djRjL, and you wouldn't be able to express that with templates.

@rarila
Copy link
Contributor Author

rarila commented Nov 13, 2021

@weirdan Yep.

@weirdan
Copy link
Collaborator

weirdan commented Nov 13, 2021

Tests shouldn't be that slow

It takes about 5 minutes here, using php-noxdebug vendor/bin/paratest command.

@rarila
Copy link
Contributor Author

rarila commented Nov 13, 2021

@weirdan gonna try, but I’m already at 44% now – almost done :-)

@orklah
Copy link
Collaborator

orklah commented Nov 13, 2021

Oh yeah, forgot about literals in array-shapes.

Guess it would need a return type provider in order to recursively strip any string literal.

It may be worth it one day, returning array is a sure way to trigger a lot of MixedAssignment just by using the returned array

@rarila
Copy link
Contributor Author

rarila commented Nov 13, 2021

Probably

/**
 * @template T of array|string
 * @param T $string
 * @return (T is array<int, string> ? array<int, string> : 
 *          T is array              ? array : 
 *                                  string)
 */

Would help for most cases

@orklah
Copy link
Collaborator

orklah commented Nov 13, 2021

Yeah, it would, Mind adding that to the stubs?

@rarila
Copy link
Contributor Author

rarila commented Nov 13, 2021

@orklah ok

@orklah
Copy link
Collaborator

orklah commented Nov 13, 2021

Note that you have to make a change to CallMap_80 and possibly CallMap_historical for your PR to pass the CI. More is explained here: https://psalm.dev/docs/contributing/editing_callmaps/

@rarila
Copy link
Contributor Author

rarila commented Nov 13, 2021

@orklah CoreGenericFunctions.phpstub? Just add it at the end?

@rarila
Copy link
Contributor Author

rarila commented Nov 13, 2021

@orklah

How do you like this format:

/**
 * @template T of array|string
 * @param T $string
 * @return (T is array<int, string> ? array<int, string> : 
 *          T is array              ? array : 
 *          T is string             ? string :
 *                                    never)
 */

I like it that way because it lists all possible options and is quite readable that way.

@orklah
Copy link
Collaborator

orklah commented Nov 13, 2021

Please don't add never at the end. Listing the allowed params is enough to make Psalm emit an issue. Adding never will just create a whole new set of weird errors that will be irrelevant

@rarila
Copy link
Contributor Author

rarila commented Nov 13, 2021

@orklah ok.

Looking at the other callmaps now…

If I see it right, I'll have to remove the array version from CallMap_historical.php, add it to 'added' CallMap_72_delta.php?!

@orklah
Copy link
Collaborator

orklah commented Nov 13, 2021

nah, you have to change the signature in CallMap_80 "new" to match with the one in your CallMap file and then, if needed, change the CallMap_historical to match with the CallMap_80 "old" (we start from CallMap and then process each delta file from newer to older untli CallMap_historical)

@rarila
Copy link
Contributor Author

rarila commented Nov 13, 2021

I think I have it, running tests…

@@ -34,6 +34,7 @@
'ldap_exop_whoami' => ['string|false', 'ldap'=>'resource'],
'ldap_parse_exop' => ['bool', 'ldap'=>'resource', 'result'=>'resource', '&w_response_data='=>'string', '&w_response_oid='=>'string'],
'mb_chr' => ['string|false', 'codepoint'=>'int', 'encoding='=>'string'],
'mb_convert_encoding\'1' => ['array', 'string'=>'array', 'to_encoding'=>'string', 'from_encoding='=>'mixed'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, nice catch! I didn't know arrays were introduced in 7.2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least the manual says so :-)

* @template T of array|string
* @param T $string
* @return (T is array<int, string> ? array<int, string> : T is array ? array : string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should add |false everywhere in the return types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch. But only at the string return.

I still wonder what happens, when you add a string that triggers returning false for a string into an array…

@orklah
Copy link
Collaborator

orklah commented Nov 14, 2021

Can you add a @psalm-ignore-falsable-return on the stub? It will allow people with the config disabled to not handle the false return without getting errors

@rarila
Copy link
Contributor Author

rarila commented Nov 14, 2021

@orklah Here you are :-)

@orklah orklah merged commit 16f0db6 into vimeo:master Nov 14, 2021
@orklah
Copy link
Collaborator

orklah commented Nov 14, 2021

Thank you! That's great!

@rarila
Copy link
Contributor Author

rarila commented Nov 14, 2021

No, I have to thank for this great tool!

@rarila rarila deleted the issue6886 branch November 14, 2021 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Various problems with mb_convert_encoding
3 participants