Skip to content

implement str-case functions return type extension #1325

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

Merged
merged 9 commits into from
May 18, 2022

Conversation

staabm
Copy link
Contributor

@staabm staabm commented May 17, 2022

$accessoryTypes[] = new AccessoryNonEmptyStringType();
}
if ($argType->isLiteralString()->yes()) {
$accessoryTypes[] = new AccessoryLiteralStringType();
Copy link
Member

Choose a reason for hiding this comment

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

Don't sneak that in here, @craigfrancis would disagree ;)

Copy link
Contributor Author

@staabm staabm May 18, 2022

Choose a reason for hiding this comment

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

I thought about the same thing and was not sure. but then I found

if ($arrayType->getIterableValueType()->isLiteralString()->yes() && $separatorType->isLiteralString()->yes()) {
$accessoryTypes[] = new AccessoryLiteralStringType();
}
which made me think this one here is fine.

maybe it is also wrong there?

Copy link
Member

Choose a reason for hiding this comment

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

implode is one of the four functions that can produce literal strings as blessed by the original RFC: https://wiki.php.net/rfc/is_literal

Copy link
Contributor

@craigfrancis craigfrancis May 18, 2022

Choose a reason for hiding this comment

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

Yep, sorry, the only modification you can do that keep the literal-string flag is concatenation (this was done to help adoption, and is seen as acceptable by several security people, like the developers behind go-safe-html; where str_repeat(), str_pad(), implode(), and join() are all glorified forms of concatenation)... other modifications no longer result in a "fixed value in source code" (ref); and while the security properties of some functions (like strtoupper) might be ok, I would like to avoid the discussion of what functions are allowed vs what aren't :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx for the insights

@staabm staabm marked this pull request as ready for review May 18, 2022 07:49
staabm referenced this pull request in phpstan/phpstan-doctrine May 18, 2022
@staabm staabm mentioned this pull request May 18, 2022
@ondrejmirtes
Copy link
Member

@staabm Please send a PR to phpstan-doctrine after this is merged :)

if (count($strings) === 1) {
return $strings[0];
}
return new UnionType($strings);
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the if here. Using TypeCombinator::union() is more correct. You can have an edge case of calling lcfirst of 'foo'|'Foo' and the current code would result in 'foo'|'foo' which is bad :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh wow... how do you come up with such ideas...? well spotted!

Copy link
Member

Choose a reason for hiding this comment

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

I guess the experience from doing this every day for the past 6 years 😅

@ondrejmirtes ondrejmirtes merged commit 476a6b1 into phpstan:1.7.x May 18, 2022
@ondrejmirtes
Copy link
Member

Thank you!

@staabm staabm deleted the str-casing branch May 18, 2022 08:34
@craigfrancis
Copy link
Contributor

I might be misreading something, but this seems to have changed with 1.7.x-dev (9ad2792):
https://phpstan.org/r/0dc8ccd0-37bb-49c8-bf2c-6bfb3ed459f0

@staabm
Copy link
Contributor Author

staabm commented May 18, 2022

I might be misreading something, but this seems to have changed with 1.7.x-dev (9ad2792):

what exactly do you mean?

@craigfrancis
Copy link
Contributor

I'm trying to work on a different thing (using templates), so it might not be related... but lines 19-21 are doing what I expect with the output being seen as a string, but to do that I need to add something else to the strtolower() (I've used time)... the lines below, where I was using strtolower('B'), that's now being seen as a literal-string in this context.

@ondrejmirtes
Copy link
Member

Because strtolower('B') becomes 'b' and that's a literal string. If you want to test against non-literal-string, you now need some code that PHPStan isn't able to figure out, like calling a function that just has string return type.

@craigfrancis
Copy link
Contributor

Cool, thanks, I'll update some tests :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False-positive on strtolower() when explicit strings are required
4 participants