InvalidOperand for int values in string concatenations

[iquito]

With Psalm 6.7.1 there are a lot of InvalidOperand errors for using ints in strings like this in my project:

https://psalm.dev/r/fc959f2373

These are mostly used (in my case) in error messages and exception messages, and using an int there in a string context is perfectly safe (and converting it to a string does not change anything about the code, the output, or make it any safer). The Psalm documentation for InvalidOperand shows a syntax error as an example (https://psalm.dev/docs/running_psalm/issues/InvalidOperand/) which is definitely worthy of catching, so turning off all InvalidOperand issues for a project seems like a pity, as I would miss actual issues, but I see no point in the errors for using a integer in an error message.

Can this be turned off or toggled specifically for such cases? Or maybe introduce a different issue than InvalidOperand for harmless string concatenations - because I see reasons to give issues for string concatenations when they produce a syntax error or an output that is not helpful (like concatenations of null and bool I would like to prevent, as it loses information, where int or float does not lose information in the same way).

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/fc959f2373

<?php

$year = 2015;

echo 'Error for year ' . $year;

Psalm output (using commit 0405632):

ERROR: InvalidOperand - 5:26 - Cannot concatenate with a int

[haas-dtv]

Same issue here.

The change was introduced between 6.6.2 and 6.7.0.

I suspect this was caused by enabling strictBinaryOperands by default in #11283, which doesn't seem like a good idea to me. Something like echo 'Hello' . 42; is perfectly valid PHP code.

[iquito]

Ah yes, setting strictBinaryOperands to false in the config gets rid of these checks, thanks for the hint!

I think this should at least be mentioned in the docs for InvalidOperand, as it is not clear what this issue covers or how to disable it for certain checks. I definitely agree that something like echo 'Hello' . 42; should not be an issue enabled by default, as there is no risk or problem there.

[orklah]

This in indeed the strictBinaryOperands change that enabled those. I agree that it's an opiniated check but it's easy to disable (while it's hard to know it exists in the first place if you would want it enabled)

It should be documented better though, if anyone want to help enriching the doc, it's here: https://github.com/vimeo/psalm/blob/6.x/docs/running_psalm/issues/InvalidOperand.md

About the "it's perfectly safe" stance, I'm not sure I agree. I mean yeah, of course it's safe when you don't make mistakes and you specificly wanted to concatenate an int and a string, but static analyzers are there to find when you're making a mistake. So if you screw up and use a non-formatted float on an invoice, it's nice to have something backing you up. I'm not sure I will keep it enable for my job because we have more urgent priorities, but some codebase will benefit having this I think

[haas-dtv]

About the "it's perfectly safe" stance, I'm not sure I agree. I mean yeah, of course it's safe when you don't make mistakes and you specificly wanted to concatenate an int and a string, but static analyzers are there to find when you're making a mistake. So if you screw up and use a non-formatted float on an invoice, it's nice to have something backing you up. I'm not sure I will keep it enable for my job because we have more urgent priorities, but some codebase will benefit having this I think

Would it make sense to allow concatenations between ints and strings, at least? I agree that concatenating floats or booleans and so on can lead to problems, but ints should be pretty safe, right?

With our codebase, I think we would like to enable strictBinaryOperands by default (since the rest of the checks seem useful), but we're getting way too many false positives for the string-int case outlined above.

[iquito]

About the "it's perfectly safe" stance, I'm not sure I agree. I mean yeah, of course it's safe when you don't make mistakes and you specificly wanted to concatenate an int and a string, but static analyzers are there to find when you're making a mistake. So if you screw up and use a non-formatted float on an invoice, it's nice to have something backing you up. I'm not sure I will keep it enable for my job because we have more urgent priorities, but some codebase will benefit having this I think

I can't think of an example where a string concatenation with an int would be a bug and cause problems (with a float like in your example one can argue about it), and string casts for all those ints make a codebase less readable, not more readable. Does strictBinaryOperands check something else than just concatenation of int and string? If so, then that would be a pity, because if I use a bool, null or even a float in a concatenation I would like to know, as you usually lose information when doing that. From the documentation and looking at the examples and some of the Psalm code I am not sure if those are covered by strictBinaryOperands. Including the int-concatenation-check will lead many developers to turn off the check entirely, or to add explicit casts all over the codebase for error/exception messages and other common occurances, both seem like a bad outcome and not an improvement to me.

Having a non-helpful check like this int-concatenation-check and enabling it by default seems like a bad direction for Psalm to take, as it will just waste a lot of time for no gain whatsoever (and that is coming from somebody who loves static analyzers and uses Psalm and PHPStan on all projects).

[danog]

+1 on splitting out or removing the int-string concat check, kind of overlooked it when enabling it

[danog]

Fixed in 6.8.6