@psalm-assert with side effects

[AndrolGenhald]

Sometimes I have a function that makes assertions about a type that also has other side effects, such as a function that takes a mixed argument and asserts it is a string is less than a maximum length.

If I annotate this with @psalm-assert string $arg, psalm will complain about RedundantConditionGivenDocblockType whenever I call it on something I already know is a string, but I still want to call it because it has the additional side effect of throwing an exception if the string is too large, as well is if it's not a string.

It would be nice if there were a way to tell psalm that such side effects exist so that it can ignore the function for the RedundantConditionGivenDocblockType check.

[psalm-github-bot]

Hey @AndrolGenhald, can you reproduce the issue on https://psalm.dev ?

[orklah]

Please provide an example of code

[AndrolGenhald]

I guess my use case is more about having a function that is more strict than it's @psalm-assert annotation: https://psalm.dev/r/c0165e28b9

But the same issue exists with side-effects: https://psalm.dev/r/9dbaa9a230

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/c0165e28b9

<?php

/**
 * @psalm-assert-if-true string $val
 */
function is_string_with_length(mixed $val, int $min, int $max): bool
{
    if (is_string($val)) {
        $len = strlen($val);
        
        return $min <= $len && $len <= $max;
    }
    
    return false;
}

function takesString(string $val): string
{
    return $val;
}

/** @var string */
$foo = "might be a long string that is too long to store in a database column or something";
if (is_string_with_length($foo, 0, 100)) {
    takesString($foo);
}

Psalm output (using commit 8bd525a):

ERROR: RedundantConditionGivenDocblockType - 24:5 - Docblock-defined type string for $foo is always string

https://psalm.dev/r/9dbaa9a230

<?php

class Foo
{
    public ?string $bar = null;
    
    /**
     * @psalm-assert string $val
     */
    public function baz(mixed $val): void
    {
        if (!is_string($val)) {
            throw new \InvalidArgumentException();
        }
        
        $this->bar = $val;
    }
}

$foo = new Foo();
$bar = "bar";
$foo->baz($bar); // If this call is removed $foo->bar will be different, so the call isn't redundant

Psalm output (using commit 8bd525a):

ERROR: RedundantCondition - 22:7 - Type "bar" for $bar is always string

[weirdan]

Basically we need a way for Psalm to distinguish purely asserting functions (those that exist only to assert things declared in @psalm-assert) and those that do something else as well (where assertion could be just a side effect, similar to how treat calls to functions in strict_types=1 mode).

[AndrolGenhald]

@weirdan That's a much better explanation. Actually, I think if we treated assertion functions as having side effects unless the function is also @psalm-pure that would work perfectly.

Maybe @psalm-assert should imply @psalm-pure, but there needs to be some way of removing that implication, like having @psalm-impure or @psalm-assert-impure? I'm not sure if it would be good or bad to have assertion functions default to warning about impurity.

[AndrolGenhald]

In #8378 we have functions that are a good candidate for using @psalm-assert with a conditional type. The functions are pure, but the assertion should never be redundant, so I don't think @psalm-assert-impure is the way to go. Maybe @psalm-assert-imprecise since the assertion is less precise than what the function is actually checking (due to more specific types not actually being possible)?

[orklah]

yeah, except that in the exemple, sometimes it also asserts that the type is more precise so I don't like imprecise terminology (and honestly, I also don't like we would end up with @psalm-assert-imprecise-if-true)

What we're dealing with in 8378, is pretty different to me. We're actually abusing the assertion module (that was used to express that the goal of the function was to refine the type of a param) to describe a mere side effect or an implied truth about the function

In that regard, most calls to str_starts_with or similar could end up Redundant from the point of view of the assertion module and this is the core of the issue.

So, there are two solutions to find:

• find how to express an implied assertion (that would be different from an assertion in the sense that it doesn't generate redundant errors). Ideally, this would not get in the way with the current keywords. Maybe an additional tag like @psalm-side-assert or something?
• find how to actually implement such a bypass because the assertion module doesn't care where the assertion is coming from (probably with a flag in Assertion)

[AndrolGenhald]

I don't like imprecise terminology

I don't like any of the names I've been able to come up with 🙁 Naming things is hard.

We're actually abusing the assertion module (that was used to express that the goal of the function was to refine the type of a param) to describe a mere side effect or an implied truth about the function

It's far from the only case of that though, I think there might actually be some like that that are already merged, but I might be misremembering. There are several similar cases waiting on this issue.

Maybe an additional tag like @psalm-side-assert or something?

I'm mostly ambivalent between an additional tag vs a different assert tag like @psalm-assert-???? Foobar $baz. The additional tag has the benefit of not also having to do @psalm-assert-???-if-true, so maybe that way is better?

I think part of the problem with naming this is that it can serve several purposes, the two I'm thinking about right now being functions that assert a type for a variable but also have side effects, and functions like str_starts_with where the assertion is more specific, but it has to assert non-empty-string because there's no string-starts-with<TNeedle> type. I think naming it based on what it does is probably better than naming it based on what it's used for, so maybe you have a @psalm-assert tag, but next to it you have an additional @psalm-assert-ignore-redundant tag?