Add @psalm-require-usage annotation and report UnusedFunctionCall for @psalm-taint-escape calls

[kkmuffme]

By default psalm considers all functions as impure and UnusedFunctionCall errors are only emitted for pure functions, since pure functions have no side-effects and therefore their return value must be used (otherwise the function call is useless)

There are some cases, where you want to force people to use the return value of a function call even if it's not pure - e.g. for escaping functions or when upgrading legacy code bases where a function returned void/never previously, but now returns a value that must get validated. Currently, there is no valid way to do that (you could add @psalm-pure and suppress that on the function, but that will lead to other issues)

I propose a new @psalm-require-usage to get this behavior. (I'd prefer @psalm-require-use however I think that might lead to confusion with @use/@template-use/@psalm-use)

Additionally, anything annotated with @psalm-taint-escape will also report unused function calls by default, since the escape function call only makes sense if the returned value is used.

Since phpstan doesn't offer this functionality in the first place (since by default it treats everything as pure https://phpstan.org/writing-php-code/phpdocs-basics#impure-functions), there are no PHP eco-system big picture concerns either.

---

Summary:

• fix @psalm-pure + @psalm-assert-if-true not reporting UnusedFunctionCalls on methods https://psalm.dev/r/7093ba0a35 (already worked for functions https://psalm.dev/r/afb33d970e)
• fix methods that return void should not be reporting UnusedMethodCall since the return value can't be used
• report unused function/method calls if the function is annotated with @psalm-require-usage or has a @psalm-taint-escape annotation

[weirdan]

Overall this seems useful.

Am I right the proposed changes allow @psalm-pure + @psalm-assert-if-true function results to be unused? If so, I don't think that's right. Unlike @psalm-assert, which can be thought of as having a side effect (on type inference), side effects from @psalm-assert-if-true are conditional on using the return value, and require return value to be used to be realized. Thus https://psalm.dev/r/afb33d970e should still report UnusedFunctionCall, and it looks like a part of the proposed changes would prevent that.

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/afb33d970e

<?php

/**
 * @psalm-pure
 * @psalm-assert-if-true int|string $val
 */
function acceptable(mixed $val): bool {
    return is_string($val) || is_int($val);
}

acceptable($argv[0]);

Psalm output (using commit c488d40):

ERROR: UnusedFunctionCall - 11:1 - The call to acceptable is not used

[kkmuffme]

@weirdan thanks, in that case I actually got it the wrong way round then - bc atm there is a bug that your example when wrapped in a class doesn't report an error: https://psalm.dev/r/7093ba0a35 - I assumed this is correct there and just ported that part from Methods to Functions - turns out this was a bug in methods - which I fixed now.

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/7093ba0a35

<?php

class Foo {
	/**
     * @psalm-pure
     * @psalm-assert-if-true int|string $val
     */
    public function acceptable(mixed $val): bool {
        return is_string($val) || is_int($val);
    }    
}

$foo = new Foo();
$foo->acceptable($argv[0]);

Psalm output (using commit c488d40):

No issues!

[kkmuffme]

@weirdan could you please review if it's alright for you like that now - then I will remove all the unused @psalm-suppress in unrelated code too, before it gets merged (just want to avoid unnecessary re/undos)

[ohader]

This seems to be very useful! 👍 We had to deal with similar patterns in 3rd party code analyzing security vulnerabilities - like visualized in this example:

https://psalm.dev/r/4daa849fa9

/**
 * @psalm-require-usage
 */
class AccessDeniedResponse implements ResponseInterface {}

Questions concerning this PR

• Would it quality to introduce a new CodeIssue class for that, e.g. MissingRequiredUsage instead of UnusedFunctionCall
• Would it be possible to extend @psalm-require-usage to class instances that need to be used? Thus, basically making it available for other concepts (classes, variables, ...) as well...

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/4daa849fa9

<?php

/** PSR-7/PSR-15 stubs */

interface ResponseInterface {}
interface RequestHandlerInterface {}
interface ServerRequestInterface {}
class Response implements ResponseInterface
{
    public function __construct(string $message, int $code) {}
}


/** --- actual static analysis --- */

/**
 * @psalm-require-usage
 */
class AccessDeniedResponse implements ResponseInterface
{
    public function __construct(string $message, int $code) {}
}

class SomeController implements RequestHandlerInterface
{
    public function handle(ServerRequestInterface $request): ResponseInterface
    {
		$this->authorize();
        // the possible `AccessDeniedResponse` of method `authorize` is not used,
        // as a result the un-authorized process execution continues...
        return new Response('All good... Here are the secrets...', 200);
    }

    private function authorize(): ?ResponseInterface
    {
        if ($this->anyAuthorizationCheck() === false) {
            return new AccessDeniedResponse('Forbidden', 403);
        }
        return null;
    }

    private function anyAuthorizationCheck(): bool
    {
        return false;
    }
}

Psalm output (using commit c488d40):

ERROR: InvalidDocblock - 19:7 - Unrecognised annotation @psalm-require-usage in docblock for AccessDeniedResponse

ERROR: InvalidDocblock - 19:1 - Unrecognised annotation @psalm-require-usage

[weirdan]

@ohader the thing you're describing looks much more complicated then what's proposed here. While a return value usage is fairly easy to define (any read operation), usage of an atomic in a union is much less clear. E.g. would all of the following constitute usage of AccessDenied?

https://psalm.dev/r/fcb87e545b

Also note that in all of those cases we don't really see AccessDenied in any return type, only its parent.

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/fcb87e545b

<?php

interface Response {}

final class AccessDenied implements Response {}
final class Ok implements Response {}

function authorize(): ?Response {
    return rand(0, 1) ? null : (rand(0, 1) ? new AccessDenied : new Ok);
}

function f1(): void {
    if (authorize() === null) {
        // was Response used here?
        echo 'secrets';
    }
}

function f2(): void {
    if (authorize()) {
        // was it used here?
        throw new Exception;
    }
    echo 'secrets';
}

function f3(): void {
    if (!authorize() instanceof Response) {
        // what about this?
        echo 'secrets';
    }
}

Psalm output (using commit c488d40):

No issues!

[ohader]

@weirdan Thanks for providing the examples! I agree that my idea would make it much more complicated. Besides that, I did not want to block this change nor request any changes in particular. Sorry for hijacking this PR with my own agenda...

[kkmuffme]

@weirdan added errors for invalid annotations and updated tests. It's done.

[kkmuffme]

@weirdan can you please merge this

[kkmuffme]

May I ask for a code review :)