Allow stringable objects in sprintf() values

[MidnightDesign]

No description provided.

[MidnightDesign]

Another option would be to use stringable-object and allow it in any version because that's also worked forever.

[orklah]

yeah, stringable-object is more correct here I think

[MidnightDesign]

@orklah I've updated the PR.

[MidnightDesign]

Well, that's tricky. What should we do with the failing tests? Now it is an implicit cast, which might not be what the developer intended. That's why I used the (somewhat more explicit) Stringable in the first place.

Why not just leave it as-is and disallow any implicit cast?

I'm also using Infection, which has a "remove the type cast" mutator. So there's a tension between the two tools and I have to decide which one to suppress.

Both Psalm and Infection are somewhat right, in my opinion. On one hand, you probably don't want to accidentally cast an object to a string. On the other hand, you don't need to litter your code with string casts of Stringables if the outcome is the same. I'm leaning more towards Infection's side here, at least when the object explicitly implements Stringable.

@maks-rafalko Do you have any opinions here?

[orklah]

Wow, a lot of things to unpack here.

First thing, I'm not too happy to see that Stringable and stringable-object don't behave similarly here (given that tests runs on a recent PHP version, I'd expect no difference in behavior). This is probably a bug that should be fixed

I told you to use stringable-object because I'm not actually sure what using Stringable does when analyzing an old codebase (that shouldn't be aware of the symbol Stringable or may have implemented it themselves), so I think stubs designed for every php version should avoid using it.

Second thing, I disagree with the current failing test implicitCastWithStrictTypesToEchoOrSprintf.

While passing an object to sprintf is absolutely an implicit cast to string, it's not what the issue exists in Psalm for.

The doc gives us a snippet as an example: https://psalm.dev/docs/running_psalm/issues/ImplicitToStringCast/

While the code runs absolutely fine, there's an issue when we use strict_types on the snippet: https://3v4l.org/M1Wr5

The example crashes because the function expects a string (and not a stringable-object or a Stringable)

That's what the issue is for, so that people who want to catch that kind of thing can spot and fix them.

However, sprintf doesn't expect a string: https://3v4l.org/v3tbR so it doesn't fail, even in strict_types. So there's no reason to have this error pops up here and infection is probably correct by telling you it's redundant (unlike the first snippet I shared where you are required to add a cast to string to resolve the issue)

So, you adding stringable-object is actually a good thing and the failing test should be removed (though it doesn't explain why the test was not failing when you explicitly allowed Stringable, it will have to be investigated...)

[MidnightDesign]

So the rule is meant to specifically emulate the behavior of adding strict_types? Then I agree, that's reasonable. I've removed the test.

[orklah]

So the rule is meant to specifically emulate the behavior of adding strict_types

I'm not sure it was ever explicited like that but it's the only way it would make sense. Psalm is made to work almost the same with or without strict_types, however, the implicit cast to string is only made without strict_types, which makes Psalm in a tricky position. We have to warn users that this may break but they need to be able to easily suppress this if they don't want to care about strict_types

Otherwise, requiring a cast to string is more a code style rule than a static analysis one

[orklah]

Thanks!