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

Incomplete return type for mb_split() function #7421

Closed
ryanhobrock opened this issue Jan 18, 2022 · 11 comments · Fixed by #7432
Closed

Incomplete return type for mb_split() function #7421

ryanhobrock opened this issue Jan 18, 2022 · 11 comments · Fixed by #7432
Assignees
Labels
bug easy problems Issues that can be fixed without background knowledge of Psalm good first issue Help wanted internal stubs/callmap

Comments

@ryanhobrock
Copy link

Function mb_split() should have a return type of array|false per the PHP documentation. The Psalm call maps only have array leading to a TypeDoesNotContainType "list<string> does not contain false" error when type checking. See https://psalm.dev/r/0b69779c9f to reproduce the issue.

Changing the call maps from:

'mb_split' => ['list<string>', 'pattern'=>'string', 'string'=>'string', 'limit='=>'int'],

to:

'mb_split' => ['list<string>|false', 'pattern'=>'string', 'string'=>'string', 'limit='=>'int'],

should resolve it.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/0b69779c9f
<?php

$text = 'Hello world!';

$split = mb_split(' ', $text);

if ($split === false) {
    // Do something
}
Psalm output (using commit 477c011):

ERROR: TypeDoesNotContainType - 7:5 - list<string> does not contain false

@orklah
Copy link
Collaborator

orklah commented Jan 18, 2022

Do you have a code that would actually make mb_split returns false?

Sometimes, when we're sure Psalm will catch every case that would make a function errors, we allow ourselves to not include the error return for better inference, but I'm not sure this is the case here...

@AndrolGenhald
Copy link
Collaborator

AndrolGenhald commented Jan 18, 2022

@orklah I spent a minute looking at that a couple hours ago because I had the same thought, there's one case that probably shouldn't happen, but there are also 2 valid cases: https://3v4l.org/dN6pD https://psalm.dev/r/57fd317561

@orklah
Copy link
Collaborator

orklah commented Jan 18, 2022

Cool, thanks for confirmation. @ryanhobrock you correctly identified what needed to change. Small caveat, you need to change this in CallMap and CallMap_historical.

Can you push a PR?

@orklah orklah added bug easy problems Issues that can be fixed without background knowledge of Psalm good first issue Help wanted internal stubs/callmap labels Jan 18, 2022
@RishiKumarRay
Copy link
Contributor

@orklah I would to raise PR for these changes, can you please assign me

@orklah
Copy link
Collaborator

orklah commented Jan 19, 2022

With pleasure :)

@RishiKumarRay
Copy link
Contributor

Hey @orklah in which directory can i Find the files CallMap and CallMap_historical as you said the changes are to be made in this 2 files?

@orklah
Copy link
Collaborator

orklah commented Jan 19, 2022

@RishiKumarRay
Copy link
Contributor

@orklah cool ! I found it , Thanks a lot , will be generating a PR soon

@RishiKumarRay
Copy link
Contributor

RishiKumarRay commented Jan 19, 2022

@orklah please do the review the PR #7432 , Thank you

@ryanhobrock
Copy link
Author

Thank you @orklah, @AndrolGenhald, and @RishiKumarRay for the quick work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug easy problems Issues that can be fixed without background knowledge of Psalm good first issue Help wanted internal stubs/callmap
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants