Add error for assert()

[kkmuffme]

By default when using opcache, all assert() are non-op https://www.php.net/manual/en/function.assert.php

We should add an error for this specifically, in case people aren't aware, as assert() might be used to fix any errors psalm reports, but using assert() isn't actually fixing these issues once the application goes "live"

Theoretically one could argue that just adding assert to ForbiddenFunctions would suffice, however I think a distinct error would make sense, as this should be "forbidden" by default (and only allowed by suppressing the error globally for assert)

Psalm itself is significantly faster when enabling opcache, therefore it shouldn't rely on assert() itself either, therefore an error would come in handy too.

[psalm-github-bot[bot]]

Hey @kkmuffme, can you reproduce the issue on https://psalm.dev? These will be used as phpunit tests when implementing the feature or fixing this bug.

[weirdan]

Can you provide several examples that you think should be flagged by Psalm? I think I disagree with you, but would like to have some concrete examples to discuss.

With that said, assert handling in Psalm can definitely be improved. For example, it should flag non-pure expressions like this case: https://psalm.dev/r/239ac90ae0

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/239ac90ae0

<?php
interface Repo {
    public function fetch(int $id): Entity|null;
}

interface Entity {}

function fetchExisting(Repo $r, int $id): Entity {
    assert(($o = $r->fetch($id)) instanceof Entity);
    return $o;
}

Psalm output (using commit c488d40):

No issues!

[kkmuffme]

Can you provide several examples that you think should be flagged by Psalm?

All :)

zend.assertions is set to -1 in most production servers. Therefore all the assert calls are effectively removed and won't do anything.

Since zend.assertions can only be changed in the .ini (and not via ini_set), there is also a sizable number of development machines and some CI pipelines that have this value set to the production value of -1, therefore any assert in psalm itself are skipped when psalm is run on them and most people aren't aware that this could hide/trigger some errors when running psalm.

[weirdan]

/** @var */ comments don't do anything in PHP either (neither do @param or @return docblocks), yet we still use them. How's assert(), which at least runs in dev/test environments, any worse than them?

[kkmuffme]

Because the behavior of @var is the same on dev and production environments.

e.g. assert() can throw on dev environments, which can be caught in the caller and handled, but will run as if it didn't exist in production environments.

To quote the PHP docs for assert:

they should not be used for normal runtime operations As a rule of thumb code should behave as expected even if assertion checking is deactivated.

Basically, it's a simple way of creating "works on my machine" bugs.

[weirdan]

which can be caught in the caller and handled

Sounds like a misuse of assertions to me. As a rule of thumb assertion errors should never be caught (unless you really really really know what you're doing). It's an immediate red flag during code review.

Let's use an example (intentionally questionable): https://psalm.dev/r/bd4207801c How would you propose to express a zero-cost no-op check (in production) that nevertheless crashes tests (in dev/test)?

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/bd4207801c

<?php
interface Repo {
    public function fetch(int $id): Entity|null;
}

interface Entity {}

function fetchExisting(Repo $r, int $id): Entity {
    $o = $r->fetch($id); // I *know* I'm fetching an existing entity. 
                         // Perhaps I validated it before calling this method
    assert($o instanceof Entity); // I want this check eliminated in production, because of the above
                                  // But I want it to run in dev/test, to validate my assumption 
                                  // and *crash* the tests if it doesn't hold
                                  // It's not a 'normal runtime operation' because it should never happen
    return $o; // I don't want Psalm to complain here, because I *know* $o is always an Entity
}

Psalm output (using commit c488d40):

No issues!

[kkmuffme]

Valid point.

The problem I see is with what you wrote in your example:

because it should never happen

It shouldn't happen but it can happen - assert creates a false sense of security/strict typing, but in fact isn't any better than @var for production. Handling the type and e.g. throwing an error is preferred, isn't it? Since it works the same on dev & production.