False positive: mixed is a reserved word

[derrabus]

Reproducer: https://github.com/derrabus/psalm-mixed-reproducer

When installing Symfony Cache 6 in a project, I get a strange error:

ERROR: ReservedWord - src/MyClass.php:12:19 - mixed is a reserved word (see https://psalm.dev/095)
        if (1 === $cache->getItem(__FUNCTION__)->get()) {

As you can see, the highlighted code does not contain the mixed keyword at all.

If I downgrade Symfony cache to 5.4, the error is gone.

[psalm-github-bot]

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

[derrabus]

I don't know how, dear bot.

[weirdan]

symfony/cache:6 is PHP8 only, right? But your project is set to check against 7.4.

Running with --php-version=8.0 gets rid of the issue.

[weirdan]

Psalm 4.14 will make it more apparent: https://twitter.com/psalmphp/status/1464772918563557377/photo/1, but otherwise I don't think there's anything to fix here.

[derrabus]

symfony/cache:6 is PHP8 only, right? But your project is set to check against 7.4.

Right, but then again, Psalm is supposed to check my code for errors and not the dependencies'. Also, it is odd that this is the only error reported: Symfony 6 uses quite some more PHP 8 language features, why is Psalm complaining about the mixed type only?

If you want a more real-life example, we've found this in the CI of https://github.com/doctrine/dbal. Here, the codebase has to remain compatible with PHP 7.3, but allows dependencies that operate on a higher language level.

Running with --php-version=8.0 gets rid of the issue.

But this is not what I want. If my codebase would really use the mixed keyword, I certainly want to know. Should I run Psalm twice with different sets of dependencies or what would your recommondation be in that regard?

I don't think there's anything to fix here

Even if I was just using the tool wrong, I still believe that the output should be different:

• Psalm reports an error that originates in one of my dependencies.
• Psalm highlights a piece of code that is perfectly fine.

[weirdan]

If my codebase would really use the mixed keyword, I certainly want to know.

It's not about the keyword, it's about the type. And yes, it's your codebase that uses the class (as that's the only way :mixed can be interpreted under PHP 7.4) that does not exist in the version you're targeting - you've been given an instance of it by CacheItem::get(), and you're comparing it to 1.

Should I run Psalm twice with different sets of dependencies or what would your recommondation be in that regard?

If dependencies change depending on the PHP version - then not only you need to run Psalm multiple times, you also need to run composer install and your test suite with all the PHP versions you need to support. And looking at your CI setup it appears you already do that, with lowest and highest versions. Psalm does need to inspect your dependencies to know what they accept and what they return, and it can only read what's there on disk.

Symfony 6 uses quite some more PHP 8 language features, why is Psalm complaining about the mixed type only?

Because in this case it's the only thing that crossed the boundary between your dependency and your code. Perhaps other features did not manifest in method signatures - but this one did.

[derrabus]

Thanks for taking your time explaining this issue to me. ❤️

If my codebase would really use the mixed keyword, I certainly want to know.

It's not about the keyword, it's about the type.

That's not what the error message says.

And yes, it's your codebase that uses the class (as that's the only way :mixed can be interpreted under PHP 7.4) that does not exist in the version you're targeting - you've been given an instance of it by CacheItem::get(), and you're comparing it to 1.

Understood. But shouldn't I rather be getting an error telling me that strictly comparing an instance of that mixed class to an integer is pointless?

I understand that we should probably run Psalm with different settings than we currently do, but the output I receive is really confusing.

Anyway, thank you very much. I think, I have the answer I was looking for.

[weirdan]

But shouldn't I rather be getting an error telling me that strictly comparing an instance of that mixed class to an integer is pointless.

I suppose you would get that error if class existed, and wasn't named 'mixed'. Psalm just thinks it found a more serious issue that makes reporting other potential issues in that expression superfluous.

Reporting UndefinedClass here would be technically correct, but I guess it was judged that errors like 'UndefinedClass: class void does not exist' were too confusing (similar to https://3v4l.org/qktSv#v7.0.33), and it was better to flag potential keywords with a separate issue type (this also allows you to selectively suppress it).

[stof]

To me, it is still confusing that the code inside vendor is analyzed using the PHP version taken from the min php requirement of the root package. Either psalm should use the current php version (the one that was used by composer to resolve dependencies, and so compatible with the vendor folder), or it should do this guessing based on the composer.json separately for each vendor package (so that symfony/cache 6 would be parsed with the PHP 8 rules and so recognizing that mixed there does not mean a class name but the mixed type)

[orklah]

Please note that the feature that takes the min php version from composer.json was just designed for ease of use, especially for users that are beginners in using Psalm and don't have yet all the configs in mind.

It is just a default php version that makes sense for a lot of users to analyse with.

Now the case for libraries is different. In a majority of cases, the library have to support more than one version of PHP and may want to be forward compatible.

If that's the case, there is more than keywords that can change in dependencies. Old versions of dependencies may have a completely different API, or simply a different param type or return type.

It feels weird to focus on the fact that mixed is not really a reserved keyword and not consider at all that, if the dependencies were really installed in PHP 7.3, the param that's mixed in PHP 8.1 could have been a string at that time.

tl;dr: If you run Psalm in version 7.3 against PHP 8.1 dependencies, Psalm may miss a lot of issues. I think a sane way to approach this would be to run composer with --prefer-lowest and analyse with 7.3, then run normally and analyse with 8.1.

[stof]

tl;dr: If you run Psalm in version 7.3 against PHP 8.1 dependencies, Psalm may miss a lot of issues. I think a sane way to approach this would be to run composer with --prefer-lowest and analyse with 7.3, then run normally and analyse with 8.1.

if that's the case, the php version guessing based on the min php requirement in the composer.json does more harm than good, because the dependencies in vendor are not selected based on that min requirement but based on the current platform. It would be better to analyze by default against the current PHP version then.

[orklah]

It would be better to analyze by default against the current PHP version then.

Psalm currently requires at least PHP 7.1 to run, but it supports analysis up(down?) to PHP 5.3. That means any default value based on the php version used to run psalm would be automatically bogus for those.
Also setting the php-version to the environment version would allow your project to use features and keyword that does not exists yet if you're not careful to analyse the project in the min version allowed in composer (that was the previous default and judging by feedbacks like this #6950 (comment) it really helps people)

Starting from next version, Psalm will start displaying the version used to analyze the code and the source of the version (thanks to #7006). This looks like Target PHP version: 7.3 (inferred from composer.json) Would it have helped you? Do you have a suggestion on what we could add after that to help people understand what's going on under the hood?

[stof]

Well, analysing my own code with my min version makes indeed sense as a default. The issue is that it also gets applied to the vendor folder, while those packages were not installed based on that min version and so have a different one.
So to me, the good behavior would be to parse each package based on their own min PHP version (which is mostly impacting how reserved keywords are parsed as native types AFAICT). This would avoid reporting bogus errors due to parsing the vendor files in the wrong way.

The fact that an older version of Symfony supporting PHP 7.3 might have a different return type than mixed in its signature is an unrelated topic. The same could be true for types that are supported on both versions. So it definitely makes sense for a library to analyse with several versions of their dependencies installed (i.e. analysing with --prefer-lowest too), but that's not related to the PHP version range.

Starting from next version, Psalm will start displaying the version used to analyze the code and the source of the version (thanks to #7006). This looks like Target PHP version: 7.3 (inferred from composer.json) Would it have helped you? Do you have a suggestion on what we could add after that to help people understand what's going on under the hood?

This would ot have helped me understand the error in the reproducer, (well, maybe a little by guessing the cause of the issue in psalm, but then that would have only allowed to report it more precisely from the start, but not to fix it in the project), as the bogus message is related to parsing the dependency files in a wrong way.

[weirdan]

I'd like to understand how we can improve ux (ruling out, for a moment, ideas like different PHP versions per dependency).

Say, if we checked minimum PHP versions allowed by all installed dependencies and added a notice if they are higher then the inferred target version - would it have helped?

Alternatively, what if we defaulted to the PHP version Psalm runs with, but added a notice saying that your project should also check with the lowest version it specifies in composer.json?

[stof]

Say, if we checked minimum PHP versions allowed by all installed dependencies and added a notice if they are higher then the inferred target version - would it have helped?

This could have helped understand the issue, but not fixing it. For now, the only fix I see is either ignoring the error in the config as done by Doctrine, or analysing the project based on the current PHP version (with which the deps are compatible).

Alternatively, what if we defaulted to the PHP version Psalm runs with, but added a notice saying that your project should also check with the lowest version it specifies in composer.json?

if you cannot parse the dependencies based on their own min PHP version, this looks like the best behavior, as it will produce a working analysis.
We should still find a way for this warning to be opted-out IMO, so that projects running multiple CI jobs on different PHP versions can disable this warning (as they are doing that check in the other CI job)

[weirdan]

We should still find a way for this warning to be opted-out IMO, so that projects running multiple CI jobs on different PHP versions can disable this warning (as they are doing that check in the other CI job)

The warning would only be there if you did not specify the version, either through CLI switch or config.

[stof]

@weirdan but that's the whole point: those CI jobs would not specify the version. They would have different CI jobs using a different "current version" (and so having vendors compatible with that version)

[weirdan]

If different CI jobs use different PHP versions then they would not have any problem passing --php-version=x.x when they invoke Psalm.

[loevgaard]

For people coming by this issue, here is an example of a github action job doing what @weirdan suggests:

# ...

jobs:
    static-code-analysis:
        runs-on: "ubuntu-latest"

        strategy:
            matrix:
                php-version:
                    - "7.4"
                    - "8.0"
                    - "8.1"
                    - "8.2"

                dependencies:
                    - "lowest"
                    - "highest"

        steps:
            -   name: "Checkout"
                uses: "actions/checkout@v3"

            -   name: "Setup PHP, with composer and extensions"
                uses: "shivammathur/setup-php@v2"
                with:
                    coverage: "none"
                    php-version: "${{ matrix.php-version }}"
            
            -   name: "Install composer dependencies"
                uses: "ramsey/composer-install@v2"
                with:
                    dependency-versions: "${{ matrix.dependencies }}"

            -   name: "Static analysis"
                run: "vendor/bin/psalm --php-version=${{ matrix.php-version }}"