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

strpos/stripos should assert non-empty-string like str_contains #9753

Closed
kkmuffme opened this issue May 6, 2023 · 9 comments
Closed

strpos/stripos should assert non-empty-string like str_contains #9753

kkmuffme opened this issue May 6, 2023 · 9 comments
Labels
easy problems Issues that can be fixed without background knowledge of Psalm enhancement good first issue Help wanted internal stubs/callmap

Comments

@kkmuffme
Copy link
Contributor

kkmuffme commented May 6, 2023

https://psalm.dev/r/2f458027ac

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/2f458027ac
<?php

/**
 * @var string $a
 * @var string $b
 */

if ( str_contains( $a, 'foo' ) ) {
 	/** @psalm-trace $a */;   
}

if ( strpos( $b, 'foo' ) !== false ) {
 	/** @psalm-trace $b */;   
}

if ( stripos( $b, 'foo' ) !== false ) {
 	/** @psalm-trace $b */;   
}
Psalm output (using commit 0ea2a6a):

INFO: Trace - 9:25 - $a: non-empty-string

INFO: Trace - 13:25 - $b: string

INFO: Trace - 17:25 - $b: string

kkmuffme added a commit to kkmuffme/psalm that referenced this issue May 6, 2023
@orklah
Copy link
Collaborator

orklah commented May 6, 2023

seems easy to implement:

* @psalm-assert-if-true =(T is '' ? string : non-empty-string) $haystack

@orklah orklah added enhancement easy problems Issues that can be fixed without background knowledge of Psalm internal stubs/callmap Help wanted good first issue labels May 6, 2023
@robchett
Copy link
Contributor

I'm not sure this is a simple fix as the return type of str(i)pos is false|int, there would need to be an assert-if-int annotation?

@orklah
Copy link
Collaborator

orklah commented May 14, 2023

Some tests would be needed but it could still work with assert-if-true because Psalm does not check for true but check if you will enter the condition or not, so it's more a assert-if-truthy

@robchett
Copy link
Contributor

int is not truthy because of 0 and 0 is a valid return when the haystack starts with the needle.

Is it currently possible to do input type narrowing in a ReturnTypeProvider?

@orklah
Copy link
Collaborator

orklah commented May 14, 2023

0 is not truthy, but for things like

if(strpos(...)){ }
or
if(strpos(...) !== false){ }
it should still work as intended, but yeah, it's a dirty hack.

It could probably be possible to narrow the type from a ReturnTypeProvider but it may be hard. You do have the context so you know the type of each variable in the caller context and you can change them

@kkmuffme
Copy link
Contributor Author

Why not just do it the other way round? assert-if-false -> if it's false, we cannot infer anything, but in the opposite case we know it's non-empty?

@robchett
Copy link
Contributor

assert-if-false would require changing the type to non-empty-string in the default case then widening when false which would require multiple annotations if it worked at all

/** 
 * @psalm-assert non-empty-string $haystack
 * @psalm-assert-if-false string $haystack
 */

Orklah's "hack" did indeed work he said it would so I've opened #9788

@orklah
Copy link
Collaborator

orklah commented Jun 15, 2023

That was fixed :)

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

No branches or pull requests

3 participants