#7387 Add asserting non-empty-string by strlen

[LeoVie]

This solves #7387.

I'm not quite sure, if I catched all cases... What do you think?

[weirdan]

All tests show strlen() call on the left-hand side of the expression; it would be great to have at least some of them checking the other direction, e.g. 0 < strlen($str).

Does it handle (or ignore) expressions as strlen() arguments? E.g. strlen("a" . $str . "b") > 2?

[LeoVie]

Indeed, having strlen on the right-hand side or calling strlen with a concatenated string like in your example does not work, yet. I will take a look at that. :)

[LeoVie]

I implemented both cases. There is some code redundancy between strlen and count assertions, now. That could be removed by extracting generic methods. But I am not sure, if this would lead to any benefit here, as the current implementation is very verbose and therefore quite easy to unterstand.

[weirdan]

Isn't it identical to strlenRighthandWithGreaterZero?

[LeoVie]

In fact, it is. I remove that case.

[weirdan]

I think you can remove this case and keep preventStrlenWithConcatenatedString only.

[LeoVie]

Removed that case, too.

[weirdan]

Did you mean

Suggested change

	throw new UnexpectedValueException('$count_equality_position value');
	throw new UnexpectedValueException('$strlen_equality_position value');

?

[LeoVie]

Yes, thank you. :)

[weirdan]

Also, can you check if Shepherd failures are actual code issues?

https://github.com/vimeo/psalm/actions/runs/3559331115/jobs/5980058302

[LeoVie]

Also, can you check if Shepherd failures are actual code issues?

https://github.com/vimeo/psalm/actions/runs/3559331115/jobs/5980058302

What's your suggestion on these issues?
They say, the call to getArgs and then accessing the first array item is not safe. That's right, in general. But here in this case, we know, that the expression is a strlen call, which always has exactly one argument.

/** @var PhpParser\Node\Expr\FuncCall $strlen_expr */
$var_name = ExpressionIdentifier::getExtendedVarId(
    $strlen_expr->getArgs()[0]->value,
    $this_class_name,
    $source
);

I've seen similar errors in psalm-baseline.xml. Should I just add the false-positives there?

[orklah]

the expression is a strlen call, which always has exactly one argument

It will have exactly one parameter on valid code yes, but Psalm is used to detect invalid code :)

It's also used to analyse code being typed which may miss parts to be correct. Trying to access an undefined offset would be risky.

We should probably try to fix the ones in the baseline and avoid adding new ones :) Just check isset($strlen_expr->getArgs()[0]) and skip the assertion if it returns false

[LeoVie]

Ah, okay, I thought psalm (respectively php-parser) would assume the code to be syntactically correct. As that does not seem to be the case, I will add the checks you proposed. Thank you :)

[orklah]

php-parser checks that the syntax is correct but having the correct number of argument is not a syntax issue. Psalm will check that the number of arguments is correct but it's its own thing that won't particularly block assertions to be processed anyway

[LeoVie]

I've added the proposed checks.

[LeoVie]

Ah, missed one check. Psalm now reports no PR related errors locally, anymore.
Please give the workflow another try.

[orklah]

There you go :)

CI is failing, not because of your change directly, but because there is an assertion in a loop that messes with other condition in that same loop.

@muglug if you could enlighten us on what can be done to avoid this kind of error, it would be great. I had the same kind of errors when working on integer ranges and I never could fix them correctly

[LeoVie]

Hi, is there anything I can do to make this PR mergable? :)

[orklah]

Yeah, put the error in the baseline, the behaviour is not related to this PR and is already broken anyway, it may be fixed later

[LeoVie]

Hi, @orklah, I've added the two errors to the baseline. CI hopefully gets green, now :)

[orklah]

Thanks!

[weirdan]

This is causing a lot of issues, so I'm going to revert it and reopen the original ticket.